allocate the underlying data of Template kind separately, this would reduce AST memory usage
- TemplateArgumentLocInfo 24 => 8 bytes
- TemplateARgumentLoc 48 => 32 bytes
- DynTypeNode 56 => 40 bytes
Paths
| Differential D87080
[AST] Reduce the size of TemplateArgumentLocInfo. ClosedPublic Authored by hokein on Sep 3 2020, 2:58 AM.
Details Summary allocate the underlying data of Template kind separately, this would reduce AST memory usage
Diff Detail
Event TimelineComment Actions Nit on the commit message: I think this is TemplateArgumentLoc 48 -> 32 bytes, and DynTypedNode 56 -> 40 bytes.
hokein marked 2 inline comments as done. Comment Actionsaddress review comments:
Comment Actions Looks good apart from a minor technical issue with the traits. Would it be possible to compile some big file in LLVM (probably doesn't matter much which, Sema something?) and observe if there's a significant change in overall ASTContext size?
Comment Actions
~3% saving (measuring the ASTContext::.getASTAllocatedMemory)
rsmith added inline comments.
Comment Actions Forgot to mention, 3% memory saving is huge, way more than I expected (was mostly just hoping for no regression).
This revision is now accepted and ready to land.Sep 21 2020, 2:46 AM This revision was landed with ongoing or failed builds.Sep 21 2020, 4:09 AM Closed by commit rGaf29591650c4: [AST] Reduce the size of TemplateArgumentLocInfo. (authored by hokein). · Explain Why This revision was automatically updated to reflect the committed changes. hokein marked an inline comment as done.
Revision Contents
Diff 290188 clang/include/clang/AST/TemplateBase.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/TypeLoc.cpp
clang/lib/Sema/SemaTemplate.cpp
clang/lib/Sema/SemaTemplateDeduction.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/lib/Sema/SemaTemplateVariadic.cpp
clang/lib/Sema/TreeTransform.h
clang/lib/Serialization/ASTReader.cpp
|
At first glance this is unsafe: you could have two different definitions of the same specialization in different files.
In fact it's OK, the default one can now never be instantiated, because Expr.h includes this file and so anyone that can see the definition can see this specialization.
But this is subtle/fragile: at least it needs to be spelled out explicitly in the comment.
I'd also suggest adding a static assert below the definition of Expr that it is compatible with this specialization (because it is sufficiently aligned).
(I can't think of a better alternative - use of PointerUnion is a win, partly *because* it validates the alignment)