Bug 78825 - Web Inspector: use static counters for estimation of allocated Documents, Nodes and JS EventListeners
Summary: Web Inspector: use static counters for estimation of allocated Documents, Nod...
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: 2012-02-16 09:30 PST by Yury Semikhatsky
Modified: 2012-02-17 00:33 PST (History)
10 users (show)

See Also:


Attachments
Patch (20.52 KB, patch)
2012-02-16 09:33 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (21.53 KB, patch)
2012-02-16 09:37 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (32.77 KB, patch)
2012-02-16 23:38 PST, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2012-02-16 09:30:22 PST
This counters cost nothing but will be very useful in the Timeline panel. Values of the counters could be sent along with each timeline event unlike DOM counters collected by traversing DOM on each event since their calculation is quite expensive.
Comment 1 Yury Semikhatsky 2012-02-16 09:33:01 PST
Created attachment 127392 [details]
Patch
Comment 2 Yury Semikhatsky 2012-02-16 09:37:21 PST
Created attachment 127394 [details]
Patch
Comment 3 Pavel Feldman 2012-02-16 10:23:11 PST
Comment on attachment 127392 [details]
Patch

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

> Source/WebCore/bindings/v8/V8AbstractEventListener.cpp:38
> +#include "InspectorInstrumentation.h"

I would introduce InspectorCounters class that would have all the counter fields and guarantee a no-op upon the instrumentation call. WebCore/dom would update its values directly with no need for s_* in Node.h.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:334
> +    ++InspectorMemoryAgent::counter(InspectorMemoryAgent::JSEventListenerCounter);

You could merge memory counters into the new InspectorCounter class.
Comment 4 Ilya Tikhonovsky 2012-02-16 10:27:29 PST
Comment on attachment 127394 [details]
Patch

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

> Source/WebCore/inspector/InspectorInstrumentation.cpp:334
> +    ++InspectorMemoryAgent::counter(InspectorMemoryAgent::JSEventListenerCounter);

it looks strange. I'd prefer to implement the counter as a class with int& operator[](int index) { .... }
Comment 5 Yury Semikhatsky 2012-02-16 23:38:14 PST
Created attachment 127532 [details]
Patch
Comment 6 Yury Semikhatsky 2012-02-16 23:41:22 PST
(In reply to comment #3)
> (From update of attachment 127392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127392&action=review
> 
> > Source/WebCore/bindings/v8/V8AbstractEventListener.cpp:38
> > +#include "InspectorInstrumentation.h"
> 
> I would introduce InspectorCounters class that would have all the counter fields and guarantee a no-op upon the instrumentation call. WebCore/dom would update its values directly with no need for s_* in Node.h.
> 
Done.

> > Source/WebCore/inspector/InspectorInstrumentation.cpp:334
> > +    ++InspectorMemoryAgent::counter(InspectorMemoryAgent::JSEventListenerCounter);
> 
> You could merge memory counters into the new InspectorCounter class.
Done.

(In reply to comment #4)
> (From update of attachment 127394 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127394&action=review
> 
> > Source/WebCore/inspector/InspectorInstrumentation.cpp:334
> > +    ++InspectorMemoryAgent::counter(InspectorMemoryAgent::JSEventListenerCounter);
> 
> it looks strange. I'd prefer to implement the counter as a class with int& operator[](int index) { .... }

There is no method which would return a reference to the counter anymore. Instead we have more restrictive inc/dec methods and value getter.
Comment 7 Yury Semikhatsky 2012-02-17 00:33:56 PST
Committed r108047: <http://trac.webkit.org/changeset/108047>