Introduce SmallTypedArray IDL type and use it for bufferSubData

Introduce SmallTypedArray IDL type and use it for bufferSubData

This IDL type can be used instead of an ArrayBufferView if the
ArrayBufferView is small (i.e. it doesn't have a materialized buffer).
To work properly, the IDL should declare the same method with an
ArrayBufferView as argument.

Currently, we'll copy the contents to an alloca'd array. Ideally, we'd
pass in the v8::ArrayBufferView, so the underlying implementation can
copy the bytes to the target buffer.

BUG=v8:3996
R=kbr@chromium.org,dslomov@chromium.org,haraken@chromium.org

[jochen (gone - plz use gerrit), 2015-04-13 11:05:28]

ptal

depends on https://codereview.chromium.org/1084513002

[Dmitry Lomov (no reviews), 2015-04-13 11:10:42]

looking good overall, my main comment is to change SmallTypedArray to
SmallArrayBufferView.

[haraken, 2015-04-13 11:13:11]

Another idea would be to create a new IDL extended attribute like
[SharedArrayBufferView], since we want to make DOM attribute/method declarations
as conformant to the spec as possible.

[jochen (gone - plz use gerrit), 2015-04-13 11:46:41]

On 2015/04/13 at 11:13:11, haraken wrote:
> Another idea would be to create a new IDL extended attribute like
[SharedArrayBufferView], since we want to make DOM attribute/method declarations
as conformant to the spec as possible.

is there a way to make the code generator turn a single declaration into two
overloaded methods?

[Jens Widell, 2015-04-13 12:02:38]

On 2015/04/13 11:46:41, jochen wrote:
> On 2015/04/13 at 11:13:11, haraken wrote:
> > Another idea would be to create a new IDL extended attribute like
> [SharedArrayBufferView], since we want to make DOM attribute/method
declarations
> as conformant to the spec as possible.
> 
> is there a way to make the code generator turn a single declaration into two
> overloaded methods?

I think it's a bit unfortunate to involve overloading at all. In the
bufferSubData case there's overloading anyway, of course, but otherwise it would
slightly change behavior to introduce it.

Also, you'd perhaps want to support it for union type conversion as well, which
is essentially like the overload resolution. ArrayBufferView occurs in some
union types already.

But instead of overloading, can't we have an extended attribute that simply
causes ArrayBufferView values to be passed to the implementation as
std::pair<void*, size_t> (always)?

[jochen (gone - plz use gerrit), 2015-04-13 12:08:22]

On 2015/04/13 at 12:02:38, jl wrote:
> On 2015/04/13 11:46:41, jochen wrote:
> > On 2015/04/13 at 11:13:11, haraken wrote:
> > > Another idea would be to create a new IDL extended attribute like
> > [SharedArrayBufferView], since we want to make DOM attribute/method
declarations
> > as conformant to the spec as possible.
> > 
> > is there a way to make the code generator turn a single declaration into two
> > overloaded methods?
> 
> I think it's a bit unfortunate to involve overloading at all. In the
bufferSubData case there's overloading anyway, of course, but otherwise it would
slightly change behavior to introduce it.
> 
> Also, you'd perhaps want to support it for union type conversion as well,
which is essentially like the overload resolution. ArrayBufferView occurs in
some union types already.
> 
> But instead of overloading, can't we have an extended attribute that simply
causes ArrayBufferView values to be passed to the implementation as
std::pair<void*, size_t> (always)?

because we can't in general alloca() enough space on the stack to hold array
buffers.

The ones without buffers are <64 bytes.

I think, in the end, we want to pass in v8::ArrayBufferViews to the blink-core
methods instead of DOMArrayBufferViews and have each method decide whether it
wants to copy out the data or create a DOMArrayBufferView which allows you to
retain the data for longer than the callback.

[Jens Widell, 2015-04-13 13:01:55]

On 2015/04/13 12:08:22, jochen wrote:
> On 2015/04/13 at 12:02:38, jl wrote:
> > But instead of overloading, can't we have an extended attribute that simply
> causes ArrayBufferView values to be passed to the implementation as
> std::pair<void*, size_t> (always)?
> 
> because we can't in general alloca() enough space on the stack to hold array
> buffers.

Ah, I was thinking that in non-small case we could just get a simple pointer to
the data from V8, but I guess that's not the case.

[Ken Russell (switch to Gerrit), 2015-04-14 01:40:19]

Jochen, thank you for putting this together.

On 2015/04/13 13:01:55, Jens Widell wrote:
> On 2015/04/13 12:08:22, jochen wrote:
> > On 2015/04/13 at 12:02:38, jl wrote:
> > > But instead of overloading, can't we have an extended attribute that
simply
> > causes ArrayBufferView values to be passed to the implementation as
> > std::pair<void*, size_t> (always)?
> > 
> > because we can't in general alloca() enough space on the stack to hold array
> > buffers.
> 
> Ah, I was thinking that in non-small case we could just get a simple pointer
to
> the data from V8, but I guess that's not the case.

This should be simple. The bindings can just call
DOMArrayBufferView::baseAddress and byteLength in the non-small case.

On the whole I would prefer the approach haraken also suggested, to add an
extended IDL attribute like [SupportsSmallTypedArrays]. This will keep the IDL
closer to that in the spec. The WebGLRenderingContextBase::bufferSubData
overload taking DOMArrayBufferView* can just be replaced with one taking (const
void*, size_t). The generated binding would either alloca the storage and use
CopyContents, or extract the baseAddress and byteLength from the
DOMArrayBufferView, but always call the same C++ method.

[Ken Russell (switch to Gerrit), 2015-04-14 01:40:26]

https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:497: 'SmallTypedArray':
'{v8_value}->IsArrayBufferView() ? {variable_name} =
std::make_pair(alloca(v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
v8::ArrayBufferView::Cast(*{v8_value})->CopyContents({variable_name}.first,
{variable_name}.second), {variable_name} : std::make_pair(nullptr, 0)',
Is there any way to split this up into multiple expressions to make it more
readable and maintainable?

[Jens Widell, 2015-04-14 09:18:17]

jl@opera.com changed reviewers:
+ jl@opera.com

[Jens Widell, 2015-04-14 09:18:18]

https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:497: 'SmallTypedArray':
'{v8_value}->IsArrayBufferView() ? {variable_name} =
std::make_pair(alloca(v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
v8::ArrayBufferView::Cast(*{v8_value})->CopyContents({variable_name}.first,
{variable_name}.second), {variable_name} : std::make_pair(nullptr, 0)',
On 2015/04/14 01:40:26, Ken Russell wrote:
> Is there any way to split this up into multiple expressions to make it more
> readable and maintainable?

I think we should make a helper function out of it, if we end up with this
approach. We have such helpers (V8XXX::toImpl() typically) for most object
types.

[jochen (gone - plz use gerrit), 2015-04-14 13:35:05]

On 2015/04/14 at 09:18:18, jl wrote:
>
https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
> File Source/bindings/scripts/v8_types.py (right):
> 
>
https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
> Source/bindings/scripts/v8_types.py:497: 'SmallTypedArray':
'{v8_value}->IsArrayBufferView() ? {variable_name} =
std::make_pair(alloca(v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
v8::ArrayBufferView::Cast(*{v8_value})->CopyContents({variable_name}.first,
{variable_name}.second), {variable_name} : std::make_pair(nullptr, 0)',
> On 2015/04/14 01:40:26, Ken Russell wrote:
> > Is there any way to split this up into multiple expressions to make it more
> > readable and maintainable?
> 
> I think we should make a helper function out of it, if we end up with this
approach. We have such helpers (V8XXX::toImpl() typically) for most object
types.

does somebody from the bindings team want to pick this up? :)

[Jens Widell, 2015-04-14 13:50:59]

On 2015/04/14 13:35:05, jochen wrote:
> On 2015/04/14 at 09:18:18, jl wrote:
> >
>
https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
> > File Source/bindings/scripts/v8_types.py (right):
> > 
> >
>
https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
> > Source/bindings/scripts/v8_types.py:497: 'SmallTypedArray':
> '{v8_value}->IsArrayBufferView() ? {variable_name} =
> std::make_pair(alloca(v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
> v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
> v8::ArrayBufferView::Cast(*{v8_value})->CopyContents({variable_name}.first,
> {variable_name}.second), {variable_name} : std::make_pair(nullptr, 0)',
> > On 2015/04/14 01:40:26, Ken Russell wrote:
> > > Is there any way to split this up into multiple expressions to make it
more
> > > readable and maintainable?
> > 
> > I think we should make a helper function out of it, if we end up with this
> approach. We have such helpers (V8XXX::toImpl() typically) for most object
> types.
> 
> does somebody from the bindings team want to pick this up? :)

You mean take over the patch and implement our own damn suggestions? :-)

If so, sure, I can do that.

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

On 2015/04/14 at 13:50:59, jl wrote:
> On 2015/04/14 13:35:05, jochen wrote:
> > On 2015/04/14 at 09:18:18, jl wrote:
> > >
> >
https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
> > > File Source/bindings/scripts/v8_types.py (right):
> > > 
> > >
> >
https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
> > > Source/bindings/scripts/v8_types.py:497: 'SmallTypedArray':
> > '{v8_value}->IsArrayBufferView() ? {variable_name} =
> > std::make_pair(alloca(v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
> > v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
> > v8::ArrayBufferView::Cast(*{v8_value})->CopyContents({variable_name}.first,
> > {variable_name}.second), {variable_name} : std::make_pair(nullptr, 0)',
> > > On 2015/04/14 01:40:26, Ken Russell wrote:
> > > > Is there any way to split this up into multiple expressions to make it
more
> > > > readable and maintainable?
> > > 
> > > I think we should make a helper function out of it, if we end up with this
> > approach. We have such helpers (V8XXX::toImpl() typically) for most object
> > types.
> > 
> > does somebody from the bindings team want to pick this up? :)
> 
> You mean take over the patch and implement our own damn suggestions? :-)
> 
> If so, sure, I can do that.

Great

I think your suggestions are all perfectly fine, it's just that there's also a
lot more that needs to be done on the V8 side of array buffer handling :)

[Ken Russell (switch to Gerrit), 2015-04-15 00:45:06]

On 2015/04/14 15:22:12, jochen wrote:
> On 2015/04/14 at 13:50:59, jl wrote:
> > On 2015/04/14 13:35:05, jochen wrote:
> > > On 2015/04/14 at 09:18:18, jl wrote:
> > > >
> > >
>
https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
> > > > File Source/bindings/scripts/v8_types.py (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1079983002/diff/20001/Source/bindings/scripts...
> > > > Source/bindings/scripts/v8_types.py:497: 'SmallTypedArray':
> > > '{v8_value}->IsArrayBufferView() ? {variable_name} =
> > >
std::make_pair(alloca(v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
> > > v8::ArrayBufferView::Cast(*{v8_value})->ByteLength()),
> > >
v8::ArrayBufferView::Cast(*{v8_value})->CopyContents({variable_name}.first,
> > > {variable_name}.second), {variable_name} : std::make_pair(nullptr, 0)',
> > > > On 2015/04/14 01:40:26, Ken Russell wrote:
> > > > > Is there any way to split this up into multiple expressions to make it
> more
> > > > > readable and maintainable?
> > > > 
> > > > I think we should make a helper function out of it, if we end up with
this
> > > approach. We have such helpers (V8XXX::toImpl() typically) for most object
> > > types.
> > > 
> > > does somebody from the bindings team want to pick this up? :)
> > 
> > You mean take over the patch and implement our own damn suggestions? :-)
> > 
> > If so, sure, I can do that.
> 
> Great
> 
> I think your suggestions are all perfectly fine, it's just that there's also a
> lot more that needs to be done on the V8 side of array buffer handling :)

@jochen, thank you for putting this together and @jl, thanks in advance for
picking this up. Let me know if help is needed from the WebGL side of things.

[jochen (gone - plz use gerrit), 2015-04-15 13:26:07]

closing this CL