IDL: Add [FlexibleArrayBufferView] and use for bufferSubData

IDL: Add [FlexibleArrayBufferView] and use for bufferSubData

The [FlexibleArrayBufferView] extended attribute, when used on a method
argument of type ArrayBufferView, causes the value to be processed and
passed to the implementation as the new C++ type FlexibleArrayBufferView
instead of as DOMArrayBufferView*.

A FlexibleArrayBufferView object will either contain a pointer to a
DOMArrayBufferView, or, if the array was small (without a materialized
buffer), a stack allocated buffer containing the contents of the array.

BUG=v8:3996

[Jens Widell, 2015-04-15 11:49:33]

jl@opera.com changed reviewers:
+ dslomov@chromium.org, haraken@chromium.org, jochen@chromium.org,
kbr@chromium.org

[Jens Widell, 2015-04-15 11:49:34]

PTAL

This is a reworked version of https://codereview.chromium.org/1079983002/,
achieving the same thing effectively. Using an extended attribute instead means
we can leave the basic IDL spec compliant, which we like. OTOH, it currently has
the side-effect that this won't work for members of unions, since extended
attributes can't be used there.

(Try bots are failing because https://codereview.chromium.org/1086113002/ hasn't
landed yet.)

[jochen (gone - plz use gerrit), 2015-04-15 12:59:50]

in the end, the data is copied here to gl:
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...

Ideally, we'd not alloca() the data, but somehow pass the ArrayBufferView down
there, so we can CopyContents directly into the buffer.

Why not pass a v8::Local<v8::ArrayBufferView> to bufferSubData instead of a
FlexibleArrayBufferView? The latter is a rather dangerous class as the alloca'd
memory will be automatically freed when the stack is unwound, so if somebody
stores the class on heap, it'll point to a random location on the stack..

[Ken Russell (switch to Gerrit), 2015-04-16 05:56:52]

On 2015/04/15 12:59:50, jochen wrote:
> in the end, the data is copied here to gl:
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...
> 
> Ideally, we'd not alloca() the data, but somehow pass the ArrayBufferView down
> there, so we can CopyContents directly into the buffer.

That's a pretty heroic optimization that would require a lot of abstraction
barriers to be broken through. Let's start with this and see what it brings.

> Why not pass a v8::Local<v8::ArrayBufferView> to bufferSubData instead of a
> FlexibleArrayBufferView? The latter is a rather dangerous class as the
alloca'd
> memory will be automatically freed when the stack is unwound, so if somebody
> stores the class on heap, it'll point to a random location on the stack..

At one point there was an explicit goal to not reference V8 types from Blink's
common code, and I think that's still a good idea.

I like the FlexibleArrayBufferView concept. Let's put in some big warnings in
the header about not referencing the storage after the function returns.

Should we benchmark this and see what it brings before committing it?

Brandon or Mo can continue this review; I'll be OOO the next couple of days.

[Jens Widell, 2015-04-16 08:51:15]

On 2015/04/16 05:56:52, Ken Russell (OOO until 4-21) wrote:
> On 2015/04/15 12:59:50, jochen wrote:
> > in the end, the data is copied here to gl:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...
> > 
> > Ideally, we'd not alloca() the data, but somehow pass the ArrayBufferView
down
> > there, so we can CopyContents directly into the buffer.
> 
> That's a pretty heroic optimization that would require a lot of abstraction
> barriers to be broken through. Let's start with this and see what it brings.
> 
> > Why not pass a v8::Local<v8::ArrayBufferView> to bufferSubData instead of a
> > FlexibleArrayBufferView? The latter is a rather dangerous class as the
> alloca'd
> > memory will be automatically freed when the stack is unwound, so if somebody
> > stores the class on heap, it'll point to a random location on the stack..
> 
> At one point there was an explicit goal to not reference V8 types from Blink's
> common code, and I think that's still a good idea.
> 
> I like the FlexibleArrayBufferView concept. Let's put in some big warnings in
> the header about not referencing the storage after the function returns.

I've added a warning in FlexibleArrayBufferView.h in PS#3.

> Should we benchmark this and see what it brings before committing it?

I ran the oortonline.gl benchmark (once), and saw no difference in performance
from the patch. I micro-benchmark would probably be good, though, to try to
isolate this particular change's impact, if there is one.

[jochen (gone - plz use gerrit), 2015-04-16 14:39:51]

On 2015/04/16 at 08:51:15, jl wrote:
> On 2015/04/16 05:56:52, Ken Russell (OOO until 4-21) wrote:
> > On 2015/04/15 12:59:50, jochen wrote:
> > > in the end, the data is copied here to gl:
> > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...
> > > 
> > > Ideally, we'd not alloca() the data, but somehow pass the ArrayBufferView
down
> > > there, so we can CopyContents directly into the buffer.
> > 
> > That's a pretty heroic optimization that would require a lot of abstraction
> > barriers to be broken through. Let's start with this and see what it brings.
> > 
> > > Why not pass a v8::Local<v8::ArrayBufferView> to bufferSubData instead of
a
> > > FlexibleArrayBufferView? The latter is a rather dangerous class as the
> > alloca'd
> > > memory will be automatically freed when the stack is unwound, so if
somebody
> > > stores the class on heap, it'll point to a random location on the stack..
> > 
> > At one point there was an explicit goal to not reference V8 types from
Blink's
> > common code, and I think that's still a good idea.
> > 
> > I like the FlexibleArrayBufferView concept. Let's put in some big warnings
in
> > the header about not referencing the storage after the function returns.
> 
> I've added a warning in FlexibleArrayBufferView.h in PS#3.
> 
> > Should we benchmark this and see what it brings before committing it?
> 
> I ran the oortonline.gl benchmark (once), and saw no difference in performance
from the patch. I micro-benchmark would probably be good, though, to try to
isolate this particular change's impact, if there is one.

which is more or less expected, as we still do the same amount of allocations
due to the alloca, no?

[Jens Widell, 2015-04-16 15:15:37]

On 2015/04/16 14:39:51, jochen wrote:
> On 2015/04/16 at 08:51:15, jl wrote:
> > On 2015/04/16 05:56:52, Ken Russell (OOO until 4-21) wrote:
> > > On 2015/04/15 12:59:50, jochen wrote:
> > > > in the end, the data is copied here to gl:
> > > >
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...
> > > > 
> > > > Ideally, we'd not alloca() the data, but somehow pass the
ArrayBufferView
> down
> > > > there, so we can CopyContents directly into the buffer.
> > > 
> > > That's a pretty heroic optimization that would require a lot of
abstraction
> > > barriers to be broken through. Let's start with this and see what it
brings.
> > > 
> > > > Why not pass a v8::Local<v8::ArrayBufferView> to bufferSubData instead
of
> a
> > > > FlexibleArrayBufferView? The latter is a rather dangerous class as the
> > > alloca'd
> > > > memory will be automatically freed when the stack is unwound, so if
> somebody
> > > > stores the class on heap, it'll point to a random location on the
stack..
> > > 
> > > At one point there was an explicit goal to not reference V8 types from
> Blink's
> > > common code, and I think that's still a good idea.
> > > 
> > > I like the FlexibleArrayBufferView concept. Let's put in some big warnings
> in
> > > the header about not referencing the storage after the function returns.
> > 
> > I've added a warning in FlexibleArrayBufferView.h in PS#3.
> > 
> > > Should we benchmark this and see what it brings before committing it?
> > 
> > I ran the oortonline.gl benchmark (once), and saw no difference in
performance
> from the patch. I micro-benchmark would probably be good, though, to try to
> isolate this particular change's impact, if there is one.
> 
> which is more or less expected, as we still do the same amount of allocations
> due to the alloca, no?

Yeah. I guess we might avoid ever allocating DOMArrayBufferView/DOMArrayBuffer
objects, but the data would be allocated and copied the same number of times.

[jochen (gone - plz use gerrit), 2015-04-22 14:10:29]

what about changing this so the Flexible thing has the following methods:

bool HasBuffer()
size_t Length()
void CopyOut(void*, size_t byteLen)
DOMArrayBufferView* GetBuffer()

internally, it keeps a Local<ArrayBufferView> and it's non-copyable

Then bufferSubData() has to do the alloca() if it wants to, however, if we
decide to change the gl thing to avoid the alloca, we don't have to change
FlexibleArrayBufferView (also, it's then safe to use)

kbr@ we could change the gl API to return a buffer to copy into, then we don't
need to pass the ArrayBufferView down, wdyt?

[Jens Widell, 2015-04-22 14:14:29]

On 2015/04/22 14:10:29, jochen wrote:
> what about changing this so the Flexible thing has the following methods:
> 
> bool HasBuffer()
> size_t Length()
> void CopyOut(void*, size_t byteLen)
> DOMArrayBufferView* GetBuffer()
> 
> internally, it keeps a Local<ArrayBufferView> and it's non-copyable

Sounds good to me. Probably it ought to move into bindings/core/v8/ then as
well.

> Then bufferSubData() has to do the alloca() if it wants to, however, if we
> decide to change the gl thing to avoid the alloca, we don't have to change
> FlexibleArrayBufferView (also, it's then safe to use)
> 
> kbr@ we could change the gl API to return a buffer to copy into, then we don't
> need to pass the ArrayBufferView down, wdyt?

[Ken Russell (switch to Gerrit), 2015-04-23 23:14:30]

On 2015/04/22 14:10:29, jochen wrote:
> what about changing this so the Flexible thing has the following methods:
> 
> bool HasBuffer()
> size_t Length()
> void CopyOut(void*, size_t byteLen)
> DOMArrayBufferView* GetBuffer()
> 
> internally, it keeps a Local<ArrayBufferView> and it's non-copyable
> 
> Then bufferSubData() has to do the alloca() if it wants to, however, if we
> decide to change the gl thing to avoid the alloca, we don't have to change
> FlexibleArrayBufferView (also, it's then safe to use)

That means every user of FlexibleArrayBufferView has to have code to alloca and
copy out the contents. I think it is better if FlexibleArrayBufferView
encapsulates these details.

> kbr@ we could change the gl API to return a buffer to copy into, then we don't
> need to pass the ArrayBufferView down, wdyt?

I don't see how that would work. bufferSubData would then be split up into two
calls? One to get the destination buffer, and one to actually submit it? That's
overly complicated. I like the current structure better.

[jochen (gone - plz use gerrit), 2015-04-24 14:02:30]

On 2015/04/23 at 23:14:30, kbr wrote:
> On 2015/04/22 14:10:29, jochen wrote:
> > what about changing this so the Flexible thing has the following methods:
> > 
> > bool HasBuffer()
> > size_t Length()
> > void CopyOut(void*, size_t byteLen)
> > DOMArrayBufferView* GetBuffer()
> > 
> > internally, it keeps a Local<ArrayBufferView> and it's non-copyable
> > 
> > Then bufferSubData() has to do the alloca() if it wants to, however, if we
> > decide to change the gl thing to avoid the alloca, we don't have to change
> > FlexibleArrayBufferView (also, it's then safe to use)
> 
> That means every user of FlexibleArrayBufferView has to have code to alloca
and copy out the contents. I think it is better if FlexibleArrayBufferView
encapsulates these details.

as long as the users alloca() using a FlexibleArrayBufferView is pointless - it
doesn't save anything.

> 
> > kbr@ we could change the gl API to return a buffer to copy into, then we
don't
> > need to pass the ArrayBufferView down, wdyt?
> 
> I don't see how that would work. bufferSubData would then be split up into two
calls? One to get the destination buffer, and one to actually submit it? That's
overly complicated. I like the current structure better.

[jochen (gone - plz use gerrit), 2015-04-24 14:02:31]

On 2015/04/23 at 23:14:30, kbr wrote:
> On 2015/04/22 14:10:29, jochen wrote:
> > what about changing this so the Flexible thing has the following methods:
> > 
> > bool HasBuffer()
> > size_t Length()
> > void CopyOut(void*, size_t byteLen)
> > DOMArrayBufferView* GetBuffer()
> > 
> > internally, it keeps a Local<ArrayBufferView> and it's non-copyable
> > 
> > Then bufferSubData() has to do the alloca() if it wants to, however, if we
> > decide to change the gl thing to avoid the alloca, we don't have to change
> > FlexibleArrayBufferView (also, it's then safe to use)
> 
> That means every user of FlexibleArrayBufferView has to have code to alloca
and copy out the contents. I think it is better if FlexibleArrayBufferView
encapsulates these details.

as long as the users alloca() using a FlexibleArrayBufferView is pointless - it
doesn't save anything.

> 
> > kbr@ we could change the gl API to return a buffer to copy into, then we
don't
> > need to pass the ArrayBufferView down, wdyt?
> 
> I don't see how that would work. bufferSubData would then be split up into two
calls? One to get the destination buffer, and one to actually submit it? That's
overly complicated. I like the current structure better.

[Ken Russell (switch to Gerrit), 2015-04-24 21:46:26]

On 2015/04/24 14:02:31, jochen wrote:
> On 2015/04/23 at 23:14:30, kbr wrote:
> > On 2015/04/22 14:10:29, jochen wrote:
> > > what about changing this so the Flexible thing has the following methods:
> > > 
> > > bool HasBuffer()
> > > size_t Length()
> > > void CopyOut(void*, size_t byteLen)
> > > DOMArrayBufferView* GetBuffer()
> > > 
> > > internally, it keeps a Local<ArrayBufferView> and it's non-copyable
> > > 
> > > Then bufferSubData() has to do the alloca() if it wants to, however, if we
> > > decide to change the gl thing to avoid the alloca, we don't have to change
> > > FlexibleArrayBufferView (also, it's then safe to use)
> > 
> > That means every user of FlexibleArrayBufferView has to have code to alloca
> and copy out the contents. I think it is better if FlexibleArrayBufferView
> encapsulates these details.
> 
> as long as the users alloca() using a FlexibleArrayBufferView is pointless -
it
> doesn't save anything.

I like the current structure of the code and would not like to see alloca()
calls being made outside the bindings or FlexibleArrayBufferView class. I also
like the fact that WebGLRenderingContextBase only had to be changed trivially to
support small array buffers.

The current patch set LGTM.


> > 
> > > kbr@ we could change the gl API to return a buffer to copy into, then we
> don't
> > > need to pass the ArrayBufferView down, wdyt?
> > 
> > I don't see how that would work. bufferSubData would then be split up into
two
> calls? One to get the destination buffer, and one to actually submit it?
That's
> overly complicated. I like the current structure better.