[webkit-reviews] review denied: [Bug 108824] Web Inspector: Native Memory Instrumentation: reduce native heap snapshot runtime memory footprint : [Attachment 186375] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 108824: Web Inspector: Native Memory Instrumentation: reduce native heap
snapshot runtime memory footprint
https://bugs.webkit.org/show_bug.cgi?id=108824

Attachment 186375: Patch
https://bugs.webkit.org/attachment.cgi?id=186375&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
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