[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev added a subscriber: alexfh. v.g.vassilev added a comment. @alexfh, could you test that patch on your internal infrastructure and check if it actually decreases the memory footprint and/or compile times? I believe this was the only blocker to land it as it is. CHANGES SINCE LAST AC

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev added a comment. In D41416#4496389 , @ChuanqiXu wrote: > In D41416#4496293 , @v.g.vassilev > wrote: > >> In D41416#4492367 , @v.g.vassilev >> wrote: >> >>> Add

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added a comment. In D41416#4496293 , @v.g.vassilev wrote: > In D41416#4492367 , @v.g.vassilev > wrote: > >> Address most of the comments. I will need some help with the >> `Modules/pr60085.cppm` failure

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev added a comment. In D41416#4492367 , @v.g.vassilev wrote: > Address most of the comments. I will need some help with the > `Modules/pr60085.cppm` failure. I suspect we pass to CodeGen some > instantiation by iterator index and that does not

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 539670. v.g.vassilev added a comment. Fix the odr_hash.cpp failure. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41416/new/ https://reviews.llvm.org/D41416 Files: clang/include/clang/AST/DeclTemplate.h clang/lib/AST/DeclTemplate.cpp cla

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 539664. v.g.vassilev added a comment. Fix a compilation error. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41416/new/ https://reviews.llvm.org/D41416 Files: clang/include/clang/AST/DeclTemplate.h clang/lib/AST/DeclTemplate.cpp clang/li

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added a comment. > I will need some help with the Modules/pr60085.cppm failure. I am looking at https://reviews.llvm.org/D154914 so maybe I can't look this deeply now. If there are concrete and small issues, maybe I can try to take a look. Hope this helps. Comment

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev added inline comments. Comment at: clang/test/Modules/odr_hash.cpp:2905 // expected-error@second.h:* {{'PointersAndReferences::S5::x' from module 'SecondModule' is not present in definition of 'PointersAndReferences::S5' in module 'FirstModule'}} -// expected-note

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev added inline comments. Comment at: clang/lib/AST/DeclTemplate.cpp:337 +void RedeclarableTemplateDecl::loadLazySpecializationsImpl( + bool OnlyPartial/*=false*/) const { // Grab the most recent declaration to ensure we'v

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 539409. v.g.vassilev marked 7 inline comments as done. v.g.vassilev added a comment. Address most of the comments. I will need some help with the `Modules/pr60085.cppm` failure. I suspect we pass to CodeGen some instantiation by iterator index and that

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev added inline comments. Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:288 + } + for (auto &SpecInfo : LazySpecializations) { +Record.push_back(SpecInfo.DeclID); SAtacker wrote: > ChuanqiXu wrote: > > v.g.vassilev wrote: > > >

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added a comment. In D41416#4488517 , @Hahnfeld wrote: > In D41416#4487516 , @ChuanqiXu wrote: > >> But some local in tree tests got failed. I took some time to investigate >> them but I didn't get insight

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:786-788 +uint32_t DeclID = ~0U; +unsigned ODRHash = ~0U; +bool IsPartial = false; Hahnfeld wrote: > ChuanqiXu wrote: > > Maybe this can save some space. > Can you elab

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

Hahnfeld added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:786-788 +uint32_t DeclID = ~0U; +unsigned ODRHash = ~0U; +bool IsPartial = false; ChuanqiXu wrote: > Maybe this can save some space. Can you elaborate why this would sa

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

shafik added inline comments. Comment at: clang/lib/AST/DeclTemplate.cpp:337 +void RedeclarableTemplateDecl::loadLazySpecializationsImpl( + bool OnlyPartial/*=false*/) const { // Grab the most recent declaration to ensure we've load

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

SAtacker added inline comments. Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:288 + } + for (auto &SpecInfo : LazySpecializations) { +Record.push_back(SpecInfo.DeclID); ChuanqiXu wrote: > v.g.vassilev wrote: > > We should not store the l

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

Hahnfeld added a comment. In D41416#4487516 , @ChuanqiXu wrote: > But some local in tree tests got failed. I took some time to investigate them > but I didn't get insights : ( If the failures involve template template parameters, please note that you ne

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added a comment. I tested this with our internal use cases with modules and it looks good. Things get compiled correctly and we saved 10% compilation time. (May be due to different code bases). But some local in tree tests got failed. I took some time to investigate them but I didn't

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:786-788 +uint32_t DeclID = ~0U; +unsigned ODRHash = ~0U; +bool IsPartial = false; Maybe this can save some space. Comment at: clang/lib/AST/DeclTemp

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 538633. v.g.vassilev added a comment. Rebase. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41416/new/ https://reviews.llvm.org/D41416 Files: clang/include/clang/AST/DeclTemplate.h clang/lib/AST/DeclTemplate.cpp clang/lib/AST/ODRHash.cpp

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev added inline comments. Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:288 + } + for (auto &SpecInfo : LazySpecializations) { +Record.push_back(SpecInfo.DeclID); We should not store the lazy specialization information as an ar

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

ChuanqiXu added a reviewer: ChuanqiXu. ChuanqiXu added a comment. Add me as a reviewer to add this to my stack. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41416/new/ https://reviews.llvm.org/D41416 ___ cfe-commits mailing list cfe-commits@

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 509998. v.g.vassilev added a comment. Herald added a subscriber: mgrang. Rebase. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41416/new/ https://reviews.llvm.org/D41416 Files: clang/include/clang/AST/DeclTemplate.h clang/lib/AST/DeclTempl

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 137902. v.g.vassilev added a comment. Add sanity check. We assert that the ODR hash of the template arguments should be the same as the one for from the actually found template specialization. This way we managed to catch a few collisions in the ODRHas

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

rsmith added a comment. I just tried this on one of our larger builds. The good news is that it appears to work: the compilation still succeeds, and significantly fewer nodes are deserialized. The bad news is that the final .cc file compilation got slower, from 42s to 86s. (There's a lot of noi

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev added a comment. Finer grained clang stats can be seen here . https://reviews.llvm.org/D41416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 128802. v.g.vassilev added a comment. Reduce further the deserializations from 451 to 449 by providing a more complete implementation of `ODRHash::AddTemplateArgument`. Kudos @rtrieu! https://reviews.llvm.org/D41416 Files: include/clang/AST/DeclTem

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 128797. v.g.vassilev added a comment. Reduce hash collisions for template specializations. Currently when we hash a tag type the visitor calls `ODRHash::AddDecl` which mostly relies on the decl name give distinct hash value. The types coming from templ

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 128657. v.g.vassilev added a comment. Reverse template specialization order. https://reviews.llvm.org/D41416 Files: include/clang/AST/DeclTemplate.h lib/AST/DeclTemplate.cpp lib/Serialization/ASTReader.cpp lib/Serialization/ASTReaderDecl.cpp

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 128655. v.g.vassilev added a comment. Zero `IsPartial` and improve comments in assert and free text. https://reviews.llvm.org/D41416 Files: include/clang/AST/DeclTemplate.h lib/AST/DeclTemplate.cpp lib/Serialization/ASTReader.cpp lib/Serializat

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev added a comment. I have created a simple (-ish) benchmark targeted to stress test modules with a good number of template specializations. I use the mp11 library from boost. I am building two modules which contain around 1K specializations (mostly

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 128645. v.g.vassilev added a comment. Loading of lazy partial template specializations should not trigger loading of full template specializations. Often during selection process we need to load all partial template specializations. It is very rare to

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 128575. v.g.vassilev marked 4 inline comments as done. v.g.vassilev added a comment. Address comments: - Fix style; - Do not potentially deserialize a specialization in debug mode. https://reviews.llvm.org/D41416 Files: include/clang/AST/DeclTemplat

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

rsmith added a comment. This is looking like a really good start; we'll still want to do something about partial specializations, but this should already help a lot. Can you produce some sample performance numbers for this change? This is replacing a linear-time algorithm (with a high constant)

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev added inline comments. Comment at: lib/Serialization/ASTReaderDecl.cpp:92 for (unsigned I = 0, Size = Record.readInt(); I != Size; ++I) -IDs.push_back(ReadDeclID()); +IDs.push_back(LazySpecializationInfo(ReadDeclID(), Record.readInt())); }

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 128465. v.g.vassilev added a comment. Teach `ASTReader::CompleteRedeclChain` to load only the template specializations with the same template argument list. https://reviews.llvm.org/D41416 Files: include/clang/AST/DeclTemplate.h lib/AST/DeclTempla

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 128460. v.g.vassilev marked 2 inline comments as done. v.g.vassilev added a comment. Address inline comments: order the read operations. https://reviews.llvm.org/D41416 Files: include/clang/AST/DeclTemplate.h lib/AST/DeclTemplate.cpp lib/Serializ

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

rsmith added a comment. I think you should track lazy partial specializations separately from lazy full specializations -- we need to load all the partial specializations when doing partial specialization selection, and don't want to load all full specializations at the same time. ==

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev updated this revision to Diff 127606. v.g.vassilev added a reviewer: rtrieu. v.g.vassilev added a comment. Fixed a comment typo. Repository: rL LLVM https://reviews.llvm.org/D41416 Files: include/clang/AST/DeclTemplate.h lib/AST/DeclTemplate.cpp lib/Serialization/ASTReader

[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

v.g.vassilev created this revision. v.g.vassilev added reviewers: rsmith, bruno. Currently, we load all lazy template specializations when we search whether there is a suitable template specialization for a template. This is especially suboptimal with modules. If module `B` specializes a templat