Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Coverage] Improve performance of propagating Counter of Expansions #122589

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 74 additions & 16 deletions llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/TargetParser/Triple.h"
#include <limits>
#include <vector>

using namespace llvm;
Expand Down Expand Up @@ -440,26 +441,83 @@ Error RawCoverageMappingReader::read() {
// Set the counters for the expansion regions.
// i.e. Counter of expansion region = counter of the first region
// from the expanded file.
// Perform multiple passes to correctly propagate the counters through
// all the nested expansion regions.
SmallVector<CounterMappingRegion *, 8> FileIDExpansionRegionMapping;
FileIDExpansionRegionMapping.resize(VirtualFileMapping.size(), nullptr);
for (unsigned Pass = 1, S = VirtualFileMapping.size(); Pass < S; ++Pass) {
for (auto &R : MappingRegions) {
if (R.Kind != CounterMappingRegion::ExpansionRegion)
continue;
assert(!FileIDExpansionRegionMapping[R.ExpandedFileID]);
FileIDExpansionRegionMapping[R.ExpandedFileID] = &R;
// This assumes:
// - Records are partitioned by FileID.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. no record is a part of two different files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each FIleID should have at least one record, since FileID is defied by Record.
Actually the Record is sorted by FileID in CoverageMappingWriter.

// - The Count in Expansion is propagated from 1st Record in
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.

nth record's count == n-1th count + n-2th count ... 0th count?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my less descriptive explanation.

"1st (the top) Record in the cluster that is defined by FileID, that is referred with ExpandedFileID".

// ExpandedFileID.
// Therefore another fact below can be assumed.
// - Propagation chain consists of Expansions at each 1st Record.

/// Node per File.
struct FileNode {
/// 1st Record in the File.
CounterMappingRegion *FirstR = nullptr;
/// The Expansion Record out of this File.
CounterMappingRegion *ExpanderR = nullptr;
/// Count hasn't been propagated yet (for Expansion)
bool ExpansionPending = false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool ExpansionPending = false;
bool ExpansionCountPropagated = false;

Maybe more descriptive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like leave it "true as marked". Or ExpansionCountNotPropagated?

};
SmallVector<FileNode> FileIDExpansionRegionMapping(VirtualFileMapping.size());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SmallVector<FileNode> FileIDExpansionRegionMapping(VirtualFileMapping.size());
// We will construct a tree with one node per file in the virtual file mapping.
SmallVector<FileNode> FileIDExpansionRegionMapping(VirtualFileMapping.size());

Add a comment explaining what we're going to do with this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also what will the edges in the tree be? That's not clear to me right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I rename the name shorter and add more descriptions instead of the object?
Longer symbols are bad (IMO).


// Construct the tree.
auto PrevFileID = std::numeric_limits<unsigned>::max(); // Invalid value
for (auto &R : MappingRegions) {
if (R.Kind == CounterMappingRegion::ExpansionRegion) {
// The File that contains Expansion may be a node.
assert(!FileIDExpansionRegionMapping[R.ExpandedFileID].ExpanderR);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this checking that there is only one expansion region?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Expander" means "Expansion Region in other nodes". ExpandedFileID is unique in the current impl.

FileIDExpansionRegionMapping[R.ExpandedFileID].ExpanderR = &R;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be slightly cleaner to pull this out into a reference variable?

E.g.

FileNode &N = FileIDExpansionRegionMapping[R.ExpandedFileID];
assert(!N.ExpanderR && "Already found an expansion region for file node?!");
N.ExpanderR = &R;
N.ExpansionPending = true;


// It will be the node if (isExpansion and ExpansionPending).
FileIDExpansionRegionMapping[R.ExpandedFileID].ExpansionPending = true;
}
for (auto &R : MappingRegions) {
if (FileIDExpansionRegionMapping[R.FileID]) {
FileIDExpansionRegionMapping[R.FileID]->Count = R.Count;
FileIDExpansionRegionMapping[R.FileID] = nullptr;
}

if (PrevFileID != R.FileID) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can reduce indentation here.

// Have we found the first record for this file already?
if (PrevFileID == R.FileID) {
  // We've seen this FileID before, so we have found the
  // first record. Move on to the next region.
  continue;
}
// We have not. Mark the current record as the first record
// for this file.
assert(!FileIDExpansionRegionMapping[R.FileID].FirstR && "Already found a record for an ID we haven't seen before?!");
FileIDExpansionRegionMapping[R.FileID].FirstR = &R;
// Remember that we've processed this file already.
PrevFileID = R.FileID;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me think restructuring this.

// Record 1st Record for each File.
assert(!FileIDExpansionRegionMapping[R.FileID].FirstR);
FileIDExpansionRegionMapping[R.FileID].FirstR = &R;
PrevFileID = R.FileID;
}
}

return Error::success();
auto Propagate = [&](FileNode &X) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a lambda and not a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not a lambda? I prefer using a lambda as definition of subfunction that shares the context.

I could abandon serpentine seek just to get rid of the lambda. I don't want to introduce another method here.

// Skip already processed node.
if (!X.ExpanderR)
return false;
// Skip Pending Expansion node.
// Process non-Expansion Record and non-Pending Expansion.
if (X.ExpansionPending &&
X.FirstR->Kind == CounterMappingRegion::ExpansionRegion)
return false;

// Propagate.
X.ExpanderR->Count = X.FirstR->Count;
// Mark the destination ready.
FileIDExpansionRegionMapping[X.ExpanderR->FileID].ExpansionPending = false;
// Mark this processed.
X.ExpanderR = nullptr;
return true;
};

// This won't iterate in most cases but iterates finitely for
// unusual cases.
for (unsigned I = 0, E = FileIDExpansionRegionMapping.size(); I != E; ++I) {
// Propagate Descending. All Files will be processed if each
// ExpandedFileID is greater than FileID.
for (auto &X : reverse(FileIDExpansionRegionMapping))
Propagate(X);

// Check whether all Files are processed (or propagate Ascending,
// not possible in the usual assumptions).
bool Changed = false;
for (auto &X : FileIDExpansionRegionMapping)
Changed = (Propagate(X) || Changed);

if (!Changed)
return Error::success();
}

return make_error<CoverageMapError>(coveragemap_error::malformed,
"unexpected Expansions");
}

Expected<bool> RawCoverageMappingDummyChecker::isDummy() {
Expand Down
Loading