Bug 173087

Summary:

Web Inspector: Instrument active pixel memory used by canvases

Product:

WebKit

Reporter:

Matt Baker <mattbaker>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, hi, inspector-bugzilla-changes, joepeck, keith_miller, magomez, mark.lam, mcatanzaro, msaboff, nvasilyev, rniwa, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

138941

Bug Blocks:

Attachments:

Description	Flags
[Patch] WIP	hi: review-, hi: commit-queue-
[Image] After Patch is applied	none
Patch	buildbot: commit-queue-
Archive of layout-test-results from ews107 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews117 for mac-elcapitan	none
Archive of layout-test-results from ews100 for mac-elcapitan	none
Patch	none
Patch	joepeck: review-
Patch	joepeck: review+, joepeck: commit-queue-
Patch	none

Matt Baker

Reported 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).

Attachments
[Patch] WIP (22.86 KB, patch) 2017-06-14 16:22 PDT, Devin Rousso	hi: review- hi: 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
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2017-06-12 12:29:14 PDT

<rdar://problem/32719261>

Devin Rousso

Comment 2 2017-06-14 16:22:41 PDT

Created attachment 312930 [details] [Patch] WIP

Devin Rousso

Comment 3 2017-06-14 16:22:56 PDT

Created attachment 312931 [details] [Image] After Patch is applied

Nikita Vasilyev

Comment 4 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?

Devin Rousso

Comment 5 2017-06-20 18:46:38 PDT

Created attachment 313466 [details] Patch

Build Bot

Comment 6 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

Build Bot

Comment 7 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

Build Bot

Comment 8 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.

Build Bot

Comment 9 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

Build Bot

Comment 10 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

Build Bot

Comment 11 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

Devin Rousso

Comment 12 2017-06-20 21:41:12 PDT

Created attachment 313486 [details] Patch

Devin Rousso

Comment 13 2017-06-20 21:48:14 PDT

Created attachment 313488 [details] Patch

Joseph Pecoraro

Comment 14 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.

Joseph Pecoraro

Comment 15 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());`

Devin Rousso

Comment 16 2017-06-27 23:06:28 PDT

Created attachment 313996 [details] Patch

Joseph Pecoraro

Comment 17 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.

Devin Rousso

Comment 18 2017-06-28 16:07:30 PDT

Created attachment 314063 [details] Patch

WebKit Commit Bot

Comment 19 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>

WebKit Commit Bot

Comment 20 2017-06-28 18:34:57 PDT

All reviewed patches have been landed. Closing bug.

Michael Catanzaro

Comment 21 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?

Devin Rousso

Comment 22 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.

Note You need to log in before you can comment on or make changes to this bug.