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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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) with a quadratic-time algorithm (with a lower constant), and it'd be useful to confirm that's not going to be a pressing problem in the short term. (In the medium term, we should move to using an on-disk hash table for template specialization lookup, at least for templates with large numbers of specializations.)



================
Comment at: include/clang/AST/DeclTemplate.h:762-765
+    bool operator< (const LazySpecializationInfo &Other) const {
+      return DeclID < Other.DeclID;
+    }
+    bool operator== (const LazySpecializationInfo &Other) const {
----------------
No space between operator and `(`, please.


================
Comment at: lib/AST/DeclTemplate.cpp:209
+      if (Specs[I+1].ODRHash == Hash)
+        (void)loadLazySpecializationImpl(Specs[I+1].DeclID);
+   }
----------------
Can we remove the loaded specializations from the list / zero them out, so know we've already asked the ExternalASTSource for them?


================
Comment at: lib/AST/DeclTemplate.cpp:235-242
 #ifndef NDEBUG
     void *CorrectInsertPos;
     assert(!findSpecializationImpl(Specializations,
                                    SETraits::getTemplateArgs(Entry),
                                    CorrectInsertPos) &&
            InsertPos == CorrectInsertPos &&
            "given incorrect InsertPos for specialization");
----------------
I'm a little concerned about this: a debug build could perform more deserialization than an optimized build here. It's probably not a big deal, but it could lead to some surprising debugging experiences down the line (where a debug build works but an optimized build misbehaves).

Perhaps we could assert that there are no lazy specializations with the relevant hash in our list before performing this lookup? (This'd require that we remove them as we load them.)


================
Comment at: lib/Serialization/ASTReader.cpp:7009-7010
+    Args = CTSD->getTemplateArgs().asArray();
   }
+  else if (auto *VTSD = dyn_cast<VarTemplateSpecializationDecl>(D)) {
+    Template = VTSD->getSpecializedTemplate();
----------------
`}` and `else` on same line, please.


https://reviews.llvm.org/D41416