Cache container names in CheckedIndex
#1909
Conversation
It is important that we cache this because we might find a lot of symbols in the same container for eg. workspace symbols (eg. consider many symbols in the same C++ namespace). If we didn't cache this value, then we would need to perform a `primaryDefinitionOrDeclarationOccurrence` lookup for all of these containers, which is expensive. For example, searching for `only` in SourceKit-LSP’s codebase used to not return results in more than 20s (after which I gave up) and now returns >8000 results in <2s. rdar://141412138
|
@swift-ci Please test |
| /// Maps the USR of a symbol’s container the name of that container as well as the name of all containers the | ||
| /// container might itself be contained in. |
There was a problem hiding this comment.
| /// Maps the USR of a symbol’s container the name of that container as well as the name of all containers the | |
| /// container might itself be contained in. | |
| /// Maps the USR of a symbol to its name and the name of all its containers, from outermost to innermost. |
| return cached | ||
| } | ||
| let result: [String] | ||
| if let containerDefinition = primaryDefinitionOrDeclarationOccurrence(ofUSR: container.symbol.usr) { |
There was a problem hiding this comment.
This is still going to be fairly slow for decls with many occurrences. It's at least limited by def/decl, but even then for something like llvm that would still end up reading many records (eg. any file with namespace llvm { in it). Instead we could just use forEachSymbolOccurrence and stop at the first one, since we don't actually care about getting a consistent decl here.
Also, could we add tests for extensions? Eg.
struct Foo {
struct Bar {}
}
extension Foo.Bar {
func bar() {}
}
IIUC today this will just give Bar.bar rather than Foo.Bar.bar since bar has childOf to the Foo.Bar extension (which has name Bar) but that extension doesn't have any childOf to Foo.
(I'm fine with all that in a follow up/an issue, since this PR is mostly just moving code around)
There was a problem hiding this comment.
This is still going to be fairly slow for decls with many occurrences. It's at least limited by def/decl, but even then for something like
llvmthat would still end up reading many records (eg. any file withnamespace llvm {in it). Instead we could just useforEachSymbolOccurrenceand stop at the first one, since we don't actually care about getting a consistent decl here.
I just measured the performance impact of containerNames. When searching for only in SouceKit-LSP (roughly 8000 hits), we need ~800ms if we don’t compute the container names. Computing container names takes another ~800ms. If we use forEachSymbolOccurrence instead of primaryDefinitionOrDeclarationOccurrence we can save about 150ms of that container name computation.
There was a problem hiding this comment.
Forget about the exact measurements because I was using a debug build to gather them. But from a Release sample, container name computation still roughly doubles the time to search for only, which now takes ~800ms in total with your suggestion to use forEachSymbolOccurrence.
This improves performance when searching for `only` in SourceKit-LSP by ~30%.
|
@swift-ci Please test |
It is important that we cache this because we might find a lot of symbols in the same container for eg. workspace symbols (eg. consider many symbols in the same C++ namespace). If we didn't cache this value, then we would need to perform a
primaryDefinitionOrDeclarationOccurrencelookup for all of these containers, which is expensive.For example, searching for
onlyin SourceKit-LSP’s codebase used to not return results in more than 20s (after which I gave up) and now returns >8000 results in <2s.