[AST] Match the FoldingSetNodeID computed before and after creating TypedefType

[ilya-biryukov]

At some point the call to static TypedefType::Profile inside ASTContext::getTypedefType got out-of-sync with the non-static TypedefType::Profile.

This seem to cause some bad performance patterns with FoldingSet and are seeing 10x increases in compile times in certain scenarios. After this commit, the compile times go back to normal.

This commit does not include tests or benchmarks because we want to land this ASAP to unbreak our deployment. I will work on adding asserts, tests or benchmarks in a follow-up.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Ilya Biryukov (ilya-biryukov)

Changes

At some point the call to static TypedefType::Profile inside ASTContext::getTypedefType got out-of-sync with the non-static TypedefType::Profile.

This seem to cause some bad performance patterns with FoldingSet and are seeing 10x increases in compile times in certain scenarios. After this commit, the compile times go back to normal.

This commit does not include tests or benchmarks because we want to land this ASAP to unbreak our deployment. I will work on adding asserts, tests or benchmarks in a follow-up.

---

Full diff: https://github.com/llvm/llvm-project/pull/157662.diff

1 Files Affected:

• (modified) clang/lib/AST/ASTContext.cpp (+2-1)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index dca05b41aee77..c04de4e132739 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -5316,7 +5316,8 @@ ASTContext::getTypedefType(ElaboratedTypeKeyword Keyword,
   }
 
   llvm::FoldingSetNodeID ID;
-  TypedefType::Profile(ID, Keyword, Qualifier, Decl, UnderlyingType);
+  TypedefType::Profile(ID, Keyword, Qualifier, Decl,
+                       *TypeMatchesDeclOrNone ? QualType() : UnderlyingType);
 
   void *InsertPos = nullptr;
   if (FoldingSetPlaceholder<TypedefType> *Placeholder =

[ilya-biryukov]

cc @mizvekov as I believe this is probably related to the new representation of NameQualifier.

[alexfh]

Thank you for the fix!

[kadircet]

ugh this looks quite fragile (not your fix but rather having multiple ways to call profile), i am worried we might have divergences in other places as well :(

i think it would be nice to handle that in a more principled way for the future. thanks for the fix!

[ilya-biryukov]

ugh this looks quite fragile (not your fix but rather having multiple ways to call profile), i am worried we might have divergences in other places as well :(

i think it would be nice to handle that in a more principled way for the future. thanks for the fix!

I am definitely worried as well and was trying to figure out a generic way to assert the two FoldingSetNodeIDs are the same. I didn't find one, but we could at least assert the IDs of the node after we create each type is the same as the one we looked up in the FoldingSet before we added it.

I'll prepare a PR to propose that so we can discuss this further.

[ilya-biryukov]

Sent out a draft PR with an assert that seems to catch more cases like this: #157692, let's discuss further there.