1204172 - Doing Uint8Array.set to a cross-global Uint8Array causes 1-2 second freeze on main thread

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[noitidart]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150907144446

Steps to reproduce:

Transferred ArrayBuffer from ChromeWorker to main thread.

On the main thread I create an ImageData and set this array buffer in like this:

var w = 1920, h = 1280;
var myArrBuff = new ArrayBuffer(w * h * 4);
var cidat = new ImageData(w, h);
cidat.data.set(new Uint8ClampedArray(myArrBuff));



If pass the typed array as constructor it wont freeze which is very interesting:
var w = 1920, h = 1280;
var myArrBuff = new ArrayBuffer(w * h * 4);
var cidat = new ImageData(myArrBuff, w, h);



Expected results:

No freeze when doing .set

Comment 1

[Boris Zbarsky [:bzbarsky]]

> cidat.data.set(new Uint8ClampedArray(myArrBuff));

This requires a copy of the data to be made, right?  Can you please link to, or attach, an actual testcase that shows the problem?  I wouldn't expect a copy of a few megabytes of data to take 1-2 seconds, so something else is going on here...

> If pass the typed array as constructor it wont freeze which is very interesting:

Well, at the very least this doesn't require a copy, yes?  It just stores a poiner to the object in the imagedata.

Comment 2

[noitidart]

(In reply to Boris Zbarsky [:bz] from comment #1)
> > cidat.data.set(new Uint8ClampedArray(myArrBuff));
> 
> This requires a copy of the data to be made, right?  Can you please link to,
> or attach, an actual testcase that shows the problem?

Thanks Boris, the actual situation is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1203976

I accidentally thought it was due to transfer but its not. I found out this bug when I fixed this freeze issue but passing it to constructor instead: https://github.com/Noitidart/NativeShot/commit/fe8a4d33a71ead841bb7a9f2037f325bbffd34fb#diff-b83c0bde60e252508b3088b391d3c14fL2073

I'm trying to not make a copy, just transfer from ArrayBuffer sent from ChromeWorker to the main thread which i did, now im trying to transfer this arraybuffer value to an ImageData object. Hoping not to make a copy, as my addon is a multi monitor screenshot tool, so eveyr is about 6mb in size.

Thanks Boris!

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Thanks Boris, the actual situation is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1203976

Uh... the code linked from there (<https://github.com/Noitidart/NativeShot/commit/3cac7d649d8795cacb1ac30fa129bedc02a70c3e>) totally doesn't match what you said in comment 0!  Comment 0 has the ImageData and the Uint8ClampedArray being passed to set() coming from the same global, but the commit there has the ImageData being created from aWindow while the Uint8Array is created from whatever window we're running in.  So the set() not only does a copy, it has to take a slow-path [[Get]] for every single array element, because it's working with a cross-global proxy, not an actual typed array.  I can certainly believe this takes a while for several million array elements.  None of that has anything to do with the DOM.  You can see it trivially in a pure JS benchmark:

  <iframe></iframe>
  <script>
  var w = 1920, h = 1280;
  var img = new frames[0].ImageData(w, h);
  var arr = new Uint8Array(w*h*4);
  var start = new Date;
  img.data.set(arr);
  var stop = new Date;
  document.write(stop - start);
  </script>

That takes about 600ms for me.  Doing a same-global set() takes more like 25ms, even with the copy it has to do.

> I'm trying to not make a copy

Then you shouldn't use set().  set() is defined to make a copy.

What you want to do is transfer your ArrayBuffer to aWindow, create a Uint8Array from aWindow with that post-transfer ArrayBuffer, and then create your ImageData from aWindow using the Uint8Array you created.

Resummarizing for the actual performance problem in set() here...

Comment 4

[noitidart]

Ah interesting, I'm not on e10s though, is it still using Proxy?

I used aDOMWindow for the buffer because of a issue i had in the past: https://bugzilla.mozilla.org/show_bug.cgi?id=1140796

Can you please point me to mdn on how i can transfer arraybuffer to window. Right now I open a new window called panel.xul, i create a html:canvas using that document, then i do putImageData from the ImageData i make in the bootstrap.js. If I can just transfer my arraybuffer into that canvas in that window that would be so ideal, if you can please provide tip on that i would really appreciate it.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I'm not on e10s though, is it still using Proxy?

It's not using Proxy.  It's using a cross-global security wrapper.  But yes, that's the case whether e10s is involved or not.

> Can you please point me to mdn on how i can transfer arraybuffer to window

I'm not quite sure, actually.  Bobby?

Comment 6

[noitidart]

Oh also @Boris, how come when i used the constructor method, it was still using aDOMWindow.Uint8 how come that didn't cause freeze?

    aDOMWindow.ImageData(new aDOMWindow.Uint8ClampedArray(colMon[i].screenshot), colMon[i].w, colMon[i].h);

This version does not cause freeze.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It doesn't freeze because it never tries to do anything with the data.  It's not doing a copy.

Now accessing imgData.data[n] on the return value of that constructor would be rather slow, but that's a separate issue.

Comment 8

[noitidart]

Ah so passing the arraybuffer to the constructor causes it to d a transfer/memmove to the ImageData? Is that the only way to transfer. This is maybe question for bobby. Excuse the lots of questions, i ask because i will be updating MDN, so want to get it right.

Comment 9

[noitidart]

This is an article i have, which is very newbie written I need to improve it, if you experts can improve it taht would be awesome: https://developer.mozilla.org/en-US/docs/Mozilla/js-ctypes/Using_js-ctypes/Working_with_ArrayBuffers I couldnt find anywhere on mdn that gives us context with arraybuffers and real applicaitons, its just function method defs.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Ah so passing the arraybuffer to the constructor causes it to d a transfer/memmove to the
> ImageData? 

No.  Passing a Uint8Array object to the ImageData constructor just makes the .data of the ImageData be that object.  So:

  var imgData = new ImageData(arr, w, h);
  alert(arr === imgData.data); // alerts true

You're not using js-ctypes anywhere in here, so the article you cite isn't relevant at all.

Comment 11

[Till Schneidereit [:till]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> > Ah so passing the arraybuffer to the constructor causes it to d a transfer/memmove to the
> > ImageData? 
> 
> No.  Passing a Uint8Array object to the ImageData constructor just makes the
> .data of the ImageData be that object.  So:
> 
>   var imgData = new ImageData(arr, w, h);
>   alert(arr === imgData.data); // alerts true

Bug 907637 comment 6 is probably relevant here: if you create a typed array (or ImageData instance) from an ArrayBuffer from another compartment, you're gonna have a bad time, basically.

Comment 12

[Bobby Holley (:bholley)]

I'm not sure if there's a programmatic way to transfer an ArrayBuffer cross-global. Till or Waldo might know.

However, you _can_ clone it using Cu.cloneInto, after which point all access should be fast.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I'm not sure if there's a programmatic way to transfer an ArrayBuffer cross-global

Well.  postMessage can do it, right?  The problem is that in this case we don't want the target global to be able to intercept the transfer, which with postMessage and its event based mechanism we'd end up doing.

> However, you _can_ clone it using Cu.cloneInto

But that makes a copy, right?

Comment 14

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #13)
> > I'm not sure if there's a programmatic way to transfer an ArrayBuffer cross-global
> 
> Well.  postMessage can do it, right?  The problem is that in this case we
> don't want the target global to be able to intercept the transfer, which
> with postMessage and its event based mechanism we'd end up doing.

Hm, yeah. Might be work adding an explicit transfer API on Cu or similar. Open to hacks to do this without that.

> > However, you _can_ clone it using Cu.cloneInto
> 
> But that makes a copy, right?

Yes, but it's much faster than whatever sort of cross-compartment iteration is causing this operation to register in seconds.

Comment 15

[Till Schneidereit [:till]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> > I'm not sure if there's a programmatic way to transfer an ArrayBuffer cross-global
> 
> Well.  postMessage can do it, right?  The problem is that in this case we
> don't want the target global to be able to intercept the transfer, which
> with postMessage and its event based mechanism we'd end up doing.

Unfortunately postMessage is the only way to do it, yes. Would be nice to have a way to do it from the receiver side, and within the same turn of the event loop.
> 
> > However, you _can_ clone it using Cu.cloneInto
> 
> But that makes a copy, right?

I think the copy might not be all that bad if, at the same time, you also ensure that the source buffer can be GC'd. It's certainly not ideal, but far better than the alternatives, it seems.

https://gist.github.com/lukewagner/2735af7eea411e18cf20 would help with all this, I think. Depends on what it will/would do with foreign-realm objects. Luke, did you consider this case? Would ArrayBuffer.transfer efficiently create a current-realm ArrayBuffer?

Comment 16

[Bobby Holley (:bholley)]

That looks like exactly what I was imagining, and non-proprietary to boot. What do you think luke?

Comment 17

[Till Schneidereit [:till]]

(In reply to Bobby Holley (:bholley) from comment #16)
> That looks like exactly what I was imagining, and non-proprietary to boot.
> What do you think luke?

We'd have to get the JS proposal far enough along in the process[1] to implement it. I don't know what the plan is for that, but this might be yet another reason to push for it.

[1] See https://github.com/tc39/ecma262 and https://tc39.github.io/process-document/

Comment 18

[Luke Wagner [:luke]]

I'm in favor of getting ArrayBuffer.transfer standardized.  I just wrote up that strawman for Brendan and dherman, I don't have a way of actually getting it on the agenda.  I'd ping one of Brendan/dherman directly.

Other engines w/o wrappers might not buy the "it helps us work around FF cross-global perf cliffs" argument, but the argument that everyone should appreciate would be: this is the only way to synchronously release large gobs of buffer memory (which could be used to avoid OOM spikes, e.g.) and the most efficient way to copy a buffer into a larger/smaller buffer.

Comment 19

[noitidart]

By the way is it possible to transfer arrayBuffer's with sendAsyncMessage? I need to get it into a content tab in a friendly way.

Comment 20

[noitidart]

(In reply to noitidart from comment #19)
> By the way is it possible to transfer arrayBuffer's with sendAsyncMessage? I
> need to get it into a content tab in a friendly way.

Reply to self. The answer to my question is not possible. I have to copy between processes, I cant transfer.