[PATCH] D109632: [clang] de-duplicate methods from AST files

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a reviewer: vsapsai.
dexonsmith added a subscriber: vsapsai.
dexonsmith added a comment.

Thanks for looking at this!

I have a couple of comments inline. @vsapsai, can you also take a look?



================
Comment at: clang/lib/Serialization/ASTReader.cpp:8188-8190
+  for (auto *L = &List; L; L = L->getNext()) {
+    seen.insert(L->getMethod());
+  }
----------------
I find quadratic algorithms a bit scary, even when current benchmarks don't expose problems. For example, it seems like this could blow up on the following workload:
- one module adds many things to the global method list
- there are many (fine-grained) modules that transitively load that module

Or have I misunderstood the complexity here?


================
Comment at: clang/lib/Serialization/ASTReader.cpp:8194
+    if (seen.insert(M).second) {
+      S.addMethodToGlobalList(&List, M);
+    }
----------------
rmaz wrote:
> manmanren wrote:
> > Does it make sense to check for duplication inside addMethodToGlobalList, as the function goes through the list as well? Maybe it is slower, as we will need to go through the list for each method, instead of a lookup.
> Yes, you are right, it is slower as we need to do a list traverse per insert rather than per selector lookup. I also profiled keeping some global state along with the `MethodPool` so that the set didn't have to be rebuilt each time, but the performance difference was negligible.
Can you take another look at the approach you experimented with, putting the global state in the MethodPool? Besides the performance difference (you measured it as negligible but it'd avoid the concern I have about uncommon workloads hitting quadratic blowups), it'd also provide consistency in behaviour between callers of `addMethodToGlobalList`... and enable you to add a unit test for the API change to MethodPool.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632