Bug 89106 - Web Inspector: Implement native memory bar diagram
Summary: Web Inspector: Implement native memory bar diagram
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: Alexei Filippov
URL:
Keywords:
Depends on: 89518
Blocks: 87262
  Show dependency treegraph
 
Reported: 2012-06-14 09:02 PDT by Alexei Filippov
Modified: 2012-06-21 08:45 PDT (History)
12 users (show)

See Also:


Attachments
screenshot (55.38 KB, image/png)
2012-06-14 09:07 PDT, Alexei Filippov
no flags Details
Patch (5.97 KB, patch)
2012-06-14 09:14 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (8.56 KB, patch)
2012-06-15 06:21 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
screenshot (82.08 KB, image/png)
2012-06-15 06:25 PDT, Alexei Filippov
no flags Details
Patch (8.52 KB, patch)
2012-06-15 08:59 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
screenshot (83.54 KB, image/png)
2012-06-15 09:03 PDT, Alexei Filippov
no flags Details
Patch (9.25 KB, patch)
2012-06-15 10:24 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (9.25 KB, patch)
2012-06-15 10:28 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (7.51 KB, patch)
2012-06-18 09:51 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
screenshot (82.74 KB, image/png)
2012-06-18 11:32 PDT, Alexei Filippov
no flags Details
Patch (8.65 KB, patch)
2012-06-20 01:16 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexei Filippov 2012-06-14 09:02:23 PDT
We currently have native memory pie chart implemented.
Bar diagram has one advantage over a pie chart: you can see the absolute amount of memory allocated.
Comment 1 Alexei Filippov 2012-06-14 09:07:49 PDT
Created attachment 147594 [details]
screenshot
Comment 2 Alexei Filippov 2012-06-14 09:14:39 PDT
Created attachment 147597 [details]
Patch
Comment 3 Ilya Tikhonovsky 2012-06-15 02:17:36 PDT
Comment on attachment 147597 [details]
Patch

I'd like to see Total at the top of the list and percentages
Comment 4 Alexei Filippov 2012-06-15 06:21:53 PDT
Created attachment 147797 [details]
Patch
Comment 5 Alexei Filippov 2012-06-15 06:25:28 PDT
Created attachment 147799 [details]
screenshot
Comment 6 Alexei Filippov 2012-06-15 08:59:43 PDT
Created attachment 147830 [details]
Patch
Comment 7 Alexei Filippov 2012-06-15 09:03:11 PDT
Created attachment 147831 [details]
screenshot
Comment 8 Ilya Tikhonovsky 2012-06-15 09:46:30 PDT
Comment on attachment 147830 [details]
Patch

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

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:444
> +    _paint: function()

too long method. please split it.

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:495
> +                /*
> +                if (subchildSize < 0.95 * child.size)
> +                    sizeText = toMB(subchildSize).toFixed(1) + " of " + sizeText;
> +                */

We don't keep unused code in WebKit.

> Source/WebCore/inspector/front-end/ProfileLauncherView.js:56
> +    if (WebInspector.experimentsSettings.nativeMemorySnapshots.isEnabled()) {

please use separate experimental flag
Comment 9 Ilya Tikhonovsky 2012-06-15 09:49:19 PDT
lgtm: please fix nits before landing
Comment 10 Alexei Filippov 2012-06-15 10:19:17 PDT
Comment on attachment 147830 [details]
Patch

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

>> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:444
>> +    _paint: function()
> 
> too long method. please split it.

It's hard to split it as it has lots of dependencies. E.g. to move the loop body into a separate method would require it to have six arguments.
However I managed to make it a bit shorter.

>> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:495
>> +                */
> 
> We don't keep unused code in WebKit.

done.

>> Source/WebCore/inspector/front-end/ProfileLauncherView.js:56
>> +    if (WebInspector.experimentsSettings.nativeMemorySnapshots.isEnabled()) {
> 
> please use separate experimental flag

done
Comment 11 Alexei Filippov 2012-06-15 10:24:02 PDT
Created attachment 147850 [details]
Patch
Comment 12 Alexei Filippov 2012-06-15 10:28:23 PDT
Created attachment 147852 [details]
Patch
Comment 13 Pavel Feldman 2012-06-15 10:42:01 PDT
Comment on attachment 147852 [details]
Patch

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

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:475
> +        for (var i = 0; i < memoryBlock.children.length; ++i) {

You should render it using DOM instead!
Comment 14 Alexei Filippov 2012-06-18 09:51:21 PDT
Created attachment 148109 [details]
Patch
Comment 15 Alexei Filippov 2012-06-18 09:56:03 PDT
Comment on attachment 147852 [details]
Patch

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

>> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:475
>> +        for (var i = 0; i < memoryBlock.children.length; ++i) {
> 
> You should render it using DOM instead!

Done
Comment 16 Alexei Filippov 2012-06-18 11:32:10 PDT
Created attachment 148137 [details]
screenshot
Comment 17 Ilya Tikhonovsky 2012-06-18 13:17:55 PDT
Comment on attachment 148109 [details]
Patch

Clearing flags on attachment: 148109

Committed r120621: <http://trac.webkit.org/changeset/120621>
Comment 18 Ilya Tikhonovsky 2012-06-18 13:18:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Review Bot 2012-06-19 15:36:25 PDT
Re-opened since this is blocked by 89518
Comment 20 Alexei Filippov 2012-06-20 01:16:59 PDT
Created attachment 148522 [details]
Patch
Comment 21 Alexei Filippov 2012-06-20 01:20:38 PDT
Put the live bars behind an experimental flag.
Comment 22 WebKit Review Bot 2012-06-20 06:32:31 PDT
Comment on attachment 148522 [details]
Patch

Clearing flags on attachment: 148522

Committed r120819: <http://trac.webkit.org/changeset/120819>
Comment 23 WebKit Review Bot 2012-06-20 06:32:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Simon Fraser (smfr) 2012-06-20 11:40:28 PDT
Does "native memory" include memory consumed by compositing layers?
Comment 25 Yury Semikhatsky 2012-06-21 01:00:00 PDT
(In reply to comment #24)
> Does "native memory" include memory consumed by compositing layers?

By "native memory" we mean all memory consumed by the process owning the inspected Page. As described in http://wkb.ug/87262 our goal is to give the user a clear insight into where the memory goes in the inspected process.

If compositing layers are allocated in the inspected process then we would like to plot it on the chart. At the moment we show total RenderArena size for the inspected page. Could you point us at the places where the compositing layers are allocated so that we can add them to the diagram?
Comment 26 Simon Fraser (smfr) 2012-06-21 08:45:08 PDT
You'll need to build up a comprehensive list of memory consumers that are outside the DOM. Some of these will be in subsystems outside of WebKit, so I'm not sure you'll be able to get them all.

Off the top of my head:

media elements (<audio> and <video>)
Web Audio data
image resources
plugins
canvas backing store
compositing layers
image buffers
caches in webcore code

Compositing layers are allocated via RenderLayerBacking. There's a backingStoreArea() method that returns the number of pixels. Multiply by 4 to get bytes (we could change this to return bytes).

You should file new bugs for the various memory consumers that you intend to track.