From c3e1682a6ea49770107249836b5baf6ca47ede25 Mon Sep 17 00:00:00 2001 From: Milian Wolff Date: Mon, 2 Sep 2024 09:39:39 +0200 Subject: [PATCH] fix: correct cost attribution of inline frames in disassembly view 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: https://github.com/KDAB/hotspot/issues/671 --- src/models/data.h | 33 +++++++++++++++++++-------------- src/models/disassemblymodel.cpp | 11 ++++++----- src/models/disassemblymodel.h | 1 + src/parsers/perf/perfparser.cpp | 3 ++- tests/modeltests/tst_models.cpp | 3 +-- 5 files changed, 29 insertions(+), 22 deletions(-) diff --git a/src/models/data.h b/src/models/data.h index dd905442..0bf8778b 100644 --- a/src/models/data.h +++ b/src/models/data.h @@ -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(numTypes)) { - it->inclusiveCost.resize(numTypes); - it->selfCost.resize(numTypes); - } - return *it; - } - ItemCost& callee(const Symbol& symbol, int numTypes) { auto it = callees.find(symbol); @@ -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; struct CallerCalleeResults { CallerCalleeEntryMap entries; + // per-binary map of per-IP (relAddr) map for disassembly + QHash binaryOffsetMap; Costs selfCosts; Costs inclusiveCosts; @@ -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(numTypes)) { + it->inclusiveCost.resize(numTypes); + it->selfCost.resize(numTypes); + } + return *it; + } }; void callerCalleesFromBottomUpData(const BottomUpResults& data, CallerCalleeResults* results); diff --git a/src/models/disassemblymodel.cpp b/src/models/disassemblymodel.cpp index a1f458d1..fdf209dd 100644 --- a/src/models/disassemblymodel.cpp +++ b/src/models/disassemblymodel.cpp @@ -23,6 +23,7 @@ void DisassemblyModel::clear() { beginResetModel(); m_data = {}; + m_offsetMap = {}; endResetModel(); } @@ -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; @@ -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(); @@ -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]) { diff --git a/src/models/disassemblymodel.h b/src/models/disassemblymodel.h index 2cba0ad9..3401e05d 100644 --- a/src/models/disassemblymodel.h +++ b/src/models/disassemblymodel.h @@ -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; }; diff --git a/src/parsers/perf/perfparser.cpp b/src/parsers/perf/perfparser.cpp index 6ef07734..a180b832 100644 --- a/src/parsers/perf/perfparser.cpp +++ b/src/parsers/perf/perfparser.cpp @@ -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; diff --git a/tests/modeltests/tst_models.cpp b/tests/modeltests/tst_models.cpp index e8ad3960..c0dbefa7 100644 --- a/tests/modeltests/tst_models.cpp +++ b/tests/modeltests/tst_models.cpp @@ -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;