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

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:8188-8190
+  for (auto *L = &List; L; L = L->getNext()) {
+    seen.insert(L->getMethod());
+  }
----------------
dexonsmith wrote:
> 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?
Yes, I take your point, if the method pool generation updates inbetween each of the later modules then it is possible to hit this case.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:8194
+    if (seen.insert(M).second) {
+      S.addMethodToGlobalList(&List, M);
+    }
----------------
dexonsmith wrote:
> 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.
I'll update with this approach, it should allow for moving the set insert logic into `addMethodToGlobalList` in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109632