Skip to content

[lldb] Implement cache for swift typeref metadata #5100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 18, 2022
Merged

[lldb] Implement cache for swift typeref metadata #5100

merged 1 commit into from
Aug 18, 2022

Conversation

augusto2112
Copy link

No description provided.

@augusto2112
Copy link
Author

Leaving this as a draft for now as it depends on some upstream changes.

@augusto2112
Copy link
Author

Accompanying swift patch: swiftlang/swift#60497

adrian-prantl
adrian-prantl reviewed Aug 10, 2022
View reviewed changes
Copy link

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow that's quite a patch :-)
Some first thoughts inline!

lldb/source/Core/CoreProperties.td Outdated
def EnableSwiftMetadataCache: Property<"enable-swift-metadata-cache", "Boolean">,
Global,
DefaultFalse,
Desc<"Enable caching for swift reflection metadata in LLDB.">;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

above swift is spelled Swift

lldb/source/Core/ModuleList.cpp
nullptr, idx, g_modulelist_properties[idx].default_uint_value != 0);
}

uint64_t ModuleListProperties::GetSwiftMetadataCacheMaxByteSize() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should write macros for these functions :-)

@adrian-prantl
Copy link

Similarly to the clang module cache, we need to localize this cache to the test dir when running the LLDB test suite.

@augusto2112 augusto2112 force-pushed the swift-meta-cache branch 2 times, most recently from 8d4b8fb to aa092d6 Compare August 11, 2022 22:02
@augusto2112 augusto2112 marked this pull request as ready for review August 11, 2022 23:46
@augusto2112
Copy link
Author

swiftlang/swift#60497

@swift-ci test

@augusto2112 augusto2112 mentioned this pull request Aug 12, 2022
Merged
adrian-prantl
adrian-prantl reviewed Aug 12, 2022
View reviewed changes
lldb/source/Plugins/LanguageRuntime/Swift/SwiftMetadataCache.cpp
LLDB_LOG(log,
"[SwiftMetadataCache] Failed to read cached UUID for module {0}.",
module->GetFileSpec().GetFilename());
m_data_file_cache->RemoveCacheFile(module_name);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected RemoveCacheFile() to be called immediately before a new file is written. What's the flow if the cache is stale?
loadCacheForModule() deletes the cache, but where is a new one written?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new one is written in registerFieldDescriptors, which is when we have new data to cache. I think it makes sense to delete the cache immediately if it's stale/corrupted. Since we're including the lldb version as part of the hash now I don't think there would be any advantages in doing that later. Doing it earlier is also nice because we clear up disk space.

adrian-prantl
adrian-prantl reviewed Aug 12, 2022
View reviewed changes
lldb/source/Plugins/LanguageRuntime/Swift/SwiftMetadataCache.cpp
const auto *start = (const char *)header_extractor.GetData(&read_offset, 0);
// Create a reference to the compressed data.
llvm::StringRef string_buffer(start, (uint64_t)mem_buffer_up->getBufferEnd() -
(uint64_t)start);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

membuffer_up->getBuffer() ?
https://llvm.org/doxygen/classllvm_1_1MemoryBuffer.html#a2ea075bd68f15b57e95f0771b8ba0bca

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean getBufferSize? If so, we can't do that because we have to subtract the header we just read.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I didn't realize that start is not the start of the buffer,

@adrian-prantl
Copy link

To make sure this didn't get lost:

  • Similarly to the clang module cache, we need to localize this cache to the test dir when running the LLDB test suite.
  • I think we should add a test where we turn on logging, and build foo.dylib, log that the cache is created, rebuild foo.dylib with modification, log that the cache is deleted and recreated, then scan the log file for those log messages to test the basic cache management.

@augusto2112
Copy link
Author

Similarly to the clang module cache, we need to localize this cache to the test dir when running the LLDB test suite.

I don't understand what you mean by this. Do you mean when running tests in general? We can probably leave it off in general, and turn it on only when testing this feature.

@augusto2112 augusto2112 force-pushed the swift-meta-cache branch 2 times, most recently from e94cc80 to c3a8235 Compare August 13, 2022 00:49
@augusto2112
Copy link
Author

swiftlang/swift#60497

@swift-ci test

@adrian-prantl
Copy link

Similarly to the clang module cache, we need to localize this cache to the test dir when running the LLDB test suite.

I don't understand what you mean by this. Do you mean when running tests in general? We can probably leave it off in general, and turn it on only when testing this feature.

Yes, something like this: https://github.com/llvm/llvm-project/blob/cd3a234fa9272f90dcfad3158cd9b51a0e643d1b/lldb/packages/Python/lldbsuite/test/lldbtest.py#L729

Turning it off and on only for the test is fine, if we can make the testsuite faster by sharing a cache for an entire test suite run, then that's great, too.

adrian-prantl
adrian-prantl reviewed Aug 15, 2022
View reviewed changes
adrian-prantl
adrian-prantl reviewed Aug 16, 2022
View reviewed changes
adrian-prantl
adrian-prantl reviewed Aug 16, 2022
View reviewed changes
lldb/source/Core/ModuleList.cpp
if (llvm::sys::path::cache_directory(path)) {
llvm::sys::path::append(path, "lldb");
llvm::sys::path::append(path, "SwiftMetadataCache");
lldbassert(SetLLDBIndexCachePath(FileSpec(path)));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

adrian-prantl
adrian-prantl reviewed Aug 16, 2022
View reviewed changes
lldb/source/Plugins/LanguageRuntime/Swift/SwiftMetadataCache.h Outdated
/// A map from reflection infos ids to a pair constituting of its
/// corresponding module and whether or not we've inserted the cached metadata
/// for that reflection info already.
std::unordered_map<uint64_t, std::pair<lldb::ModuleSP, bool>>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this an unordered_map and not a std::vector<>? I would have expected the ids to be in a contiguous range?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be holes since we don't register JITed modules.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so the uint64_t is not the index that shows up in image list but the index of refkection sections we register with the runtime. The first 100 or so elements are identical, but that's it. And during a session the list grows steadily, and there could by dylibs loaded after a JIT session.

Last question: would a uint32_t suffice?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so, I doubt we'd ever have more than 4294967295 reflection infos :)

adrian-prantl
adrian-prantl reviewed Aug 16, 2022
View reviewed changes
adrian-prantl
adrian-prantl reviewed Aug 16, 2022
View reviewed changes
adrian-prantl
adrian-prantl approved these changes Aug 16, 2022
View reviewed changes
Copy link

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few minor nits inline

@augusto2112
Copy link
Author

swiftlang/swift#60497

@swift-ci test

1 similar comment
@augusto2112
Copy link
Author

swiftlang/swift#60497

@swift-ci test

@augusto2112
Copy link
Author

swiftlang/swift#60497

@swift-ci test

@augusto2112 augusto2112 force-pushed the swift-meta-cache branch 2 times, most recently from 61f06a3 to cdcb91a Compare August 18, 2022 04:07
@augusto2112
[lldb] Implement cache for swift typeref metadata
80e10a6 
Implement a mechanism to cache field descriptor location information.
One of the main problems of using type metadata as debug info, when in
comes to field descriptors in particular, is that there's no easy way
to find the correct type given a (string key), so a linear search over
every source of reflection information must be done. This patch
implements a cache which allows for field descriptors to be found in a
more efficient manner.
@augusto2112
Copy link
Author

swiftlang/swift#60497

@swift-ci test

@augusto2112
Copy link
Author

The new test is failing on linux because the bot doesn't have zlib installed, so the cache is disabled as well. Since I have a new patch, which doesn't require zlib, ready to be put up for review after this one is merged, I'm temporarily skipping linux, and will re-enable it on the next patch.

@augusto2112 augusto2112 merged commit 59f1187 into swiftlang:stable/20220421 Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrian-prantl adrian-prantl adrian-prantl approved these changes

@kastiglione kastiglione Awaiting requested review from kastiglione

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@augusto2112 @adrian-prantl