Bug 74100 - Web Inspector: provide per Document Node count statistics
Summary: Web Inspector: provide per Document Node count statistics
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: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-08 09:50 PST by Yury Semikhatsky
Modified: 2011-12-12 05:10 PST (History)
13 users (show)

See Also:


Attachments
[PATCH] Accidentally I have a patch for that already. (10.11 KB, patch)
2011-12-08 09:59 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (15.67 KB, patch)
2011-12-08 10:00 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (19.59 KB, patch)
2011-12-09 06:04 PST, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff
Patch for landing (26.29 KB, patch)
2011-12-09 07:26 PST, Yury Semikhatsky
pfeldman: review-
Details | Formatted Diff | Diff
Patch for landing (26.89 KB, patch)
2011-12-12 05:03 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2011-12-08 09:50:42 PST
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.
Comment 1 Pavel Feldman 2011-12-08 09:59:46 PST
Created attachment 118410 [details]
[PATCH] Accidentally I have a patch for that already.
Comment 2 Yury Semikhatsky 2011-12-08 10:00:38 PST
Created attachment 118411 [details]
Patch
Comment 3 WebKit Review Bot 2011-12-08 10:02:19 PST
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.
Comment 4 WebKit Review Bot 2011-12-08 10:03:44 PST
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.
Comment 5 Pavel Feldman 2011-12-08 10:04:05 PST
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.
Comment 6 Pavel Feldman 2011-12-08 10:38:12 PST
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.
Comment 7 Yury Semikhatsky 2011-12-08 10:39:18 PST
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.
Comment 8 Yury Semikhatsky 2011-12-08 10:44:25 PST
(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?
Comment 9 Yury Semikhatsky 2011-12-09 06:04:48 PST
Created attachment 118565 [details]
Patch
Comment 10 Pavel Feldman 2011-12-09 06:22:37 PST
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?
Comment 11 Yury Semikhatsky 2011-12-09 07:25:28 PST
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.
Comment 12 Yury Semikhatsky 2011-12-09 07:26:05 PST
Created attachment 118574 [details]
Patch for landing
Comment 13 Pavel Feldman 2011-12-09 08:20:00 PST
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
Comment 14 Yury Semikhatsky 2011-12-12 05:03:49 PST
Created attachment 118776 [details]
Patch for landing
Comment 15 Yury Semikhatsky 2011-12-12 05:04:13 PST
(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.
Comment 16 Yury Semikhatsky 2011-12-12 05:10:47 PST
Committed r102569: <http://trac.webkit.org/changeset/102569>