Bug 108824 – Web Inspector: Native Memory Instrumentation: reduce native heap snapshot runtime memory footprint

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Ilya Tikhonovsky]

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.

[Ilya Tikhonovsky]

Created attachment 186375 [details]
Patch

[Yury Semikhatsky]

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

[Ilya Tikhonovsky]

(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

[Ilya Tikhonovsky]

Created attachment 186863 [details]
Patch

[Yury Semikhatsky]

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

[Ilya Tikhonovsky]

Committed r142074: <http://trac.webkit.org/changeset/142074>