Bug 173087 - Web Inspector: Instrument active pixel memory used by canvases
Summary: Web Inspector: Instrument active pixel memory used by canvases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 138941
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-07 21:08 PDT by Matt Baker
Modified: 2017-07-07 10:50 PDT (History)
17 users (show)

See Also:


Attachments
[Patch] WIP (22.86 KB, patch)
2017-06-14 16:22 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
[Image] After Patch is applied (676.82 KB, image/png)
2017-06-14 16:22 PDT, Devin Rousso
no flags Details
Patch (27.07 KB, patch)
2017-06-20 18:46 PDT, Devin Rousso
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.06 MB, application/zip)
2017-06-20 19:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (2.33 MB, application/zip)
2017-06-20 20:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.08 MB, application/zip)
2017-06-20 20:47 PDT, Build Bot
no flags Details
Patch (27.48 KB, patch)
2017-06-20 21:41 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (27.52 KB, patch)
2017-06-20 21:48 PDT, Devin Rousso
joepeck: review-
Details | Formatted Diff | Diff
Patch (23.50 KB, patch)
2017-06-27 23:06 PDT, Devin Rousso
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
Patch (24.26 KB, patch)
2017-06-28 16:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2017-06-07 21:08:51 PDT
Summary:
Instrument active pixel memory used by canvases.

1) Add property `Canvas.memoryCost` to the Canvas protocol. Get canvas pixel memory using:

size_t HTMLCanvasElement::memoryCost() const;
size_t HTMLCanvasElement::externalMemoryCost() const;

2) Add event `canvasMemoryChanged` to the Canvas protocol. Instrumentation points:

void HTMLCanvasElement::createImageBuffer() const;
void HTMLCanvasElement::setImageBuffer(std::unique_ptr<ImageBuffer>) const;

3) It might be useful to display the maximum pixel memory available to all canvases. This info can be retrieved by hooking into `maxActivePixelMemory()` (see HTMLCanvasElement.cpp:190).
Comment 1 Radar WebKit Bug Importer 2017-06-12 12:29:14 PDT
<rdar://problem/32719261>
Comment 2 Devin Rousso 2017-06-14 16:22:41 PDT
Created attachment 312930 [details]
[Patch] WIP
Comment 3 Devin Rousso 2017-06-14 16:22:56 PDT
Created attachment 312931 [details]
[Image] After Patch is applied
Comment 4 Nikita Vasilyev 2017-06-14 16:57:21 PDT
(In reply to Devin Rousso from comment #3)
> Created attachment 312931 [details]
> [Image] After Patch is applied

    Memory: 4.47 MB / 4 GB

This wasn't clear to me that the second number is "maximum pixel memory available to all canvases".

What happens if canvas pixel memory is greater than this number?
Comment 5 Devin Rousso 2017-06-20 18:46:38 PDT
Created attachment 313466 [details]
Patch
Comment 6 Build Bot 2017-06-20 19:53:23 PDT
Comment on attachment 313466 [details]
Patch

Attachment 313466 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3969320

New failing tests:
inspector/canvas/memory.html
Comment 7 Build Bot 2017-06-20 19:53:24 PDT
Created attachment 313476 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-06-20 20:02:01 PDT
Comment on attachment 313466 [details]
Patch

Attachment 313466 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3969301

Number of test failures exceeded the failure limit.
Comment 9 Build Bot 2017-06-20 20:02:03 PDT
Created attachment 313478 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 Build Bot 2017-06-20 20:47:51 PDT
Comment on attachment 313466 [details]
Patch

Attachment 313466 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3969595

New failing tests:
inspector/canvas/memory.html
Comment 11 Build Bot 2017-06-20 20:47:53 PDT
Created attachment 313482 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Devin Rousso 2017-06-20 21:41:12 PDT
Created attachment 313486 [details]
Patch
Comment 13 Devin Rousso 2017-06-20 21:48:14 PDT
Created attachment 313488 [details]
Patch
Comment 14 Joseph Pecoraro 2017-06-21 13:36:34 PDT
(In reply to Nikita Vasilyev from comment #4)
> (In reply to Devin Rousso from comment #3)
> > Created attachment 312931 [details]
> > [Image] After Patch is applied
> 
>     Memory: 4.47 MB / 4 GB
> 
> This wasn't clear to me that the second number is "maximum pixel memory
> available to all canvases".

I agree. My initial thought was that the canvas was currently 4.5MB and can grow to 4GB which was not correct. How can we make this clearer? Maybe:

    Memory: 4.47MB (4 GB Limit)

> What happens if canvas pixel memory is greater than this number?

That shouldn't be allowed to happen.
Comment 15 Joseph Pecoraro 2017-06-24 00:56:15 PDT
Comment on attachment 313488 [details]
Patch

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

r- to see another patch after dropping the "max" thing. The rest looks good.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:42
> +                { "name": "memoryCost", "type": "number", "optional": true, "description": "Memory usage of the canvas." }

This is great!

> Source/JavaScriptCore/inspector/protocol/Canvas.json:80
> +        {
> +            "name": "maxPixelMemory",
> +            "description": "Gets the maximum pixel memory available for all canvases.",
> +            "returns": [
> +                { "name": "maxPixelMemory", "type": "number", "description": "Maximum pixel memory available for all canvases." }
> +            ]

Lets drop this. I don't think this max is useful. If/when the page crosses this max pixel memory a console message will be logged that lists what the maximum size is.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:99
> +                { "name": "canvasId", "$ref": "CanvasId", "description": "Identifier of canvas who's memory cost changed." },

Simplify the description.

> Source/WebCore/inspector/InspectorInstrumentation.h:1200
> +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForDocument(&canvasElement.document()))

This should `FAST_RETURN_IF_NO_FRONTENDS(void());`
Comment 16 Devin Rousso 2017-06-27 23:06:28 PDT
Created attachment 313996 [details]
Patch
Comment 17 Joseph Pecoraro 2017-06-28 11:57:43 PDT
Comment on attachment 313996 [details]
Patch

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

r=me, nice tests

> LayoutTests/inspector/canvas/memory.html:55
> +            InspectorTest.log(`Change size of canvas to ${width}x${height}.`);

We typically use this "\xd7", not that its important here though.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:42
> +                { "name": "memoryCost", "type": "number", "optional": true, "description": "Memory usage of the canvas." }

Lets specify "in bytes" somewhere.

> Source/WebInspectorUI/.eslintrc:37
> +        "CanvasAgent": true,

Pro.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:110
> +    CanvasMemoryChanged: "canvas-manager-canvas-memory-changed",

Its weird to me that this event is on the CanvasManager and not the Canvas instance that is changing.

If it was on the Canvas object then listeners could either listen on a particular instance:

    instance.addEventListener(WebInspector.Canvas.Event.MemoryChanged, () => {});

Or for all canvases:

    WebInspector.Canvas.addEventListener(WebInspector.Canvas.Event.MemoryChanged, () => {});

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:96
> +    set memoryCost(memoryCost) { this._memoryCost = memoryCost; }

This is where I would have expected the event to be dispatched.
Comment 18 Devin Rousso 2017-06-28 16:07:30 PDT
Created attachment 314063 [details]
Patch
Comment 19 WebKit Commit Bot 2017-06-28 18:34:55 PDT
Comment on attachment 314063 [details]
Patch

Clearing flags on attachment: 314063

Committed r218908: <http://trac.webkit.org/changeset/218908>
Comment 20 WebKit Commit Bot 2017-06-28 18:34:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Michael Catanzaro 2017-07-06 22:52:57 PDT
Hey Devin, this new test is failing on GTK:

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/inspector/canvas/memory-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/inspector/canvas/memory-actual.txt
@@ -7,5 +7,5 @@
 
 -- Running test case: Canvas.memory.canvasMemoryChanged
 Change size of canvas to 200x200.
-Memory cost of canvas updated to 166400.
+Memory cost of canvas updated to 160000.

Is that harmless, or should it exactly equal 160000 on our port?
Comment 22 Devin Rousso 2017-07-07 10:50:55 PDT
(In reply to Michael Catanzaro from comment #21)
> Hey Devin, this new test is failing on GTK:
> 
> ---
> /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/
> inspector/canvas/memory-expected.txt
> +++
> /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/
> inspector/canvas/memory-actual.txt
> @@ -7,5 +7,5 @@
>  
>  -- Running test case: Canvas.memory.canvasMemoryChanged
>  Change size of canvas to 200x200.
> -Memory cost of canvas updated to 166400.
> +Memory cost of canvas updated to 160000.
> 
> Is that harmless, or should it exactly equal 160000 on our port?

As far as I understand, the memory cost is just (width * height * 4).  I'm not actually sure why it shows as 166400 :|  I think 160000 is fine.