<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-29 12:37 GMT-07:00 Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">Alex Lorenz <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> writes:<br>
> This patch reduces the complexity of the two inner loops in order to<br>
> speed up the loading of coverage data for very large functions<br>
> (>100kloc, >15k regions, >5k files).<br>
><br>
> <a href="http://reviews.llvm.org/D4708" target="_blank">http://reviews.llvm.org/D4708</a><br>
><br>
> Files:<br>
>   lib/ProfileData/CoverageMappingReader.cpp<br>
><br>
> Index: lib/ProfileData/CoverageMappingReader.cpp<br>
> ===================================================================<br>
> --- lib/ProfileData/CoverageMappingReader.cpp<br>
> +++ lib/ProfileData/CoverageMappingReader.cpp<br>
> @@ -251,15 +251,20 @@<br>
>    // from the expanded file.<br>
>    // Perform multiple passes to correctly propagate the counters through<br>
>    // all the nested expansion regions.<br>
> +  SmallVector<CounterMappingRegion *, 8> ExpansionRegions;<br>
<br>
</div>How often are there 8 or less expansion regions? If we're talking about<br>
more than 15k regions I don't see this vector staying small very often.<br></blockquote><div><br></div><div>The vector grows only to the number of file ids, and it's not often that a lot<br>of file ids are used in a single function. Unfortunately the name of this<br>
variable doesn't reflect that, I should rename it to 'FileIDExpansionRegionMapping'.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
>    for (unsigned Pass = 1, S = VirtualFileMapping.size(); Pass < S; ++Pass) {<br>
> -    for (auto &I : MappingRegions) {<br>
> -      if (I.Kind == CounterMappingRegion::ExpansionRegion) {<br>
> -        for (const auto &J : MappingRegions) {<br>
> -          if (J.FileID == I.ExpandedFileID) {<br>
> -            I.Count = J.Count;<br>
> -            break;<br>
> -          }<br>
> -        }<br>
> +    ExpansionRegions.clear();<br>
> +    ExpansionRegions.resize(VirtualFileMapping.size(), nullptr);<br>
<br>
</div>No point in doing this resize every time through the loop - the size<br>
doesn't change.<br>
<br>
I also don't find it obvious that you're using resize for the side<br>
effect of zero filling the vector here. That probably deserves a<br>
comment at least. <br></blockquote><div><br></div><div>Looking at it again I think that zero filling every pass is not<br></div><div>necessary as the non-null ExpansionRegions get nulled anyway<br></div><div>in the second loop.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
> +    for (auto &R : MappingRegions) {<br>
> +      if (R.Kind != CounterMappingRegion::ExpansionRegion)<br>
> +        continue;<br>
> +      assert(!ExpansionRegions[R.ExpandedFileID]);<br>
> +      ExpansionRegions[R.ExpandedFileID] = &R;<br>
> +    }<br>
> +    for (auto &R : MappingRegions) {<br>
> +      if (ExpansionRegions[R.FileID]) {<br>
> +        ExpansionRegions[R.FileID]->Count = R.Count;<br>
> +        ExpansionRegions[R.FileID] = nullptr;<br>
<br>
</div>I don't think this is quite right. You don't null out th<br>
ExpansionRegions that weren't used. Is that intentional?<br></blockquote><div><br></div><div>The expansion regions that weren't used here are already null<br></div><div>though, as the if statement checks.<br></div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Overall, I suspect there's a better algorithm for propagating these<br>
counters. Have you thought about other approaches?<br></blockquote><div><br></div><div>There might be a better one, but it would probably still boil down<br>to looping over the regions for a certain number of passes.<br>
</div><div>An improvement to this algorithm would try to find the actual number<br>of passes based on the deepest expansion region tree instead of<br>relying on the number of file ids in a function, but I think that for small<br>
functions it would be an overkill, and for large functions a simple limit<br></div><div>of like 100 or something passes might be ok.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
<br>
>        }<br>
>      }<br>
>    }<br>
</blockquote></div><br></div></div>