Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
After looking at the numbers, I think we should make this configurable. I'll update this change accordingly. Not sure what should be the default, though. I'm currently erring on the side of "in-memory by default".
Current implementation eats almost twice as much memory, which quickly adds up (tried comparing with earlier builds on ClangdUnit.cpp).
Even without proper benchmarks, it's clear that if we run out of memory (and we easily could if run on a machine with, say, 8GB of RAM) on-disk preambles are an easy way out of trouble.
Heh, I was just going to ask :-)
As a very first step, can we make this configurable but off-by-default? That will address the pressing diskless-servers need.
In a typical workstation scenario, disk is plentiful (if slow) and RAM is scarce - very likely we're running browser + editor (VSCode is another browser really) + clangd + build system, and clangd is likely the least important of these...
So if we're going to increase mem usage by a significant amount I think we want to measure it carefully first.
I think on-disk by default makes sense now until we do the measurements and have a the data to prove the on-memory is really better.
As discussed, we should probably bundle together some related options into structs to make plumbing easier. Not a blocker here though.
If these options are pure-data, I think it's OK if some layers end up with access to other layers' options.
clangd/ClangdUnit.cpp | ||
---|---|---|
238 ↗ | (On Diff #122845) | ref is still here :-) |
clangd/tool/ClangdMain.cpp | ||
48 ↗ | (On Diff #122845) | Couple of questions about the flag syntax:
|
- Removed /*ref*/.
- Changed the command-line flag: it's -pch-storage now instead of -in-memory-pchs.
- Actually pass the StoreInMemory flag to PrecompiledPreamble::Build.
- Fixed a new usage of ClangdServer after rebase (In ClangdTests).
clangd/ClangdUnit.cpp | ||
---|---|---|
238 ↗ | (On Diff #122845) | Thx. Missed that. Removed. |
clangd/tool/ClangdMain.cpp | ||
48 ↗ | (On Diff #122845) |
|