Bug 74100 – Web Inspector: provide per Document Node count statistics

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

[Yury Semikhatsky]

Memory agent should provide per Document Node count statistics for all documents reachable from the main frame and also for those detached from the page.

[Pavel Feldman]

Created attachment 118410 [details]
[PATCH] Accidentally I have a patch for that already.

[Yury Semikhatsky]

Created attachment 118411 [details]
Patch

[WebKit Review Bot]

Attachment 118410 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorMemoryAgent.cpp:50:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorMemoryAgent.h:60:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorMemoryAgent.h:65:  The parameter name "domAgent" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 118411 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1

Source/WebCore/inspector/InspectorMemoryAgent.h:58:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorMemoryAgent.cpp:72:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 118411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118411&action=review

> Source/WebCore/inspector/Inspector.json:56
> +                    { "name": "statistics", "type": "object" }

You should define the object structure here.

> Source/WebCore/inspector/InspectorInstrumentation.h:91
> +    static void willDetachDocument(Document*);

In addition to the detached documents, there are detached substrees that deserve to be reported.

[Pavel Feldman]

I don't quite like the fact that we maintain detached documents list for providing memory statistics. I think we should make these stats based on the object groups / bindings info.

However, as you noticed offline, your way reports more dangling documents than my way, so for the WebKit owners it is important to know that something is retaining the document. Why don't we put your code to under the DETAILED_MEMORY_INFO? We would be able to compile with it locally and compare it with the bindings information. Should these to get different, we would know that there is likely a memory leak in the renderer.

[Yury Semikhatsky]

Comment on attachment 118410 [details]
[PATCH] Accidentally I have a patch for that already.

View in context: https://bugs.webkit.org/attachment.cgi?id=118410&action=review

> Source/WebCore/bindings/v8/ScriptProfiler.cpp:220
> +    CounterVisitor counterVisitor(domAgent, result.get());

This visitor will traverse all nodes no matter which page they belong too. I think we should collect only nodes relevant to a particular page which user is inspecting.

[Yury Semikhatsky]

(In reply to comment #6)
> I don't quite like the fact that we maintain detached documents list for providing memory statistics. I think we should make these stats based on the object groups / bindings info.
> 
> However, as you noticed offline, your way reports more dangling documents than my way, so for the WebKit owners it is important to know that something is retaining the document. Why don't we put your code to under the DETAILED_MEMORY_INFO? We would be able to compile with it locally and compare it with the bindings information. Should these to get different, we would know that there is likely a memory leak in the renderer.

I don't like the idea of hiding it behind the flag, I'd rather remove it completely and look for a better way which would server our users as well. If we are worried about the performance hit here and think that all dangling documents should be reachable from the JS bindings but still want to check that assumption there is an easy way to reach that: we may use a counter of detached documents. It would cost nothing and we'd be able to compare it with the documents reachable from the JS heap. What do you think?

[Yury Semikhatsky]

Created attachment 118565 [details]
Patch

[Pavel Feldman]

Comment on attachment 118565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118565&action=review

> Source/WebCore/bindings/js/ScriptProfiler.h:54
> +    class DOMWrapperVisitor {

Please move this interface to the WebCore/inspector

> Source/WebCore/inspector/Inspector.json:57
> +                    { "name": "nodeCount", "type": "object" },

You should expand this into the type.

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:56
> +    

blank line

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:88
> +            String name = nodeName(currentNode);

You only want names of the elements, right?

[Yury Semikhatsky]

Comment on attachment 118565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118565&action=review

>> Source/WebCore/bindings/js/ScriptProfiler.h:54

> 
> Please move this interface to the WebCore/inspector

Done.

>> Source/WebCore/inspector/Inspector.json:57
>> +                    { "name": "nodeCount", "type": "object" },
> 
> You should expand this into the type.

Done.

>> Source/WebCore/inspector/InspectorMemoryAgent.cpp:56

> 
> blank line

Removed.

>> Source/WebCore/inspector/InspectorMemoryAgent.cpp:88

> 
> You only want names of the elements, right?

No, I think we should show #text etc. as well.

[Yury Semikhatsky]

Created attachment 118574 [details]
Patch for landing

[Pavel Feldman]

Comment on attachment 118574 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=118574&action=review

> Source/WebCore/inspector/Inspector.json:55
> +                "description": "Node name to count map."

We should not use maps as the types since as you can see you can't define schema for it. It should be an array of objects with named properties instead.

> Source/WebCore/inspector/Inspector.json:62
> +                    { "name": "nodeCount", "$ref": "NodeCount" },

i.e. type: "array" items: { $ref: "NodeCounter" } where node counter has properties such as tagName and a count

[Yury Semikhatsky]

Created attachment 118776 [details]
Patch for landing

[Yury Semikhatsky]

(In reply to comment #13)
> (From update of attachment 118574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118574&action=review
> 
> > Source/WebCore/inspector/Inspector.json:55
> > +                "description": "Node name to count map."
> 
> We should not use maps as the types since as you can see you can't define schema for it. It should be an array of objects with named properties instead.
> 
Done.

> > Source/WebCore/inspector/Inspector.json:62
> > +                    { "name": "nodeCount", "$ref": "NodeCount" },
> 
> i.e. type: "array" items: { $ref: "NodeCounter" } where node counter has properties such as tagName and a count
Done.

[Yury Semikhatsky]

Committed r102569: <http://trac.webkit.org/changeset/102569>