We take and transfer native heap snapshot as a single chunk. As a result the snapshot data affects the memory we are trying to count. We can significantly reduce this side effect if we change the transfer mechanic to the chunk based. Patch to follow.
Created attachment 186375 [details] Patch
Comment on attachment 186375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186375&action=review > Source/WebCore/ChangeLog:12 > + MemoryInstrumentation was slightly changed. Now it reports base to real address map only after reporting the node with real address. This should be in a separate change. > Source/WTF/wtf/MemoryInstrumentation.cpp:108 > + memoryInstrumentation->m_client->reportBaseAddress(m_pointer, realAddress); The method code can be rearranged so that reportNode is called always before reportBaseAddress and only if memoryObjectInfo.frstVisit() == true. Then you wouldn't need to duplicate this statement. > Source/WebCore/inspector/HeapGraphSerializer.cpp:77 > + && m_nodes->length() <= chunkSize * NodeFieldsCount It would probably be better to compare weighted sum of the collection sizes with some chunkSize. Also for strings we may want to count total length. > Source/WebCore/inspector/HeapGraphSerializer.cpp:104 > + m_prevNodeLastEdgeIndex = m_reportedEdgesCount; Can we use just m_reportedEdgesCount and reset it after reporting next node? > Source/WebCore/inspector/HeapGraphSerializer.h:53 > + HeapGraphSerializer(InspectorFrontend::Memory*); explicit > Source/WebCore/inspector/HeapGraphSerializer.h:73 > + void reportEdge(const int toNodeId, const char* name, int memberType); reportEdge -> reportEdgeImpl? > Source/WebCore/inspector/HeapGraphSerializer.h:87 > + static const size_t EdgeFieldsCount = 3; EdgeFieldsCount -> s_edgeFieldsCount > Source/WebCore/inspector/HeapGraphSerializer.h:91 > + static const size_t NodeFieldsCount = 5; NodeFieldsCount -> s_nodeFieldsCount > Source/WebCore/inspector/HeapGraphSerializer.h:94 > + RefPtr<NodeIdRemapping> m_nodeIdRemapping; m_nodeIdRemapping -> m_baseToRealNodeIdMap? > Source/WebCore/inspector/HeapGraphSerializer.h:95 > + static const size_t RemappingFieldsCount = 2; RemappingFieldsCount -> s_remappingFieldsCount > Source/WebCore/inspector/HeapGraphSerializer.h:105 > + typedef TypeBuilder::Memory::HeapSnapshotChunk HeapSnapshotChunk; Why do you need this in the header? > Source/WebCore/inspector/Inspector.json:130 > + "name": "finishNativeSnapshot", Can we use response to getProcessMemoryDistribution instead of this event? > Source/WebCore/inspector/InspectorMemoryAgent.cpp:556 > +void InspectorMemoryAgent::getProcessMemoryDistributionAsMap(bool reportGraph, TypeNameToSizeMap* memoryInfo) getProcessMemoryDistributionAsMap should not need reportGraph as the parameter doesn't affect the method result in any way and may confuse clients. > Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:387 > div.style.overflow = "visible"; please revert new empty lines > Source/WebCore/inspector/front-end/NativeHeapSnapshot.js:91 > + return this._snapshot._nodes[this.nodeIndex + this._snapshot._nodeTypeOffset]; why did this change? > Source/WebCore/inspector/front-end/NativeHeapSnapshot.js:113 > + type: 3, What is 3? > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:324 > + var id2idMap = {}; id2idMap -> baseToRealId > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:329 > + for (var i = 2; i < this._nodes.length; i += 5) 5 -> nodeFieldCount or meta.node_fields.length > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:334 > + for (var i = 2; i < edges.length; i += 3) { 3 -> edgeFieldCount, 2 -> meta.edge_fields.indexOf("to_node") ? > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:409 > + this._nodeId2nodeId = []; _baseToRealNodeId
(In reply to comment #2) > (From update of attachment 186375 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186375&action=review > > > Source/WebCore/ChangeLog:12 > > + MemoryInstrumentation was slightly changed. Now it reports base to real address map only after reporting the node with real address. > > This should be in a separate change. done. https://bugs.webkit.org/show_bug.cgi?id=109051 > > > Source/WTF/wtf/MemoryInstrumentation.cpp:108 > > + memoryInstrumentation->m_client->reportBaseAddress(m_pointer, realAddress); > > The method code can be rearranged so that reportNode is called always before reportBaseAddress and only if memoryObjectInfo.frstVisit() == true. Then you wouldn't need to duplicate this statement. done in the separate patch > > > Source/WebCore/inspector/HeapGraphSerializer.cpp:77 > > + && m_nodes->length() <= chunkSize * NodeFieldsCount > > It would probably be better to compare weighted sum of the collection sizes with some chunkSize. Also for strings we may want to count total length. I think we need to count the average coefficients on a real snapshot and apply it in a separate patch. > > > Source/WebCore/inspector/HeapGraphSerializer.cpp:104 > > + m_prevNodeLastEdgeIndex = m_reportedEdgesCount; > > Can we use just m_reportedEdgesCount and reset it after reporting next node? done > > > Source/WebCore/inspector/HeapGraphSerializer.h:53 > > + HeapGraphSerializer(InspectorFrontend::Memory*); > > explicit done > > > Source/WebCore/inspector/HeapGraphSerializer.h:73 > > + void reportEdge(const int toNodeId, const char* name, int memberType); > > reportEdge -> reportEdgeImpl? done > > > Source/WebCore/inspector/HeapGraphSerializer.h:87 > > + static const size_t EdgeFieldsCount = 3; > > EdgeFieldsCount -> s_edgeFieldsCount done > > > Source/WebCore/inspector/HeapGraphSerializer.h:91 > > + static const size_t NodeFieldsCount = 5; > > NodeFieldsCount -> s_nodeFieldsCount done > > > Source/WebCore/inspector/HeapGraphSerializer.h:94 > > + RefPtr<NodeIdRemapping> m_nodeIdRemapping; > > m_nodeIdRemapping -> m_baseToRealNodeIdMap? done > > > Source/WebCore/inspector/HeapGraphSerializer.h:95 > > + static const size_t RemappingFieldsCount = 2; > > RemappingFieldsCount -> s_remappingFieldsCount done > > > Source/WebCore/inspector/HeapGraphSerializer.h:105 > > + typedef TypeBuilder::Memory::HeapSnapshotChunk HeapSnapshotChunk; > > Why do you need this in the header? removed > > > Source/WebCore/inspector/Inspector.json:130 > > + "name": "finishNativeSnapshot", > > Can we use response to getProcessMemoryDistribution instead of this event? > > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:556 > > +void InspectorMemoryAgent::getProcessMemoryDistributionAsMap(bool reportGraph, TypeNameToSizeMap* memoryInfo) > > getProcessMemoryDistributionAsMap should not need reportGraph as the parameter doesn't affect the method result in any way and may confuse clients. This change is not related to the idea of the patch but I fixed that too. > > > Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:387 > > div.style.overflow = "visible"; > > please revert new empty lines done > > > Source/WebCore/inspector/front-end/NativeHeapSnapshot.js:91 > > + return this._snapshot._nodes[this.nodeIndex + this._snapshot._nodeTypeOffset]; > > why did this change? I'd like to use type field of the snapshot as a string id so we will use it for classNames. > > > Source/WebCore/inspector/front-end/NativeHeapSnapshot.js:113 > > + type: 3, > > What is 3? it is a fallback to JSHeapSnapshot types. 3 is the 'object' type. I moved this magic number to the constructor where it will be easily recognized. The frontend code needs further refactoring but this will be a part of next pathes. > > > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:324 > > + var id2idMap = {}; > > id2idMap -> baseToRealId done > > > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:329 > > + for (var i = 2; i < this._nodes.length; i += 5) > > 5 -> nodeFieldCount or meta.node_fields.length done > > > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:334 > > + for (var i = 2; i < edges.length; i += 3) { > > 3 -> edgeFieldCount, 2 -> meta.edge_fields.indexOf("to_node") ? done > > > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:409 > > + this._nodeId2nodeId = []; > > _baseToRealNodeId done
Created attachment 186863 [details] Patch
Comment on attachment 186863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186863&action=review > Source/WebCore/inspector/HeapGraphSerializer.cpp:70 > +void HeapGraphSerializer::pushUpdateIfNeed() pushUpdateIfNeed -> pushUpdateIfNeeded > Source/WebCore/inspector/HeapGraphSerializer.cpp:169 > + info.addMember(m_baseToRealNodeIdMap, "nodeIdRemapping"); nodeIdRemapping -> baseToRealNodeIdMap > Source/WebCore/inspector/HeapGraphSerializer.h:94 > + static const size_t s_remappingFieldsCount = 2; s_idMapEntryFieldCount ? > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:335 > + nodeId2NodeIndex[this._nodes[i]] = i - nodeIdFieldOffset; shift 4 spaces to the right
Committed r142074: <http://trac.webkit.org/changeset/142074>