NagyDonat wrote: > The bug was that we inserted the `T` into the expected `InsertPos` > corresponding to the `Profile` of `T` - so far so good, but instead of > returning that, we checked if this is "overly complicated", and if so, we > created the new `SymbolOverlyComplex` symbol and returned that instead. > However, when the next time we get to call `acquired<T>`, we would simply get > a successful lookup for the `Profile` of `T` and return the symbol we > actually wanted to hide! instead of returning the `SymbolOverlyComplex` that > we returned in the previous `acquire<T>` call. [...]
Ugh, the stateful nature of these hashtable data structures is evil... > Otherwise, we need to look up the wrapped symbol first, then check if we have > a `SymbolOverlyComplex` with that wrapped symbol in the map. If there is, > just return that one, otherwise we create it. I have to admit that I don't understand the role and meaning of "the wrapped symbol" in your code. However, I think I see another, less complex approach for "fixing" the method `acquire`: instead of creating the `Dummy` symbol and calculating its complexity early, just modify the original state of the method (i.e. the parent revision of https://github.com/llvm/llvm-project/pull/144327/commits/8c29bbc6c309efe0a1d6af948f95dbb9654971b8 ) by moving the order block ``` if (SD->complexity() > MaxCompComplexity) { return cast<Ret>(acquire<SymbolOverlyComplex>(SD)); } ``` out of the `if (!SD)` block (placing it directly after the `if (!SD)` block). This would guarantee that: - we always return `SymbolOverlyComplex` if the expression is too complex (even if there is a cache hit); - but even these overly complex symbols (which only live "inside" the `SymbolOverlyComplex` block) enjoy the benefit of caching and aren't recreated repeatedly. I hope that this alternative approach could prevent the performance hit that you observed. https://github.com/llvm/llvm-project/pull/144327 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
