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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
