Skip to content

Commit

Permalink
fix: correct cost attribution of inline frames in disassembly view
Browse files Browse the repository at this point in the history
Previously, we put the cost of an inline frame into a container
indexed by the inline symbol. But during disassembly, we never get
to query that data again, since we cannot disassemble an inline frame.
Instead, we need to be able to query the cost for arbitrary binary
offsets, independent of their originating symbol.

The patch here achieves this by lifting the OffsetLocationCostMap out
of the CallerCalleeEntryMap into CallerCalleeResults, but mapped by
the binary name. This way we can efficiently store and lookup the data
of a given offset within a specific binary during disassembly, which
allows us to show the cost for inlined code in the disassembly view.

Fixes: #671
  • Loading branch information
milianw committed Sep 5, 2024
1 parent f326ef0 commit 36bedef
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 22 deletions.
33 changes: 19 additions & 14 deletions src/models/data.h
Original file line number Diff line number Diff line change
Expand Up @@ -683,18 +683,6 @@ struct CallerCalleeEntry
return *it;
}

LocationCost& offset(quint64 addr, int numTypes)
{
auto it = offsetMap.find(addr);
if (it == offsetMap.end()) {
it = offsetMap.insert(addr, {numTypes});
} else if (it->inclusiveCost.size() < static_cast<size_t>(numTypes)) {
it->inclusiveCost.resize(numTypes);
it->selfCost.resize(numTypes);
}
return *it;
}

ItemCost& callee(const Symbol& symbol, int numTypes)
{
auto it = callees.find(symbol);
Expand All @@ -719,14 +707,14 @@ struct CallerCalleeEntry
CalleeMap callees;
// source map for this symbol, i.e. locations mapped to associated costs
SourceLocationCostMap sourceMap;
// per-IP map for this symbol for disassembly
OffsetLocationCostMap offsetMap;
};

using CallerCalleeEntryMap = QHash<Symbol, CallerCalleeEntry>;
struct CallerCalleeResults
{
CallerCalleeEntryMap entries;
// per-binary map of per-IP (relAddr) map for disassembly
QHash<QString, OffsetLocationCostMap> binaryOffsetMap;
Costs selfCosts;
Costs inclusiveCosts;

Expand All @@ -739,6 +727,23 @@ struct CallerCalleeResults
}
return *it;
}

LocationCost& binaryOffset(const QString& binary, quint64 addr, int numTypes)
{
auto binaryIt = binaryOffsetMap.find(binary);
if (binaryIt == binaryOffsetMap.end()) {
binaryIt = binaryOffsetMap.insert(binary, {});
}

auto it = binaryIt->find(addr);
if (it == binaryIt->end()) {
it = binaryIt->insert(addr, {numTypes});
} else if (it->inclusiveCost.size() < static_cast<size_t>(numTypes)) {
it->inclusiveCost.resize(numTypes);
it->selfCost.resize(numTypes);
}
return *it;
}
};

void callerCalleesFromBottomUpData(const BottomUpResults& data, CallerCalleeResults* results);
Expand Down
11 changes: 6 additions & 5 deletions src/models/disassemblymodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ void DisassemblyModel::clear()
{
beginResetModel();
m_data = {};
m_offsetMap = {};
endResetModel();
}

Expand All @@ -47,6 +48,7 @@ void DisassemblyModel::setDisassembly(const DisassemblyOutput& disassemblyOutput

m_data = disassemblyOutput;
m_results = results;
m_offsetMap = m_results.binaryOffsetMap[m_data.symbol.binary];
m_numTypes = results.selfCosts.numTypes();

QStringList assemblyLines;
Expand Down Expand Up @@ -128,9 +130,8 @@ QVariant DisassemblyModel::data(const QModelIndex& index, int role) const
return {};
}

const auto entry = m_results.entries.value(m_data.symbol);
auto it = entry.offsetMap.find(data.addr);
if (it != entry.offsetMap.end()) {
auto it = m_offsetMap.find(data.addr);
if (it != m_offsetMap.end()) {
const auto event = index.column() - COLUMN_COUNT;
const auto& locationCost = it.value();

Expand Down Expand Up @@ -223,8 +224,8 @@ QModelIndex DisassemblyModel::indexForFileLine(const Data::FileLine& fileLine) c
bestMatch = i;
}

auto it = entry.offsetMap.find(line.addr);
if (it != entry.offsetMap.end()) {
auto it = m_offsetMap.find(line.addr);
if (it != m_offsetMap.end()) {
const auto& locationCost = it.value();

if (!bestCost || bestCost < locationCost.selfCost[0]) {
Expand Down
1 change: 1 addition & 0 deletions src/models/disassemblymodel.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ public slots:
HighlightedText m_highlightedText;
DisassemblyOutput m_data;
Data::CallerCalleeResults m_results;
Data::OffsetLocationCostMap m_offsetMap;
int m_numTypes = 0;
int m_highlightLine = 0;
};
3 changes: 2 additions & 1 deletion src/parsers/perf/perfparser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,8 @@ void addCallerCalleeEvent(const Data::Symbol& symbol, const Data::Location& loca
auto& entry = callerCalleeResult->entry(symbol);
auto& sourceCost = entry.source(location.fileLine, numCosts);
// relAddr can be 0 for symbols in the main executable
auto& addrCost = entry.offset(location.relAddr ? location.relAddr : location.address, numCosts);
auto& addrCost = callerCalleeResult->binaryOffset(
symbol.binary, location.relAddr ? location.relAddr : location.address, numCosts);

sourceCost.inclusiveCost[type] += cost;
addrCost.inclusiveCost[type] += cost;
Expand Down
3 changes: 1 addition & 2 deletions tests/modeltests/tst_models.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,7 @@ private slots:
Data::CallerCalleeResults results;
Data::callerCalleesFromBottomUpData(tree, &results);

auto entry = results.entry(symbol);
auto& locationCost = entry.offset(4294563, results.selfCosts.numTypes());
auto& locationCost = results.binaryOffset(symbol.binary, 4294563, results.selfCosts.numTypes());
locationCost.inclusiveCost[0] += 200;
locationCost.selfCost[0] += 200;

Expand Down

0 comments on commit 36bedef

Please sign in to comment.