1106137 - Make canvas debugger faster by avoiding a double-copy operation when deserializing image data

Status: RESOLVED FIXED

(DevTools Graveyard :: Canvas Debugger, defect)

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: avoid-double-copy.patch

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: avoid-double-copy.patch

Explaining this seemingly weird mess of array buffer constructors:
Previous behavior:
1. make an array buffer (representing pixels) in the actor
2. manipulate its 32-bit view somehow (one pixel at a time)
3. pass that 32-bit view to the protocol's serializer - since array buffer views can't be properly stringified as JSON, manually join the numbers with commas to represent a plain array
4. deserialize it on the client as plain array and create a 32-bit view from it
5. create an ImageData object
6. copy the previously created 32-bit view's buffer into the ImageData's 8-bit view's buffer

A keen observer would realize that this involves one extra redundant copy operation: step 4 vs. 6. The new approach is:

1. make an array buffer (representing pixels) in the actor
2. manipulate its 32-bit view somehow (one pixel at a time)
3. pass an 8-bit view of that array buffer to the protocol's serializer
4. deserialize it on the client as plain array
5. create an ImageData object
6. copy the previously deserialized array representing an 8-bit view into the ImageData's 8-bit view's buffer

This saves us a very costly copy operation, at the expense of serializing 4 times as many numbers. This is a good exchange.

Obviously, it'd still be better if protocol.js could just "hand over" the array buffer without serializing one of its views, but what we have right now is still mostly ok.

Comment 3

[Victor Porof [:vporof][:vp]]

Comment on attachment 8530449 [details] [diff] [review]
avoid-double-copy.patch

Review of attachment 8530449 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/canvas.js
@@ +62,2 @@
>    write: (v) => "[" + Array.join(v, ",") + "]",
> +  read: (v) => JSON.parse(v)

To be more precise, this was the redundant copy. Creating an array buffer view from a regular js array (or another view) involves a copy, as opposed to creating it from a buffer, which does not.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v2

With text fixes.

Comment 5

[Victor Porof [:vporof][:vp]]

Ugly run, but green for this bug: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c31b4c9a4c65

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8532589 [details] [diff] [review]
v2

Review of attachment 8532589 [details] [diff] [review]:
-----------------------------------------------------------------

This all looks good to me.
I'd like to see a comment (something general at the top of canvas.js) that explains the approach described in comment 2.

Comment 7

[Victor Porof [:vporof][:vp]]

Added comment and landed: https://hg.mozilla.org/integration/fx-team/rev/8a6b14d8320e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a6b14d8320e