Bug 108824 - Web Inspector: Native Memory Instrumentation: reduce native heap snapshot runtime memory footprint
Summary: Web Inspector: Native Memory Instrumentation: reduce native heap snapshot run...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL:
Keywords:
Depends on:
Blocks: 107254
  Show dependency treegraph
 
Reported: 2013-02-04 07:47 PST by Ilya Tikhonovsky
Modified: 2013-02-07 00:39 PST (History)
11 users (show)

See Also:


Attachments
Patch (39.17 KB, patch)
2013-02-04 08:01 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (35.08 KB, patch)
2013-02-06 08:44 PST, Ilya Tikhonovsky
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2013-02-04 07:47:52 PST
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.
Comment 1 Ilya Tikhonovsky 2013-02-04 08:01:50 PST
Created attachment 186375 [details]
Patch
Comment 2 Yury Semikhatsky 2013-02-06 07:21:07 PST
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
Comment 3 Ilya Tikhonovsky 2013-02-06 08:43:47 PST
(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
Comment 4 Ilya Tikhonovsky 2013-02-06 08:44:12 PST
Created attachment 186863 [details]
Patch
Comment 5 Yury Semikhatsky 2013-02-07 00:24:32 PST
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
Comment 6 Ilya Tikhonovsky 2013-02-07 00:39:47 PST
Committed r142074: <http://trac.webkit.org/changeset/142074>