[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov created this revision. ilya-biryukov added reviewers: sammccall, hokein. Herald added subscribers: jfb, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. This takes ~5% of time when running clangd unit tests. Repository: rG LLVM Github Monorepo https://reviews

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Also considered moving the std symbol mapping into a separate (private-to-implementation) class, e.g. we don't use suffix mappings and symbol mappings outside system includes. But decided to wait until suffix mapping are going away completely. Repository: rG LL

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

hokein added a comment. In D67172#1657590 , @ilya-biryukov wrote: > Also considered moving the std symbol mapping into a separate > (private-to-implementation) class, e.g. we don't use suffix mappings and > symbol mappings outside system includes. > Bu

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Ok, agreed. Adding this indirection layer is probably making things a bit too complex. I'll update the patch to clearly separate the std symbol mapping and canonical includes. In D67172#1660435 , @hokein wrote: > And I'm n

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov updated this revision to Diff 219061. ilya-biryukov added a comment. - Move system symbol mapping to a different class. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172 Files: clang-tools-ext

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Not sure whether the new code is a win overall, it's still a bit messy Let me know what you think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172 __

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

hokein added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:780 + // Prefer a mapping from the system symbols. + if (SystemSymbols) { +if (auto Result = SystemSymbols->mapHeader(Header, QualifiedName)) what's the interesti

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

hokein added a comment. In D67172#1660469 , @ilya-biryukov wrote: > Ok, agreed. Adding this indirection layer is probably making things a bit too > complex. I'll update the patch to clearly separate the std symbol mapping and > canonical includes. > > I

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:780 + // Prefer a mapping from the system symbols. + if (SystemSymbols) { +if (auto Result = SystemSymbols->mapHeader(Header, QualifiedName)) hokein wrote:

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

sammccall added a comment. I had the same reaction as Haojian to the original patch, thanks for clearing that up. I do wonder whether we're microoptimizing for the tests too much, I don't think 5% on the tests is worth very much in itself, unless it's speeding up real workloads or improving th

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. In D67172#1662888 , @sammccall wrote: > I do wonder whether we're microoptimizing for the tests too much, I don't > think 5% on the tests is worth very much in itself, unless it's speeding up > real workloads or improving t

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

sammccall added a comment. In D67172#1662916 , @ilya-biryukov wrote: > In D67172#1662888 , @sammccall wrote: > > > I do wonder whether we're microoptimizing for the tests too much, I don't > > think 5% on the test

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. In D67172#1662945 , @sammccall wrote: > Only if it's 5% of something meaningful. If the denominator isn't something > we care about, then it's really "spending XXX usec is not ok" - which depends > on XXX I think. Agree,

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov updated this revision to Diff 219335. ilya-biryukov added a comment. - Alternative approach - do not add extra classes, aim to minimize changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. The new version looks very much like the older one, with a few additions to not re-initialize data twice. PTAL and let me know what you think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:748 +llvm::StringRef CanonicalPath = Pair.second; +int Components = std::distance(llvm::sys::path::begin(Suffix), +

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LG, though if we can drop the struct and make MaxSuffixComponents a constant it'd be simpler still. In D67172#1662957 , @ilya-biryukov wrote: >

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. In D67172#1663096 , @sammccall wrote: > LG, though if we can drop the struct and make MaxSuffixComponents a constant > it'd be simpler still. Done. > Sure. This is going to win a couple of percent I guess: for these cases

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

sammccall added a comment. In D67172#1663179 , @ilya-biryukov wrote: > > "we could try" sounds like we *don't* know how to eliminate it. Parsing > > manpages aside, I thought the main problem was these symbols are > > nonstandard and an infinitely porta

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov updated this revision to Diff 219360. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Make MaxSuffixComponents a constant - Put the suffix mapping into a single constant - Initialize all StringMaps directly - Rename to Std...Mapping Repository: rG

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov marked 8 inline comments as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:41 + if (!SuffixHeaderMapping) +return Header; sammccall wrote: > nit: can we write `if (SuffixHeaderMappi

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Thanks for all suggestions. The final result is rather small and minimal. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172 ___ cfe

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

This revision was automatically updated to reflect the committed changes. Closed by commit rL371408: [clangd] Use pre-populated mappings for standard symbols (authored by ibiryukov, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: h