[PATCH] Coverage: improve efficiency of the counter propagation to the expansion regions.
Justin Bogner
mail at justinbogner.com
Tue Jul 29 12:37:08 PDT 2014
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.
> 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.
> + 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?
Overall, I suspect there's a better algorithm for propagating these
counters. Have you thought about other approaches?
> }
> }
> }
More information about the llvm-commits
mailing list