Bug 109687 - Web Inspector: add experimental native heap graph to Timeline panel
Summary: Web Inspector: add experimental native heap graph to Timeline panel
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: 2013-02-13 06:15 PST by Yury Semikhatsky
Modified: 2013-02-13 16:43 PST (History)
12 users (show)

See Also:


Attachments
Patch (17.87 KB, patch)
2013-02-13 06:17 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Screenshot with the patch applied (105.31 KB, image/png)
2013-02-13 06:18 PST, Yury Semikhatsky
no flags Details
Patch (17.83 KB, patch)
2013-02-13 07:01 PST, Yury Semikhatsky
apavlov: 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 2013-02-13 06:15:07 PST
Statistics on the native memory usage will be collected after each top level task. The data will be represented as a graph instead of DOM counters for now.
Comment 1 Yury Semikhatsky 2013-02-13 06:17:55 PST
Created attachment 188068 [details]
Patch
Comment 2 Yury Semikhatsky 2013-02-13 06:18:34 PST
Created attachment 188069 [details]
Screenshot with the patch applied
Comment 3 Ilya Tikhonovsky 2013-02-13 06:55:32 PST
Comment on attachment 188068 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:548
> +    m_memoryAgent->getProcessMemoryDistributionMap(&map);

we have null m_memoryAgent here in case of Worker.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:552
> +//            printf("%s -> %ld\n", it->key.ascii().data(), it->value);

please remove this line
Comment 4 Yury Semikhatsky 2013-02-13 07:01:07 PST
Created attachment 188077 [details]
Patch
Comment 5 Yury Semikhatsky 2013-02-13 07:09:34 PST
(In reply to comment #3)
> (From update of attachment 188068 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188068&action=review
> 
> > Source/WebCore/inspector/InspectorTimelineAgent.cpp:548
> > +    m_memoryAgent->getProcessMemoryDistributionMap(&map);
> 
> we have null m_memoryAgent here in case of Worker.
> 
Done.

> > Source/WebCore/inspector/InspectorTimelineAgent.cpp:552
> > +//            printf("%s -> %ld\n", it->key.ascii().data(), it->value);
> 
> please remove this line
Done.
Comment 6 Alexander Pavlov (apavlov) 2013-02-13 07:21:25 PST
Comment on attachment 188077 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:2634
> +                    { "name": "enabled", "type": "boolean", "description": "True to start collecting statisics on native memory usage." }

Please rephrase to include the disablement (false) case

> Source/WebCore/inspector/Inspector.json:2626
> +                    { "name": "enabled", "type": "boolean", "description": "True to start reporting DOM counters." }

Please rephrase to include the stop (false) case

> Source/WebCore/inspector/Inspector.json:2634
> +                    { "name": "enabled", "type": "boolean", "description": "True to start collecting statisics on native memory usage." }

Please rephrase to include the stop (false) case

> Source/WebCore/inspector/Inspector.json:2637
> +                "description": "Starts sending statistics on native memory usage along with timeline events."

"Starts sending" -> "Whether to send"?
Comment 7 Yury Semikhatsky 2013-02-13 07:24:06 PST
(In reply to comment #6)
> (From update of attachment 188077 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188077&action=review
> 
> > Source/WebCore/inspector/Inspector.json:2634
> > +                    { "name": "enabled", "type": "boolean", "description": "True to start collecting statisics on native memory usage." }
> 
> Please rephrase to include the disablement (false) case
> 
Done.

> > Source/WebCore/inspector/Inspector.json:2626
> > +                    { "name": "enabled", "type": "boolean", "description": "True to start reporting DOM counters." }
> 
> Please rephrase to include the stop (false) case
> 
Done.

> > Source/WebCore/inspector/Inspector.json:2634
> > +                    { "name": "enabled", "type": "boolean", "description": "True to start collecting statisics on native memory usage." }
> 
> Please rephrase to include the stop (false) case
> 
Done.

> > Source/WebCore/inspector/Inspector.json:2637
> > +                "description": "Starts sending statistics on native memory usage along with timeline events."
> 
> "Starts sending" -> "Whether to send"?
Done.
Comment 8 Yury Semikhatsky 2013-02-13 07:29:31 PST
Committed r142746: <http://trac.webkit.org/changeset/142746>
Comment 9 Joseph Pecoraro 2013-02-13 16:43:38 PST
Comment on attachment 188077 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:2624
> +                "name": "setIncludeDomCounters",

The Inspector Protocol and WebKit project consistently use "DOM" capitalization. The "Dom" here looks out of place:

    getDOMCounters, addDOMStorage, setDOMBreakpoint, removeDOMBreakpoint, ...

Quick follow-up?