This is an archive of the discontinued LLVM Phabricator instance.

Added the ability to cache the finalized symbol tables subsequent debug sessions to start faster.
ClosedPublic

Authored by clayborg on Dec 8 2021, 12:27 AM.

Details

Summary

This is an updated version of the https://reviews.llvm.org/D113789 patch with the following changes:

  • We no longer modify modification times of the cache files
  • Use LLVM caching and cache pruning instead of making a new cache mechanism (See DataFileCache.h/.cpp)
  • Add signature to start of each file since we are not using modification times so we can tell when caches are stale and remove and re-create the cache file as files are changed
  • Add settings to control the cache size, disk percentage and expiration in days to keep cache size under control

This patch enables symbol tables to be cached in the LLDB index cache directory. All cache files are in a single directory and the files use unique names to ensure that files from the same path will re-use the same file as files get modified. This means as files change, their cache files will be deleted and updated. The modification time of each of the cache files is not modified so that access based pruning of the cache can be implemented.

The symbol table cache files start with a signature that uniquely identifies a file on disk and contains one or more of the following items:

  • object file UUID if available
  • object file mod time if available
  • object name for BSD archive .o files that are in .a files if available

If none of these signature items are available, then the file will not be cached. This keeps temporary object files from expressions from being cached.

When the cache files are loaded on subsequent debug sessions, the signature is compare and if the file has been modified (uuid changes, mod time changes, or object file mod time changes) then the cache file is deleted and re-created.

Module caching must be enabled by the user before this can be used:

symbols.enable-lldb-index-cache (boolean) = false

(lldb) settings set symbols.enable-lldb-index-cache true

There is also a setting that allows the user to specify a module cache directory that defaults to a directory that defaults to being next to the symbols.clang-modules-cache-path directory in a temp directory:

(lldb) settings show symbols.lldb-index-cache-path
/var/folders/9p/472sr0c55l9b20x2zg36b91h0000gn/C/lldb/IndexCache

If this setting is enabled, the finalized symbol tables will be serialized and saved to disc so they can be quickly loaded next time you debug.

Each module can cache one or more files in the index cache directory. The cache file names must be unique to a file on disk and its architecture and object name for .o files in BSD archives. This allows universal mach-o files to support caching multuple architectures in the same module cache directory. Making the file based on the this info allows this cache file to be deleted and replaced when the file gets updated on disk. This keeps the cache from growing over time during the compile/edit/debug cycle and prevents out of space issues.

If the cache is enabled, the symbol table will be loaded from the cache the next time you debug if the module has not changed.

The cache also has settings to control the size of the cache on disk. Each time LLDB starts up with the index cache enable, the cache will be pruned to ensure it stays within the user defined settings:

(lldb) settings set symbols.lldb-index-cache-expiration-days <days>

A value of zero will disable cache files from expiring when the cache is pruned. The default value is 7 currently.

(lldb) settings set symbols.lldb-index-cache-max-byte-size <size>

A value of zero will disable pruning based on a total byte size. The default value is zero currently.
(lldb) settings set symbols.lldb-index-cache-max-percent <percentage-of-disk-space>

A value of 100 will allow the disc to be filled to the max, a value of zero will disable percentage pruning. The default value is zero.

Diff Detail

Event Timeline

clayborg created this revision.Dec 8 2021, 12:27 AM
clayborg requested review of this revision.Dec 8 2021, 12:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2021, 12:27 AM
clayborg retitled this revision from Added the ability to cache the finalized symbol tables subsequent debug sessions to start faster. This is an updated version of the https://reviews.llvm.org/D113789 patch with the following changes: - We no longer modify modification times of the... to Added the ability to cache the finalized symbol tables subsequent debug sessions to start faster..Dec 8 2021, 12:28 AM
clayborg edited the summary of this revision. (Show Details)
clayborg edited the summary of this revision. (Show Details)
clayborg updated this revision to Diff 394041.Dec 13 2021, 1:36 PM

Added string tables to the cache files.

This allowed me to add the symbol table name indexes to the same symbol table cache file and share any strings from the normal symbol table.

wallace requested changes to this revision.Dec 13 2021, 4:36 PM

much nicer than the first version. I'm just asking a few minor things.

lldb/include/lldb/Host/FileSystem.h
147
lldb/include/lldb/Symbol/Symbol.h
256

better mention that the return value means

lldb/include/lldb/Symbol/Symtab.h
227

ditto

lldb/include/lldb/Utility/DataFileCache.h
59 ↗(On Diff #394041)
175 ↗(On Diff #394041)

I'd just call this ConstStringTable for callers to know that this is using ConstString as its storage pool and that the data doesn't just go away when the object is disposed.

lldb/source/Utility/DataFileCache.cpp
58 ↗(On Diff #394041)

could you create a new lldb log channel where this information is logged? it'll help investigating issues with the cache (if they ever happen). Same for everywhere llvm::Error's or lldb::Status messages are lost.

This revision now requires changes to proceed.Dec 13 2021, 4:36 PM
clayborg updated this revision to Diff 394141.Dec 13 2021, 10:31 PM

Address all review comments.

clayborg marked 6 inline comments as done.Dec 13 2021, 10:33 PM
clayborg added inline comments.
lldb/source/Utility/DataFileCache.cpp
58 ↗(On Diff #394041)

I didn't create a new channel because we are out of log bits. I can expand the log bits to 64 in a separate patch.

wallace accepted this revision.Dec 14 2021, 7:36 AM

lgtm

This revision is now accepted and ready to land.Dec 14 2021, 7:36 AM
labath requested changes to this revision.Dec 14 2021, 7:55 AM

The patch is slightly larger than I would prefer for a through review, but here's my first pass at it. I appreciate the difficulties in bootstrapping something like this incrementally, but I do see at least a couple of opportunities to make things smaller. I think the DataEncoder, FileSystem, Symbol, and Mangled changes could go into separate patch(es). I don't know if it's feasible to separate the construction of the symtab cache content, from the actual act of storing it in the cache, but if those two processes could be separated, it would help things a lot.

lldb/source/Host/common/FileSystem.cpp
523

What's the purpose of this check ?

lldb/source/Symbol/Symbol.cpp
645

This is quite dodgy. It would be better to change the storage so that we don't have to do this. Perhaps something like

union {
  uint16_t flag_storage;
  struct {
    uint16_t m_type_data_resolved : 1;
    ...
  };
};
lldb/source/Symbol/Symtab.cpp
1187

constexpr llvm::StringLiteral

1223

constexpr uint32_t

1245–1249

I am troubled by code like this. Have you tried creating a mock object file in the test?

1331

please clang-format the patch

lldb/source/Utility/DataFileCache.cpp
10–13 ↗(On Diff #394141)

These are not appropriate dependencies for code in the utility library. Either reimplement it without them or move the class somewhere else.

51 ↗(On Diff #394141)

[&] on a lambda that outlives the enclosing function is dangerous.

61–64 ↗(On Diff #394141)

there's an LLDB_LOG_ERROR macro for this

This revision now requires changes to proceed.Dec 14 2021, 7:55 AM
clayborg updated this revision to Diff 394442.Dec 14 2021, 6:23 PM
clayborg marked an inline comment as done.

Changes:

  • Updated Symtab class to not have to work around not having an object file and created a test with an object file
  • Don't use hack to encode Symbol bitfield values.
  • Remove file system IsDirectory check that wasn't needed
  • Switch to use constexpr llvm::StringLiteral where requested
  • Switch to use constexpr uint32_t for CURRENT_CACHE_VERSION
  • Moved DataFileCache to Core since it relies on ModuleList's properties
  • Fixed unsafe capture in lambda
  • Switch log calls in DataFileCache.cpp to use LLDB_LOG_ERROR
  • Updated test to create a module/object file from yaml to avoid having to hack Symtab to deal with NULL m_objfile
clayborg marked 6 inline comments as done.Dec 14 2021, 6:24 PM

Pavel: I fixed all issues you identified. Let me know if there is anything else you would like to see changed.

lldb/source/Host/common/FileSystem.cpp
523

I checked the llvm code, and it already correctly handles not deleting a file if it is a directory, so this isn't needed.

lldb/source/Symbol/Symbol.cpp
645

I encoded them manually now. It was too disruptive to the code to try and add an anonymous union with an anonymous struct as there were many compiler warnings about it being a GNU extension and made the code way too messy, especially in the constructor as you can't init each value easily.

Thanks for the quick response.

I did another pass over the patch, and I think I have a fairly good understanding of how it works. I have another round of comments, but nothing major.

lldb/include/lldb/Core/DataFileCache.h
10
130–133

the Optional class already has comparison operators that do the right thing.

lldb/include/lldb/Core/Mangled.h
72–73
lldb/source/Core/Module.cpp
1678

The cache key and cache signature algorithms are fairly similar but subtly different (IIUC, one is used to locate the cache entry on the disk and the second one to verify that it refers to the correct file). I think it'd be better to have the two functions next to each other, to highlight the differences. If you moved the signature construction function here (by changing it from a Module constructor to a CacheSignature getter function), then that would also the dependency problem I mentioned last time as I believe these constructors were the only non-utility dependency of that code (I don't think that code has to be moved back to Utility, but it *could* be moved, it need arises).

lldb/source/Symbol/Symbol.cpp
645

That works too.

649

I guess this isn't a "trick" anymore, just manual encoding of boolean bitfields into an int.

lldb/source/Symbol/Symtab.cpp
118–147

If I managed to understand what this is doing correctly, it could be more simply implemented as:

if (pair.second.IsEmpty())
  continue;
s->Printf("\n0x%2.2x name to index table:\n", pair.first);
std::map<uint32_t, std::vector<ConstString>> index_to_names;
for (const auto &entry: pair.second)
  index_to_names[entry.value].push_back(entry.cstring); // automatically creates a new map entry for the first string
for (const auto &pair: index_to_names) {
  s->Printf("0x%8.8x: ", pair.first);
  for (const auto &name: pair.second) // vector always non-empty
    s->Printf("\"%s\" ", name.AsCString());
  s->EOL();
}
272

I guess this is no longer relevant.

1182

this is not a good use of auto per the llvm coding standards

1189

add static (for the next function as well)

lldb/test/API/functionalities/module_cache/universal/TestModuleCacheUniversal.py
22

leftover from debugging?

58–59

IIUC, the python tests check the caching behavior, while the (c++) unit tests check that the actual contents of the cache entries can be read/written correctly. I like that.

lldb/unittests/Symbol/SymbolTest.cpp
186

Our c++ test layout mirrors the layout of the code it is testing, so this test should go to MangledTest.cpp and the next one to SymtabTest.cpp. (Although it might be nicer if all the (de)serialization code lived in a single central place, and then the tests for it would live in a single place as well.)

clayborg updated this revision to Diff 394656.Dec 15 2021, 2:00 PM
clayborg marked 7 inline comments as done.
  • Fixed all review issues
  • Moved tests to separate test files in SymbolTests
clayborg added inline comments.Dec 15 2021, 2:03 PM
lldb/source/Core/Module.cpp
1678

So the signature only contains an optional UUID, optional mod time, and optional object mod time, so they really are not related. The cache key must be the same for the same file on disk even if the file gets updated. So the cache key is designed to stay the same for the same file on disk, but also allow for the same file on disk to have more than one architecture (universal files) or for a file to contain multiple files like .a files that contain .o files. Since we only include the basename of the path for the module file, we also include the hash from Module::Hash() which _does_ include the full file path. That way we can have many "a.out" cache files from different directories and the main thing that will differ is the hash that is appended at the end.

The DataCacheFile.cpp was relying on the ModuleList.cpp in order to read the "symbols.* preferences via "ModuleList::GetGlobalModuleListProperties()", so the ModuleList is actually what was the part that violated the dependency issue.

lldb/source/Symbol/Symbol.cpp
649

It is. I tried doing your approach of making an anonymous union and an anonymous struct but I got a bunch of warnings that these were GNU extensions and I feared that some buildbots would fail with warnings as errors. I also didn't know if this would mess up the windows buildbots in case they didn't support these. So to be safe I just manually encode stuff to be safe.

lldb/source/Symbol/Symtab.cpp
118–147

Whoops this was just dumping code I added to verify that the name tables were being reproduced correctly. No one will want to see these. I will remove the code.

lldb/test/API/functionalities/module_cache/universal/TestModuleCacheUniversal.py
22

yes! good catch

58–59

Exactly. I wanted to make sure that encoding could be done with unit testing as that is easier. I could also verify that every bit in the Symbol class was encoded and decoded properly since I was playing tricks before with the bitfields, but it did help issues when I did things manually as well.

Pavel: I fixed all issues you identified. Let me know if there is anything else you would like to see changed.

labath accepted this revision.Dec 16 2021, 1:06 AM

I think this is in a pretty good shape now. Thanks for your patience.

lldb/source/Symbol/Symbol.cpp
649

Yes, I saw what you're doing, and that's fine. My point was that I would use the word "trick" to describe something clever/subtle/dangerous (which the original implementation was). This isn't tricky, its just tedious. :)

But that's not a big deal.

This revision is now accepted and ready to land.Dec 16 2021, 1:06 AM

The new test (TestModuleCacheSimple) is failing on the windows lldb bot. It looks like the deletion of the executable is failing due to permissions and other similar tests are skipped on windows for that reason. I haven't looked at the test in more detail to see if it would be appropriate to modify somehow so it would work:

https://lab.llvm.org/buildbot/#/builders/83/builds/12914

(The bot is rather angry at me for the VS2019 update, but everything should be working OK again)

The new test (TestModuleCacheSimple) is failing on the windows lldb bot. It looks like the deletion of the executable is failing due to permissions and other similar tests are skipped on windows for that reason. I haven't looked at the test in more detail to see if it would be appropriate to modify somehow so it would work:

https://lab.llvm.org/buildbot/#/builders/83/builds/12914

(The bot is rather angry at me for the VS2019 update, but everything should be working OK again)

Fixed with:

commit 59f1d0eed58ce0f23c3a5a324d122a99f9757228 (HEAD -> main, origin/main, origin/HEAD)
Author: Greg Clayton <gclayton@fb.com>
Date: Thu Dec 16 16:13:58 2021 -0800

Fix windows buildbots after https://reviews.llvm.org/D115324

Windows has trouble deleting the executable due to permissions.

The new test (TestModuleCacheSimple) is failing on the windows lldb bot. It looks like the deletion of the executable is failing due to permissions and other similar tests are skipped on windows for that reason. I haven't looked at the test in more detail to see if it would be appropriate to modify somehow so it would work:

https://lab.llvm.org/buildbot/#/builders/83/builds/12914

(The bot is rather angry at me for the VS2019 update, but everything should be working OK again)

Some mac buildbots were failing due to "lldb/test/API/functionalities/module_cache/universal/TestModuleCacheUniversal.py" failing to build the executable with the makefile due to lack of arm64 support in the SDK that was being used.

commit 2a844c8869909960fb006a668d30c7349f55ee7e (HEAD -> main, origin/main, origin/HEAD)
Author: Greg Clayton <gclayton@fb.com>
Date: Fri Dec 17 12:14:44 2021 -0800

Fix macOS buildbots after https://reviews.llvm.org/D115324.

The test was attempting to make a universal x86_64/arm64 binary, but some older bots don't have a macOS SDK that can handle this. Switching over to using a yaml file instead should solve the problem.

The unit testing I added to this patch was unable to catch a serious issue that I have fixed and is needed if anyone wants to enable this feature. So please make sure if you use this patch to also make sure that the following patch is also applied to your repository if you are not using top of tree:

https://reviews.llvm.org/D124572

Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2022, 4:50 PM

Been experimenting with this recently and I noticed that loading in the cached indexes seems to do a lot of loading - specifically interning a lot of strings from the index and the symtab. Does this happen when reading a built-in index (apple_names/debug_names) (I don't have an immediately easy way to test this, or I Would've before asking)? I'd be surprised if that was the case, which is also confusing me as to why it's the case for these cached indexes? I'd have expected the cached index to look basically the same as the apple_names/debug_names builtin index and have similar performance properties, but maybe that's not the case?

Been experimenting with this recently and I noticed that loading in the cached indexes seems to do a lot of loading - specifically interning a lot of strings from the index and the symtab. Does this happen when reading a built-in index (apple_names/debug_names) (I don't have an immediately easy way to test this, or I Would've before asking)? I'd be surprised if that was the case, which is also confusing me as to why it's the case for these cached indexes? I'd have expected the cached index to look basically the same as the apple_names/debug_names builtin index and have similar performance properties, but maybe that's not the case?

Yeah, the caches are currently designed to just serialize the cache to disc and allow it to be loaded into the same data structure as is used when the cache isn't used.

The DWARF indexes are more efficient where there is a base class that represents the cache, and the manual index will create its own data structures and then do lookups using the base class' API. It allows the API calls to do the lookups as efficiently as possible without interning any strings. An improvement to this caching could do the same kind of thing, but that isn't what we have right now. We would need to continue to serialize/deserialize the symbol table, but the symbol table index and debug info index could be modified to indirect the cache lookups through a base class so the serialized indexes could be made more efficient.

The index cache is purely a serialize/deserialize of the same cache structure that is used when manually parsing and indexing.