[PATCH] Coverage: improve efficiency of the counter propagation to the expansion regions.

[Alex L]

2014-07-29 12:37 GMT-07:00 Justin Bogner <mail at justinbogner.com>:

> Alex Lorenz <arphaman at gmail.com> writes:
> > This patch reduces the complexity of the two inner loops in order to
> > speed up the loading of coverage data for very large functions
> > (>100kloc, >15k regions, >5k files).
> >
> > http://reviews.llvm.org/D4708
> >
> > Files:
> >   lib/ProfileData/CoverageMappingReader.cpp
> >
> > Index: lib/ProfileData/CoverageMappingReader.cpp
> > ===================================================================
> > --- lib/ProfileData/CoverageMappingReader.cpp
> > +++ lib/ProfileData/CoverageMappingReader.cpp
> > @@ -251,15 +251,20 @@
> >    // from the expanded file.
> >    // Perform multiple passes to correctly propagate the counters through
> >    // all the nested expansion regions.
> > +  SmallVector<CounterMappingRegion *, 8> ExpansionRegions;
>
> How often are there 8 or less expansion regions? If we're talking about
> more than 15k regions I don't see this vector staying small very often.
>

The vector grows only to the number of file ids, and it's not often that a
lot
of file ids are used in a single function. Unfortunately the name of this
variable doesn't reflect that, I should rename it to
'FileIDExpansionRegionMapping'.


>
> >    for (unsigned Pass = 1, S = VirtualFileMapping.size(); Pass < S;
> ++Pass) {
> > -    for (auto &I : MappingRegions) {
> > -      if (I.Kind == CounterMappingRegion::ExpansionRegion) {
> > -        for (const auto &J : MappingRegions) {
> > -          if (J.FileID == I.ExpandedFileID) {
> > -            I.Count = J.Count;
> > -            break;
> > -          }
> > -        }
> > +    ExpansionRegions.clear();
> > +    ExpansionRegions.resize(VirtualFileMapping.size(), nullptr);
>
> No point in doing this resize every time through the loop - the size
> doesn't change.
>
> I also don't find it obvious that you're using resize for the side
> effect of zero filling the vector here. That probably deserves a
> comment at least.
>

Looking at it again I think that zero filling every pass is not
necessary as the non-null ExpansionRegions get nulled anyway
in the second loop.


>
> > +    for (auto &R : MappingRegions) {
> > +      if (R.Kind != CounterMappingRegion::ExpansionRegion)
> > +        continue;
> > +      assert(!ExpansionRegions[R.ExpandedFileID]);
> > +      ExpansionRegions[R.ExpandedFileID] = &R;
> > +    }
> > +    for (auto &R : MappingRegions) {
> > +      if (ExpansionRegions[R.FileID]) {
> > +        ExpansionRegions[R.FileID]->Count = R.Count;
> > +        ExpansionRegions[R.FileID] = nullptr;
>
> I don't think this is quite right. You don't null out th
> ExpansionRegions that weren't used. Is that intentional?
>

The expansion regions that weren't used here are already null
though, as the if statement checks.


>
> Overall, I suspect there's a better algorithm for propagating these
> counters. Have you thought about other approaches?
>

There might be a better one, but it would probably still boil down
to looping over the regions for a certain number of passes.
An improvement to this algorithm would try to find the actual number
of passes based on the deepest expansion region tree instead of
relying on the number of file ids in a function, but I think that for small
functions it would be an overkill, and for large functions a simple limit
of like 100 or something passes might be ok.


>
> >        }
> >      }
> >    }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140729/d751cb78/attachment.html>