Repository navigation
ES2016 compat issue with Buffer subclassing Uint8Array #4701
Description
Activity
The associated V8 bug is https://bugs.chromium.org/p/v8/issues/detail?id=4665
Another option is to call the three-argument constructor for TypedArray from Buffer.prototype.slice, rather than calling subarray. This would side-step the need to override
@@species.I wouldn't recommend using
@@speciesbefore it's shipping in v8Could you please put
@@speciesin backticks so that it does not link to the user? Thanks. :)@jeisinger To clarify, there's no need to merge this patch until
@@speciesis shipping in V8.FWIW, a third possible fix is here: feross/buffer#95 . I think this would be a bit slower than basing it on
@@speciesor calling the three-argument TypedArray constructor, however.Wouldn't a solution be to let the buffer constructor accept
(buffer, byteOffset, length), like the TypedArrays does? That way you don't even need the.setPrototypeOfcall to coerce the returnedUint8Arrayinto aBuffer.@LinusU Belated, but a PR is on it's way to allow Buffers to accept an ArrayBuffer with byteoffset and length.
That way you don't even need the .setPrototypeOf call to coerce the returned Uint8Array into a Buffer.
Can you expand on this?
Once you upgrade to V8 5.0, you can take advantage of ES2015's built-in support for TypedArray subclassing. We have a web compat workaround which means that subarray will return instances of a built-in TypedArray type, but generally you can just make an ES2015-style class declaration which extends
Uint8Arrayand calls the super constructor. I don't know how the performance compares today on V8, but on some engines like SpiderMonkey, dynamic proto chain modifications carry a signficant performance penalty, and subclassing based on ES2015 classes is definitely a style which we want to encourage in the future.@trevnorris If buffer is declared as
class Buffer extends Uint8Array {}thenlet b = new Buffer(8); b.subarray(0, 4)will return a buffer.Currently, it returns a
Uint8Array, which we then usesetPrototypeOfon.Does it clear things up? I can try and make some better examples otherwise :)
39 remaining items
The Buffer constructor is one place we're already hurting.
Sure, that's exactly what I was fixing in previous PR and want to be sure it won't regress again.
almost, you would want to SetPrototypeOf to
new.target.prototypeto make sure that Buffer subclass instances have the correct prototype.But, does using SetPrototypeOf have any deoptimizing impact in V8 or other engines? If it does, I would hope that Reflect.construct did not. (It's the difference between supply the prototype before allocation or changing it after allocation)
Regard, when you think you have a complete subclassable JS implementation for Buffer point me at it and I'll review it for you.
almost, you would want to SetPrototypeOf to
new.target.prototypeto make sure that Buffer subclass instances have the correct prototype.So you're saying we'd also want to do both?:
buf->SetPrototypeOf(context, buf_prototype); buf->GetPrototype()->SetPrototypeOf(context, buf_prototype);
But, does using SetPrototypeOf have any deoptimizing impact in V8 or other engines?
Whether it does or not I'm not sure there's a way around it (that doesn't require hacking the ArrayBuffer allocator) since the data is coming in through the I/O event as a
char*on the native side.OK, I'm loosing track of what exactly you are trying to accomplish. I thought you were trying to define a version of Buffer that could be subclassed, similar to what I showed in #4701 (comment) . Whether Buffer is actually implemented in C++ rather than JS doesn't change the requirements.
Assuming that
SubBuffer extends Bufferthen an object created vianew SubBuffer()must have its [[Prototype]] set toSubBuffer.prototypeas part of its allocation process. This is complicated by that fact that the allocation takes place within theBufferconstructor (or perhaps theUint8Arrayconstructor) rather than within theSubBufferconstructor. To make that work, the actual constructor thatnewwas applied to must be passed along to the superclass constructor that does the allocation. In JS code, that is the role played by the thenew.targetmeta property. It is used to communicated the subclass being allocated up to the superclass constructor that is doing the allocation such that [[Prototype]] can be set appropriately.This must be the case regardless of whether Buffer is implemented in JS code or in C++. However, if it is implemented in C++ then the V8 C++ API must provide something that is equivalent to
new.targetandReflect.construct. I'm not very familiar with the V8 C++ APIs and don't know what they may have recently done to that API to support subclassing. But, "ES6 subclassable builtins" essentially require the equivalent of these mechanisms so I assume V8 has added something in the API to support them.So you're saying we'd also want to do both?:
No, if we are talking about
new SubBuffer()initiated allocation you would never set the prototype tobuf_prototype, it needs to be set to SubBuffer.prototype, however that value is made available to you.Whether it does or not I'm not sure there's a way around it (that doesn't require hacking the ArrayBuffer allocator) since the data is coming in through the I/O event as a char* on the native side.
That is essentially the job of Reflect.construct or an equivalent C++ level API. V8 should be providing this.
I probably need more details of the usage, but I'm not sure why ArrayBuffer would be an issue? Aren't we're talking about SubBuffer being a subclass of Buffer which is a subclass of Uint8Array? They would all default to using ArrayBuffer as their backing store. (Of course, ArrayBuffer is (according to ES6) also supposed to be subclassable if that should prove useful).
I'm probably missing something about what you need to accomplish so feel free to clarify.
@allenwb Basically the native code looks like so:
Local<ArrayBuffer> ab = ArrayBuffer::New( isolate, data, length, ArrayBufferCreationMode::kInternalized); Local<Uint8Array> ui = Uint8Array::New(ab, 0, length); Maybe<bool> mb = ui->SetPrototype(context, buffer_prototype_object);Where
buffer_prototype_objectisBuffer.prototype. What I'm wondering is if the new JS implementation will require changes to the native code.Nevermind my comment about leveraging the
v8::ArrayBuffer::Allocator. It's a hack to allow using the JS API to create the new Buffer instance, but still passing in the requiredchar*.@trevnorris the last line needs to pass the prototype of the Buffer subclass as the second argument to SetPrototype. But I don't know whether or how V8 makes that value available to you via its C++ API. It should be available because it is required to support the Reflect.construct function (and that is really what is going on here).
So, this sounds like a question for @littledan : Has V8 make Uint8Array subclassable yet, as required by ES6? How is the newTarget parameter of [[Construct]] exposed in the V8 C++ API such that
super(or Reflect.construct) calls from subclass constructors will work as specified when the superclass is implemented in C++?@trevnorris I guess we don't really need that even now; instead, we can construct
FastBufferfrom C++ land and pass thatArrayBufferas an argument.@RReverser I would be interested in seeing benchmark results for that.
@trevnorris Well I've made changes for that one locally, but is there any specific benchmark code for those C++
Newmethods or should I just come up with some?@RReverser I was thinking of yanking the specific operations happening in
Buffer::New(), though that probably won't make a big difference. We don't have any benchmarks for native modules in core (future improvement) so will need to make some new ones.If you're a constructor implemented in C++ (as a
FunctionCallback) being called, thennew.targetis exposed to you throughFunctionCallbackInfo::NewTarget. However, something implementing a subclass of a TypedArray constructor in C++ will run into other issues when trying to call its super constructor:As far as I can tell, the V8 API currently only exposes
new.targetthroughReflect.constructand the individualTypedArrayconstructors, which you can get to only through property access. Maybe we should expose a convenience API forReflect.constructmore directly in the V8 API to make it easier to call out to it.Calling out to the actual TypedArray constructor this way will be correct, but might be slower in practice for now in some cases, as it is implemented in JavaScript whereas the constructor path exposed by the API is entirely C++; the transition may have some performance cost. I have not benchmarked it yet. I am looking into rewriting this path in C++ for code health reasons, and that change may have knock-on benefits for the speed of this sort of use case.
cc @jochen
Does this need to remain open?
In the ES2016 draft specification, TypedArray methods like
%TypedArray%.prototype.subarray()call out to a constructor for the resultbased on the receiver. Ordinarily, the constructor is
instance.constructor,but subclasses can override this using the
Symbol.speciesproperty on theconstructor.
Buffer.prototype.slicecalls out to%TypedArray%.prototype.subarray, whichcalls this calculated constructor with three arguments. The argument pattern
doesn't correspond to a constructor for Buffer, so without setting
Symbol.species appropriately, the wrong kind of result is created.
This issue came up because I'm working on implementing spec-compliant
subclassable TypedArrays in V8. feross helpfully reported that I broke his
buffer library for the browser. I wrote a couple patches to update both Node
and the browser buffer library for ES2016 TypedArray subclassing:
littledan@834338e
littledan/buffer@2e60129
For now (Chrome 49/V8 4.9), I'm keeping the legacy behavior that
%TypedArray%.prototype.subarraycalls out to a base class constructorlike Uint8Array. However, in the future, I'd like to ship proper TypedArray subclassing,
which would benefit from a patch like this. What do you think?