From 8eb4c8b9f0a6ab55ce72d63ea1189657ca634ea6 Mon Sep 17 00:00:00 2001 From: Andrey Kvochko Date: Thu, 26 Oct 2017 13:49:42 +0300 Subject: [PATCH] Fix after review --- profiler/profiler/src/profiler.cpp | 84 +++++++++++------------------------ profiler/profiler/src/stackentry.h | 2 +- src/analyze/accumulatedtracedata.cpp | 9 ++-- src/analyze/accumulatedtracedata.h | 8 +--- src/analyze/gui/mainwindow.cpp | 2 +- src/analyze/gui/mainwindow.ui | 24 ++++++---- src/analyze/gui/objecttreemodel.cpp | 19 ++++++++ src/analyze/gui/parser.cpp | 4 +- src/interpret/heaptrack_interpret.cpp | 66 +++++++++++++-------------- src/track/libheaptrack.cpp | 26 ++++++++--- src/util/pointermap.h | 1 + 11 files changed, 123 insertions(+), 122 deletions(-) diff --git a/profiler/profiler/src/profiler.cpp b/profiler/profiler/src/profiler.cpp index 0d53599..ca8e03d 100644 --- a/profiler/profiler/src/profiler.cpp +++ b/profiler/profiler/src/profiler.cpp @@ -271,38 +271,6 @@ static HRESULT GetClassNameFromClassId(ICorProfilerInfo *info, ClassID classId, return S_OK; } -static HRESULT GetClassSizeFromClassId(ICorProfilerInfo2 *info, ClassID classId, ULONG *pulClassSize) { - ModuleID moduleId; - mdTypeDef mdClass; - ClassID parentClassId; - - HRESULT hr = info->GetClassIDInfo2(classId, &moduleId, &mdClass, &parentClassId, 0, nullptr, nullptr); - if (parentClassId) { - ULONG parentClassSize = 0; - hr = GetClassSizeFromClassId(info, parentClassId, &parentClassSize); - *pulClassSize += parentClassSize; - } - - if (hr != S_OK) - return hr; - - IMetaDataImport * pIMetaDataImport; - hr = info->GetModuleMetaData(moduleId, CorOpenFlags::ofRead, IID_IMetaDataImport, - (LPUNKNOWN *)&pIMetaDataImport); - if (hr != S_OK) - return hr; - - ULONG classSize = 0; - hr = pIMetaDataImport->GetClassLayout(mdClass, nullptr, nullptr, 0, nullptr, &classSize); - *pulClassSize += classSize; - - if (hr != S_OK) - return hr; - - pIMetaDataImport->Release(); - return S_OK; -} - void encodeWChar(WCHAR *orig, char *encoded) { int i = 0; while (orig[i] != 0) { @@ -445,32 +413,34 @@ HRESULT STDMETHODCALLTYPE Profiler::ClassLoadStarted(ClassID classId) { HRESULT STDMETHODCALLTYPE Profiler::ClassLoadFinished(ClassID classId, HRESULT hrStatus) { - if (hrStatus != S_OK) + if (hrStatus != S_OK) { + fprintf(stderr, "[W] Failed to load class %x, HRESULT=%x\n", classId, hrStatus); return S_OK; + } + ICorProfilerInfo2 *info; - HRESULT hr = g_pICorProfilerInfoUnknown->QueryInterface(IID_ICorProfilerInfo2, - (void **)&info); - if (hr != S_OK) - return E_FAIL; + HRESULT hr = g_pICorProfilerInfoUnknown->QueryInterface(IID_ICorProfilerInfo2, (void **)&info); + if (hr != S_OK) { + assert(false && "Failed to retreive ICorProfilerInfo"); + } + ULONG classSize = 0; WCHAR wszClassName[MAX_NAME_LENGTH + 1]; - hr = GetClassNameFromClassId(info, classId, wszClassName); - if (hr != S_OK) - return E_FAIL; + hr = GetClassNameFromClassId(info, classId, wszClassName); + if (hr != S_OK) { + fprintf(stderr, "[W] Failed to retrieve class name for class %x, HRESULT=%x\n", classId, hr); + goto Cleanup; + } char className[MAX_NAME_LENGTH + 1]; encodeWChar(wszClassName, className); - uint32_t classSize = 0; - - GetClassSizeFromClassId(info, classId, &classSize); - if (hr != S_OK) - return E_FAIL; + heaptrack_loadclass(reinterpret_cast(classId), className); - heaptrack_loadclass(reinterpret_cast(classId), classSize, className); +Cleanup: info->Release(); - return S_OK; + return hr; } HRESULT STDMETHODCALLTYPE Profiler::ClassUnloadStarted(ClassID classId) { @@ -654,10 +624,10 @@ HRESULT STDMETHODCALLTYPE ULONG cObjectRefs, ObjectID objectRefIds[]) { HRESULT hr; ICorProfilerInfo *info; - hr = g_pICorProfilerInfoUnknown->QueryInterface(IID_ICorProfilerInfo, - (void **)&info); - if (hr != S_OK) - return hr; + hr = g_pICorProfilerInfoUnknown->QueryInterface(IID_ICorProfilerInfo, (void **)&info); + if (hr != S_OK) { + assert(false && "Failed to retreive ICorProfilerInfo"); + } for (int i = 0; i < cObjectRefs; ++i) { ClassID subjClass; @@ -665,7 +635,7 @@ HRESULT STDMETHODCALLTYPE // We still want the best estimate, even though something went wrong. if (hr != S_OK) { - fprintf(stderr, "Unknown type for object %zx", objectRefIds[i]); + fprintf(stderr, "[W] Unknown type for object %x, HRESULT=%x\n", objectRefIds[i], hr); continue; } @@ -679,11 +649,11 @@ HRESULT STDMETHODCALLTYPE Profiler::RootReferences(ULONG cRootRefs, ObjectID rootRefIds[]) { HRESULT hr; ICorProfilerInfo *info; - hr = g_pICorProfilerInfoUnknown->QueryInterface(IID_ICorProfilerInfo, - (void **)&info); + hr = g_pICorProfilerInfoUnknown->QueryInterface(IID_ICorProfilerInfo, (void **)&info); - if (hr != S_OK) - return hr; + if (hr != S_OK) { + assert(false && "Failed to retreive ICorProfilerInfo"); + } for (int i = 0; i < cRootRefs; ++i) { ClassID rootClass; @@ -693,7 +663,7 @@ HRESULT STDMETHODCALLTYPE if (hr != S_OK) { // Silently ignore null objects - we're not interested in these if (rootRefIds[0] != 0) - fprintf(stderr, "Unknown type for object %zx", rootRefIds[i]); + fprintf(stderr, "[W] Unknown type for object %x, HRESULT=%x\n", rootRefIds[i], hr); continue; } diff --git a/profiler/profiler/src/stackentry.h b/profiler/profiler/src/stackentry.h index 48bba03..9757c18 100644 --- a/profiler/profiler/src/stackentry.h +++ b/profiler/profiler/src/stackentry.h @@ -20,7 +20,7 @@ extern "C" void heaptrack_startgc(); extern "C" void heaptrack_gcmarksurvived(void *rangeStart, unsigned long rangeLength, void *rangeMovedTo); extern "C" void heaptrack_finishgc(); extern "C" void heaptrack_add_object_dep(void *keyObjectId, void *keyClassId, void *valObjectId, void *valClassId); -extern "C" void heaptrack_loadclass(void *classId, unsigned long classSize, char *className); +extern "C" void heaptrack_loadclass(void *classId, char *className); extern "C" void heaptrack_gcroot(void *objectId, void *rootClass); #endif // STACKENTRY_H diff --git a/src/analyze/accumulatedtracedata.cpp b/src/analyze/accumulatedtracedata.cpp index 28b3c46..6876339 100644 --- a/src/analyze/accumulatedtracedata.cpp +++ b/src/analyze/accumulatedtracedata.cpp @@ -68,7 +68,7 @@ AccumulatedTraceData::AccumulatedTraceData() allocations.reserve(16384); stopIndices.reserve(4); opNewIpIndices.reserve(16); - classInfos.reserve(4096); + classIndices.reserve(4096); objectTreeNodes.reserve(16384); } @@ -795,10 +795,9 @@ bool AccumulatedTraceData::read(istream& in, const ParsePass pass) if (pass != FirstPass) { continue; } - ClassInfo classInfo; - reader >> classInfo.classIndex; - reader >> classInfo.size; - classInfos.push_back(classInfo); + ClassIndex classIndex; + reader >> classIndex; + classIndices.push_back(classIndex); } else { cerr << "failed to parse line: " << reader.line() << endl; } diff --git a/src/analyze/accumulatedtracedata.h b/src/analyze/accumulatedtracedata.h index 88a7949..dd3a87e 100644 --- a/src/analyze/accumulatedtracedata.h +++ b/src/analyze/accumulatedtracedata.h @@ -87,12 +87,6 @@ struct Allocation : public AllocationData TraceIndex traceIndex; }; -struct ClassInfo -{ - ClassIndex classIndex; - uint64_t size = 0; -}; - struct ObjectTreeNode { uint64_t gcNum; @@ -368,7 +362,7 @@ struct AccumulatedTraceData std::vector strings; std::vector opNewIpIndices; std::vector allocationInfos; - std::vector classInfos; + std::vector classIndices; std::vector objectTreeNodes; AddressRangesMap addressRangeInfos; diff --git a/src/analyze/gui/mainwindow.cpp b/src/analyze/gui/mainwindow.cpp index 1b0aeec..b1a489e 100644 --- a/src/analyze/gui/mainwindow.cpp +++ b/src/analyze/gui/mainwindow.cpp @@ -626,7 +626,7 @@ void MainWindow::setupStacks() if (!current.isValid()) { stacksModel->clear(); } else { - auto proxy = qobject_cast(current.model()); + auto proxy = qobject_cast(current.model()); Q_ASSERT(proxy); auto leaf = proxy->mapToSource(current); stacksModel->fillFromIndex(leaf); diff --git a/src/analyze/gui/mainwindow.ui b/src/analyze/gui/mainwindow.ui index b16bc23..4f289c8 100644 --- a/src/analyze/gui/mainwindow.ui +++ b/src/analyze/gui/mainwindow.ui @@ -29,7 +29,10 @@ - + + KMessageWidget::Information + + .. @@ -59,32 +62,38 @@ - + <qt><p>This field specifies the primary heaptrack data file. These files are called <tt>heaptrack.$APP.$PID.gz</tt>. You can produce such a file by profiling your application, e.g. via:</p> <pre><code>heaptrack &lt;yourapplication&gt; ...</code></pre> <p>Or, alternatively, you can attach to a running process via</p> <pre><code>heaptrack --pid $(pidof &lt;yourapplication&gt;)</code></pre></qt> - + heaptrack.*.*.gz - + path/to/heaptrack.$APP.$PID.gz + + Qt::WindowModal + - + <qt>You can optionally specify a second heaptrack data file to compare to. If set, this file will be used as a base and its cost gets subtracted from the primary data costs.</qt> - + heaptrack.*.*.gz - + path/to/heaptrack.$APP.$PID.gz + + Qt::WindowModal + @@ -862,7 +871,6 @@ KMessageWidget QFrame
kmessagewidget.h
- 1 KUrlRequester diff --git a/src/analyze/gui/objecttreemodel.cpp b/src/analyze/gui/objecttreemodel.cpp index bb2feaa..cea03d1 100644 --- a/src/analyze/gui/objecttreemodel.cpp +++ b/src/analyze/gui/objecttreemodel.cpp @@ -137,6 +137,25 @@ QVariant ObjectTreeModel::data(const QModelIndex& index, int role) const QString tooltip; QTextStream stream(&tooltip); stream << "
";
+        switch (static_cast(index.column())) {
+            case ClassNameColumn:
+                stream << i18n("The name of the class.");
+                break;
+            case InstanceCountColumn:
+                stream << i18n("Total number of instances in this reference chain.");
+                break;
+            case GCNumColumn:
+                stream << i18n("GC number after which the snapshot has been taken.");
+                break;
+            case ShallowSizeColumn:
+                stream << i18n("Total size of objects of this type in a given reference chain.");
+                break;
+            case ReferencedSizeColumn:
+                stream << i18n("Total size of objects of this type referenced by other objects in a given reference chain.");
+                break;
+            case NUM_COLUMNS:
+                break;
+            }
         stream << "
"; return tooltip; } diff --git a/src/analyze/gui/parser.cpp b/src/analyze/gui/parser.cpp index e7fca4c..385fb09 100644 --- a/src/analyze/gui/parser.cpp +++ b/src/analyze/gui/parser.cpp @@ -794,8 +794,7 @@ std::unique_ptr buildObjectGraph(ParserData& data, size_t &nodeIndex uint64_t childPtr = data.objectTreeNodes[nodeIndex].objectPtr; if (ObjectNode::nodes.find(childPtr) == ObjectNode::nodes.end()) { - ObjectNode::nodes.insert(std::pair>( - childPtr, std::move(buildObjectGraph(data, nodeIndex)))); + ObjectNode::nodes.insert(std::pair>(childPtr, std::move(buildObjectGraph(data, nodeIndex)))); } else { Q_ASSERT(data.objectTreeNodes[nodeIndex].numChildren == 0 && "Incorrect number of children"); ++nodeIndex; @@ -808,7 +807,6 @@ std::unique_ptr buildObjectGraph(ParserData& data, size_t &nodeIndex ObjectTreeData buildObjectTree(ParserData& data) { - ObjectTreeData ret; size_t nodeIndex = 0; while (nodeIndex < data.objectTreeNodes.size()) { diff --git a/src/interpret/heaptrack_interpret.cpp b/src/interpret/heaptrack_interpret.cpp index bd38747..cb66c28 100644 --- a/src/interpret/heaptrack_interpret.cpp +++ b/src/interpret/heaptrack_interpret.cpp @@ -322,7 +322,7 @@ struct AccumulatedTraceData return ipId; } - size_t addClass(const uintptr_t classPointer, const size_t classSize) { + size_t addClass(const uintptr_t classPointer) { if (!classPointer) { return 0; } @@ -335,7 +335,7 @@ struct AccumulatedTraceData size_t classIndex = intern(m_managedNames[classPointer]); m_encounteredClasses.insert(it, make_pair(classPointer, classIndex)); - fprintf(stdout, "C %zx %zx\n", classIndex, classSize); + fprintf(stdout, "C %zx\n", classIndex); return classIndex; } @@ -471,7 +471,7 @@ int main(int /*argc*/, char** /*argv*/) uintptr_t addressStart = 0; if (!(reader >> addressStart)) { - cerr << "failed to parse line: " << reader.line() << endl; + cerr << "[C] failed to parse line: " << reader.line() << endl; return 1; } @@ -493,7 +493,7 @@ int main(int /*argc*/, char** /*argv*/) size_t parentIndex = 0; int is_managed; if (!(reader >> instructionPointer) || !(reader >> parentIndex) || !(reader >> is_managed)) { - cerr << "failed to parse line: " << reader.line() << endl; + cerr << "[C] failed to parse line: " << reader.line() << endl; return 1; } // ensure ip is encountered @@ -502,7 +502,7 @@ int main(int /*argc*/, char** /*argv*/) fprintf(stdout, "t %zx %zx\n", ipId, parentIndex); } else if (reader.mode() == '^') { if (isGCInProcess) { - cerr << "wrong trace format (allocation during GC - according to Book of the Runtime, concurrent GC is turned off in case profiling is enabled)" << endl; + cerr << "[W] wrong trace format (allocation during GC - according to Book of the Runtime, concurrent GC is turned off in case profiling is enabled)" << endl; continue; } @@ -512,7 +512,7 @@ int main(int /*argc*/, char** /*argv*/) TraceIndex traceId; uint64_t ptr = 0; if (!(reader >> traceId.index) || !(reader >> size) || !(reader >> ptr)) { - cerr << "failed to parse line: " << reader.line() << endl; + cerr << "[W] failed to parse line: " << reader.line() << endl; continue; } @@ -528,7 +528,7 @@ int main(int /*argc*/, char** /*argv*/) int isStart; if (!(reader >> isStart) || !(isStart == 1 || isStart == 0)) { - cerr << "failed to parse line: " << reader.line() << endl; + cerr << "[W] failed to parse line: " << reader.line() << endl; continue; } @@ -536,7 +536,7 @@ int main(int /*argc*/, char** /*argv*/) { // GC chunk start if (isGCInProcess) { - cerr << "wrong trace format (nested GC chunks)" << endl; + cerr << "[W] wrong trace format (nested GC chunks)" << endl; continue; } @@ -550,7 +550,7 @@ int main(int /*argc*/, char** /*argv*/) // GC chunk end if (!isGCInProcess) { - cerr << "wrong trace format (nested GC chunks?)" << endl; + cerr << "[W] wrong trace format (nested GC chunks?)" << endl; continue; } @@ -560,7 +560,7 @@ int main(int /*argc*/, char** /*argv*/) auto allocation = ptrToIndex.takePointer(managedPtr); if (!allocation.second) { - cerr << "wrong trace format (unknown managed pointer) 0x" << std::hex << managedPtr << std::dec << endl; + cerr << "[W] wrong trace format (unknown managed pointer) 0x" << std::hex << managedPtr << std::dec << endl; continue; } @@ -575,14 +575,14 @@ int main(int /*argc*/, char** /*argv*/) } } else if (reader.mode() == 'L') { if (!isGCInProcess) { - cerr << "wrong trace format (range survival event when no GC is running)" << endl; + cerr << "[W] wrong trace format (range survival event when no GC is running)" << endl; continue; } uint64_t rangeLength, rangeStart, rangeMovedTo; if (!(reader >> rangeLength) || !(reader >> rangeStart) || !(reader >> rangeMovedTo)) { - cerr << "failed to parse line: " << reader.line() << endl; + cerr << "[W] failed to parse line: " << reader.line() << endl; continue; } @@ -593,7 +593,7 @@ int main(int /*argc*/, char** /*argv*/) auto it = gcManagedPointersSet.lower_bound(targetRangeStart); if (it != gcManagedPointersSet.end() && *it < targetRangeEnd) { - cerr << "wrong trace format (survival ranges are intersecting during a GC session)" << endl; + cerr << "[W] wrong trace format (survival ranges are intersecting during a GC session)" << endl; continue; } } @@ -652,7 +652,7 @@ int main(int /*argc*/, char** /*argv*/) TraceIndex traceId; uint64_t ptr = 0; if (!(reader >> size) || !(reader >> traceId.index) || !(reader >> ptr)) { - cerr << "failed to parse line: " << reader.line() << endl; + cerr << "[W] failed to parse line: " << reader.line() << endl; continue; } @@ -666,7 +666,7 @@ int main(int /*argc*/, char** /*argv*/) } else if (reader.mode() == '-') { uint64_t ptr = 0; if (!(reader >> ptr)) { - cerr << "failed to parse line: " << reader.line() << endl; + cerr << "[W] failed to parse line: " << reader.line() << endl; continue; } bool temporary = lastPtr == ptr; @@ -682,21 +682,22 @@ int main(int /*argc*/, char** /*argv*/) --leakedAllocations; } else if (reader.mode() == 'n') { uint64_t ip; - string name; - if (!(reader >> ip) || !(reader >> name)) { - cerr << "failed to parse line: " << reader.line() << endl; + string methodOrClassName; + if (!(reader >> ip) || !(reader >> methodOrClassName)) { + cerr << "[W] failed to parse line: " << reader.line() << endl; + continue; } - if (managed_name_ids.find(name) == managed_name_ids.end()) { - managed_name_ids.insert(std::make_pair(name, 1)); + if (managed_name_ids.find(methodOrClassName) == managed_name_ids.end()) { + managed_name_ids.insert(std::make_pair(methodOrClassName, 1)); } else { - int id = ++managed_name_ids[name]; + int id = ++managed_name_ids[methodOrClassName]; - name.append("~"); - name.append(std::to_string(id)); + methodOrClassName.append("~"); + methodOrClassName.append(std::to_string(id)); } - data.addManagedNameForIP(ip, name); + data.addManagedNameForIP(ip, methodOrClassName); } else if (reader.mode() == 'e') { size_t gcCounter = 0; size_t numChildren = 0; @@ -704,30 +705,29 @@ int main(int /*argc*/, char** /*argv*/) uintptr_t classPointer = 0; if (!(reader >> gcCounter) || !(reader >> numChildren) || !(reader >> objectPointer) || !(reader >> classPointer)) { - cerr << "failed to parse line: " << reader.line() << endl; + cerr << "[W] failed to parse line: " << reader.line() << endl; continue; } // ensure class is encountered - const auto classId = data.addClass(classPointer, 0); + const auto classId = data.addClass(classPointer); if (classId == 0 && classPointer != 0) { - cerr << "Unknown class id (" << classPointer << ") here: " << reader.line() << endl; + cerr << "[W] Unknown class id (" << classPointer << ") here: " << reader.line() << endl; continue; - } + } const auto objectId = ptrToIndex.peekPointer(objectPointer); if (!objectId.second) - cerr << "unknown object id (" << objectPointer << ") here: " << reader.line() << endl; + cerr << "[W] unknown object id (" << objectPointer << ") here: " << reader.line() << endl; // trace point, map current output index to parent index fprintf(stdout, "e %zx %zx %zx %zx %zx\n", gcCounter, numChildren, objectPointer, classId, objectId.first.index); } else if (reader.mode() == 'C') { uintptr_t classPointer = 0; - size_t classSize = 0; - if (!(reader >> classPointer) || !(reader >> classSize)) { - cerr << "failed to parse line: " << reader.line() << endl; + if (!(reader >> classPointer)) { + cerr << "[W] failed to parse line: " << reader.line() << endl; continue; } - data.addClass(classPointer, classSize); + data.addClass(classPointer); } else { fputs(reader.line().c_str(), stdout); fputc('\n', stdout); diff --git a/src/track/libheaptrack.cpp b/src/track/libheaptrack.cpp index 02f04c2..84901a1 100644 --- a/src/track/libheaptrack.cpp +++ b/src/track/libheaptrack.cpp @@ -512,7 +512,7 @@ public: if (fprintf(s_data->out, "G 1\n") < 0) { writeError(); return; - } + } ObjectGraph graph; graph.clear(); @@ -529,7 +529,7 @@ public: rangeLength, reinterpret_cast(rangeStart), reinterpret_cast(rangeMovedTo)) < 0) { writeError(); return; - } + } } void handleFinishGC() @@ -543,19 +543,19 @@ public: if (fprintf(s_data->out, "G 0\n") < 0) { writeError(); return; - } + } ObjectGraph graph; graph.print(gc_counter, s_data->out); } - void handleLoadClass(void *classId, unsigned long classSize, char *className) { + void handleLoadClass(void *classId, char *className) { std::string formattedName; formattedName.append("["); formattedName.append(className); formattedName.append("]"); fprintf(s_data->out, "n %" PRIxPTR " %s\n", reinterpret_cast(classId), formattedName.c_str()); - fprintf(s_data->out, "C %" PRIxPTR " %lx\n", reinterpret_cast(classId), classSize); + fprintf(s_data->out, "C %" PRIxPTR "\n", reinterpret_cast(classId)); TraceTree::knownNames.insert(classId); } @@ -1009,22 +1009,34 @@ void heaptrack_finishgc() { } void heaptrack_add_object_dep(void *keyObjectId, void *keyClassId, void *valObjectId, void *valClassId) { + assert(!RecursionGuard::isActive); + RecursionGuard guard; + + debugLog("heaptrack_add_object_dep"); + ObjectGraph graph; graph.index(keyObjectId, keyClassId, valObjectId, valClassId); } void heaptrack_gcroot(void *objectId, void *classId) { + assert(!RecursionGuard::isActive); + RecursionGuard guard; + + debugLog("heaptrack_gcroot"); + ObjectGraph graph; graph.addRoot(objectId, classId); } __attribute__((noinline)) -void heaptrack_loadclass(void *classId, unsigned long classSize, char *className) { +void heaptrack_loadclass(void *classId, char *className) { assert(!RecursionGuard::isActive); RecursionGuard guard; + debugLog("heaptrack_loadclass"); + HeapTrack heaptrack(guard); - heaptrack.handleLoadClass(classId, classSize, className); + heaptrack.handleLoadClass(classId, className); } void heaptrack_invalidate_module_cache() diff --git a/src/util/pointermap.h b/src/util/pointermap.h index 2ca6605..eeb3d84 100644 --- a/src/util/pointermap.h +++ b/src/util/pointermap.h @@ -165,6 +165,7 @@ public: return {index, true}; } + // Get AllocationIndex for a pointer without removing it from the map std::pair peekPointer(const uint64_t ptr) { const SplitPointer pointer(ptr); -- 2.7.4