[PATCH] D89749: SourceManager: Don't allocate an SLocEntry until it's loaded

Mon Oct 26 08:59:45 PDT 2020

dexonsmith planned changes to this revision.
dexonsmith added a comment.

In D89749#2353651 <https://reviews.llvm.org/D89749#2353651>, @v.g.vassilev wrote:

> Thanks for the patch!! This is a super hot place for us (mostly due to boost). I will try it on our end and let you know!

Great!

In D89749#2353602 <https://reviews.llvm.org/D89749#2353602>, @teemperor wrote:

> Maybe arc didn't correctly apply the patch, but this causes a few LLDB tests to fail on my machine:
>
>   lldb-api :: commands/expression/import-std-module/queue/TestQueueFromStdModule.py
>   lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py
>   lldb-api :: commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
>
> From the backtrace this seems like the ASTImporter needs to be updated:

Thanks for doing that legwork, I'll take a look.

> The tests are just loading a Clang module and then trying to import some Decls into another ASTContext, so I don't think the error here is specific to LLDB. We just don't have any ASTImporter tests that exercise modules.

I'll add one.

> FWIW, if you quickly want to see if your SourceManager changes break LLDB, then you can just set the LIT_FILTER to "Modules" and run `check-lldb` as LLDB's use of Clang modules are the only place where we have any valid SourceLocations within LLDB.

This is helpful, thanks. I also need to deal with whatever entitlements `check-lldb` needs (new machine since the last time and I remember it being non-trivial) but I'll figure that out.

> 1. The `I - 1` everywhere when translating `SLocEntryLoaded` values to a `LoadedSLocEntryTable` index is a bit cryptic. [...]

Great comment; I think the way I'll do this is to create a simple wrapper type that degrades to `Optional<unsigned>`.

> 2. If we have this as an Optional, we might as well make the sentinel value `std::numeric_limits<unsigned>::max()` instead of 0 in `SLocEntryLoaded`. Then we can just all values as-is without translating them to an index.

Could do, but there's a tradeoff since then a `resize` operation can't/doesn't use zero-initialization. As a result I have a (weak) preference for a 0-sentinel (probably my next patch will keep that semantic), but I'm open to changing it.

> 3. Now that the primary purpose of `SLocEntryLoaded` is storing indices and not the loaded status, maybe something like `SLocEntryIndices` would be a better name?

Good idea.

> 4. Do you still have your benchmarking values (or some estimates) for this? I'm just curious how much memory this actually saves.

I'll find some numbers if Vassil hasn't shared his by the time I've fixed the patch.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89749/new/

https://reviews.llvm.org/D89749