[clang][deps] Generate command lines lazily #65691
Conversation
| @@ -532,7 +539,8 @@ ModuleDepCollectorPP::handleTopLevelModule(const Module *M) { | |||
| // Finish the compiler invocation. Requires dependencies and the context hash. | |||
| MDC.addOutputPaths(CI, MD); | |||
|
|
|||
| MD.BuildArguments = CI.getCC1CommandLine(); | |||
| MD.BuildInvocationOrArguments = | |||
| std::make_unique<CowCompilerInvocation>(std::move(CI)); | |||
There was a problem hiding this comment.
You have shared_ptr but call make_unique; it works but seems unexpected
There was a problem hiding this comment.
Good catch, I'll align it with whatever pointer/value type we decide to go with.
| std::variant<std::shared_ptr<CowCompilerInvocation>, std::vector<std::string>> | ||
| BuildInvocationOrArguments; |
There was a problem hiding this comment.
The size of ModuleDeps used to be 200B. Storing CowCompilerInvocation directly would increase that to 360B, while just pointing to it keeps it at 208B. WDYT about the trade-offs?
Also, this should semantically be std::unique_ptr, since we're not sharing the ownership. But I'm not a fan of the move-only semantics that implies. I could wrap it in a unique-ptr-like type that's copyable?
There was a problem hiding this comment.
Also, this should semantically be std::unique_ptr, since we're not sharing the ownership. But I'm not a fan of the move-only semantics that implies. I could wrap it in a unique-ptr-like type that's copyable?
Not sure I understand this - if you copy it you spend the same storage as if it was inlined, so it seems like either you want shared_ptr or inline storage.
I think shared_ptr is reasonable so this can be easily copied around.
There was a problem hiding this comment.
My concern is that if I use std::shared_ptr<CowCompilerInvocation>, ModuleDeps stops having value semantics. I guess that would be solved by making it a std::shared_ptr<const CowCompilerInvocation> to prevent mutation of the invocation.
Yes, both inlined and indirect storage use the same amount of storage (+- 8B), but I was thinking about this from memory locality perspective (storing the rarely used invocation away from frequently used data). But I guess this data structure is full of pointers to heap anyways, so that doesn't really matter.
I guess I'll just go with the inline approach to save on boilerplate.
jansvoboda11
force-pushed
the
lazy-command-line-gen
branch
from
223e26a
to
6b8c1ee
Compare
This patch makes the generation of command lines for modular dependencies lazy/on-demand. That operation is somewhat expensive and prior to this patch used to be performed multiple times for the identical `ModuleDeps` (i.e. when they were imported from multiple different TUs).
This patch makes the generation of command lines for modular dependencies lazy/on-demand. That operation is somewhat expensive and prior to this patch used to be performed multiple times for the identical `ModuleDeps` (i.e. when they were imported from multiple different TUs).
This patch makes the generation of command lines for modular dependencies lazy/on-demand. That operation is somewhat expensive and prior to this patch used to be performed multiple times for the identical
ModuleDeps(i.e. when they were imported from multiple different TUs).