sammccall added inline comments.
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:77
+ bool Satisfied = false;
+ for (const Header &H : Providers) {
+ if (H.kind() == Header::Physical && H.physical() == MainFile)
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > nit: maybe leave a comment here for skipping `Header`s we've seen before.
> > > there's a quite good chance that we'll see same provider showing up
> > > multiple times, skipping processing might be helpful (later on we'll
> > > probably need to cache the analysis results for diagnostics purposes).
> > I think you're talking about a performance optimization using a cache?
> >
> > We still need to process each header to compute `Satisfied`. So at most
> > we're replacing a trivial comparison + 2 hashtable lookups (`Inc.match` and
> > `Used.insert`) with one cache lookup. (The inner loop count is ~always <=1).
> >
> > Happy with such a change if it improves a benchmark of course but my
> > expectation is that it *wouldn't* (small constant factor on a fairly fast
> > operation) so a FIXME seems premature here.
> > We still need to process each header to compute Satisfied
>
> but we need to do this only once per unique `Providers` across all the
> callbacks (e.g. we'll get `Foo.h` as a provider for every reference of `Foo`
> and `createFoo` in the main file). From header analysis point of view, i
> don't think we need to treat each of these Providers specially based on the
> nature of the reference (neither the symbol nor the reference location/type).
>
> > So at most we're replacing a trivial comparison + 2 hashtable lookups
> > (Inc.match and Used.insert) with one cache lookup. (The inner loop count is
> > ~always <=1).
>
> Yeah I guess you're right. I was treating the inner-loop as going over all
> the includes in the main file, but in practice it's just a bunch of lookups
> into a hashtable and operating over a single include.
>
> > Happy with such a change if it improves a benchmark of course but my
> > expectation is that it *wouldn't* (small constant factor on a fairly fast
> > operation) so a FIXME seems premature here.
>
> no need. agreed that it is unlikely to make a difference. and if it would,
> it's probably better to write this in a way that'll de-duplicate Providers
> across references with some bucketing over reftypes and process all of them
> once.
> but we need to do this only once per unique Providers across all the
> callbacks (e.g. we'll get Foo.h as a provider for every reference of Foo and
> createFoo in the main file)
By "process" I mean we need to at least look up into the cache.
That's pretty trivial, but we're not doing much more at the moment.
> but in practice it's just a bunch of lookups into a hashtable
in fact just one lookup!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139013/new/
https://reviews.llvm.org/D139013
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
