Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2735563002:
Migrate %TypedArray%.prototype.fill to C++ (Closed)
DescriptionMigrate %TypedArray%.prototype.fill to C++
BUG=v8:5929
R=adamk@chromium.org,bmeurer@chromium.org
Review-Url: https://codereview.chromium.org/2735563002
Cr-Commit-Position: refs/heads/master@{#43934}
Committed: https://chromium.googlesource.com/v8/v8/+/cb903e317338798fec3a43fea2ec285d3626c333
Patch Set 1 #
Total comments: 6
Patch Set 2 : minor change #
Total comments: 1
Patch Set 3 : Use DataPtr #Patch Set 4 : fast/slow path draft #
Total comments: 1
Patch Set 5 : Implement ElementAccessor #
Total comments: 12
Patch Set 6 : fix nits #Patch Set 7 : typo #
Total comments: 7
Patch Set 8 : Makes compiler happy #
Total comments: 1
Patch Set 9 : typo #
Total comments: 6
Patch Set 10 : fix more nits #Patch Set 11 : format 80 cols #
Total comments: 12
Patch Set 12 : rm template #Patch Set 13 : rm template 2 #Patch Set 14 : fix fp and no args bugs #
Total comments: 2
Patch Set 15 : Update test #
Total comments: 4
Patch Set 16 : Use existing helper #Patch Set 17 : rm unused kMethodName #Patch Set 18 : format #Patch Set 19 : Add WasNeutered checks again #
Total comments: 8
Patch Set 20 : Use BackingStore::from_{int,double} #Patch Set 21 : Use BackingStore::from_{int,double} #Patch Set 22 : Merge branch 'master' into fill #
Total comments: 2
Messages
Total messages: 76 (23 generated)
PTAL https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... src/builtins/builtins-typedarray.cc:487: } Casting integer to and from double might cause loss of precision?
Description was changed from ========== Migrate %TypedArray%.prototype.fill to C++ WIP BUG=v8:5929 R=adamk@chromium.org,bmeurer@chromium.org ========== to ========== Migrate %TypedArray%.prototype.fill to C++ WIP BUG=v8:5929 R=adamk@chromium.org,bmeurer@chromium.org ==========
bmeurer@chromium.org changed reviewers: + caitp@chromium.org
I made a mistake in previous benchmark. See updated benchmark and result at crbug.com/v8/5929#c15
Description was changed from ========== Migrate %TypedArray%.prototype.fill to C++ WIP BUG=v8:5929 R=adamk@chromium.org,bmeurer@chromium.org ========== to ========== Migrate %TypedArray%.prototype.fill to C++ BUG=v8:5929 R=adamk@chromium.org,bmeurer@chromium.org ==========
bmeurer@chromium.org changed reviewers: + cbruni@chromium.org
bmeurer@chromium.org changed required reviewers: + cbruni@chromium.org
Thanks for the patch. I like the consistent, good performance :-) Maybe we should introduce Fill in the ElementsAccessor and use that (also porting Array.p.fill)? Adding cbruni@ to this one.
On 2017/03/05 17:52:24, Benedikt Meurer wrote: > Thanks for the patch. I like the consistent, good performance :-) > > Maybe we should introduce Fill in the ElementsAccessor and use that (also > porting Array.p.fill)? Adding cbruni@ to this one. I am guessing I should do something like [1] and [2] combined? [1]: https://codereview.chromium.org/2732823002/ [2]: https://github.com/v8/v8/commit/0c76b0ae850027006d5ec0d92449e449d996d3bb
On 2017/03/06 00:05:16, rongjie wrote: > On 2017/03/05 17:52:24, Benedikt Meurer wrote: > > Thanks for the patch. I like the consistent, good performance :-) > > > > Maybe we should introduce Fill in the ElementsAccessor and use that (also > > porting Array.p.fill)? Adding cbruni@ to this one. > > I am guessing I should do something like [1] and [2] combined? > > [1]: https://codereview.chromium.org/2732823002/ > [2]: https://github.com/v8/v8/commit/0c76b0ae850027006d5ec0d92449e449d996d3bb That'd be my ideal solution. But let cbruni@ comment on this first.
first round of comments. https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... src/builtins/builtins-typedarray.cc:467: if (V8_UNLIKELY(array->WasNeutered())) return *array; This check has to happen after all the argument conversions are finished! If you follow the ToInteger spec steps (this is really well hidden :P) you see that it can have side-effects and for instance neuter the array. https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... src/builtins/builtins-typedarray.cc:472: double fp = 0.0; nit: please use proper variable names, fp is kind of an overloaded term ;) (And I know that the spec likes to use single character variable names :/) https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... src/builtins/builtins-typedarray.cc:477: isolate, num, Object::ToNumber(args.at<Object>(1))); You cannot simply do a ToNumber on the value input parameter. Sadly the spec is written in weird way here, forcing us to to a ToNumber on every! loop iteration. - https://tc39.github.io/ecma262/#sec-array.prototype.fill which calls Set(...) - https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-set-p-v-re... - Which ends up here: https://tc39.github.io/ecma262/#sec-integerindexedelementset where the step 3 is ToNumber So you have to probably bail out if the value is not a primitive value (IsNumber probably good enough). In all other cases ToNumber has a side-effect via ToPrimitive which could call the @@toPrimitive method *sigh* :). My suggestion, implement a runtime function which does the very slow spec conversion steps.
Also, I agree on implementing this in the ElementsAccessor might be cleaner. Feel free to go incremental first. Not much should change since all the arguments conversion has to happen in the builtin first.
Quite confused here, please check my understanding: 1. Call builtin func that check isNumber(value). 2. If yes, call another func implemented with ElementsAccessor to fill (fast). 3. Otherwise call runtime func which involves executing IntegerIndexedElementSet for every element (slow). Not sure what is "bail out" and how to implement it.
https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... src/builtins/builtins-typedarray.cc:467: if (V8_UNLIKELY(array->WasNeutered())) return *array; On 2017/03/06 09:13:44, Camillo Bruni wrote: > This check has to happen after all the argument conversions are finished! > If you follow the ToInteger spec steps (this is really well hidden :P) you see > that it can have side-effects and for instance neuter the array. Done. https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... src/builtins/builtins-typedarray.cc:472: double fp = 0.0; On 2017/03/06 09:13:44, Camillo Bruni wrote: > nit: please use proper variable names, fp is kind of an overloaded term ;) (And > I know that the spec likes to use single character variable names :/) Done.
On 2017/03/06 at 10:31:12, loorongjie wrote:
> Quite confused here, please check my understanding:
>
> 1. Call builtin func that check isNumber(value).
> 2. If yes, call another func implemented with ElementsAccessor to fill (fast).
> 3. Otherwise call runtime func which involves executing
IntegerIndexedElementSet for every element (slow).
>
> Not sure what is "bail out" and how to implement it.
Correct, your plan is what I had in mind :)
bail-out as in no longer following the fast path.
let p = new Proxy({}, {get(t,k) { console.log(k); return Reflect.get(t, k) }});
new Uint16Array(2).fill(p);
>> Symbol(Symbol.toPrimitive)
>> valueOf
>> toString
>> Symbol(Symbol.toStringTag)
>> Symbol(Symbol.toPrimitive)
>> valueOf
>> toString
>> Symbol(Symbol.toStringTag)
Your CL changes this behavior and only does the ToPrimitive conversion once! (it
has to call it on every loop iteration of the fill step)
If there isn't already a test262 test failing, please convert my snippet into a
full test.
cwhan.tunz@gmail.com changed reviewers: + cwhan.tunz@gmail.com
https://codereview.chromium.org/2735563002/diff/20001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2735563002/diff/20001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:516: static_cast<ctype*>(array->GetBuffer()->backing_store()); \ You should consider byte_offset for backing_store.
On 2017/03/08 08:18:30, Choongwoo Han wrote: > https://codereview.chromium.org/2735563002/diff/20001/src/builtins/builtins-t... > File src/builtins/builtins-typedarray.cc (right): > > https://codereview.chromium.org/2735563002/diff/20001/src/builtins/builtins-t... > src/builtins/builtins-typedarray.cc:516: > static_cast<ctype*>(array->GetBuffer()->backing_store()); \ > You should consider byte_offset for backing_store. Done! Still trying to figure out how to start implementing what cbruni@ suggested, but I can't understand how elements.cc works.
Patchset #4 (id:60001) has been deleted
cbruni@ cwhan.tunz@ PTAL? For obj_value->IsNumber() == false case: Since I created k as C type variable, there isn't too much thing to do for [[Set]] ( P, V, Receiver ), IntegerIndexedElementSet ( O, index, value ) except throw TypeError if detached. Do I really have to keep casting it between `String`/`Number`/`Integer`? I don't think there is side-effect.
On 2017/03/09 at 04:26:52, loorongjie wrote: > cbruni@ cwhan.tunz@ PTAL? > > For obj_value->IsNumber() == false case: > > Since I created k as C type variable, there isn't too much thing to do for [[Set]] ( P, V, Receiver ), IntegerIndexedElementSet ( O, index, value ) except throw TypeError if detached. Do I really have to keep casting it between `String`/`Number`/`Integer`? I don't think there is side-effect. For k you do the right thing. k is explicitly an integer from the spec. Converting from Number => String has no side-effects, also we avoid that as much as possible internally and use the integer representation to access things (you see this pattern in the ElementsAccessor).
https://codereview.chromium.org/2735563002/diff/80001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2735563002/diff/80001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:593: Could you put the following code inside the ElementsAccessor? You can follow the path which ends up calling IndexOfValueSlowPath. The final call should like as follows: array->GetElementsAccessor()->Fill(obj_value, start, end) Inernally in ElementsAccessor you will have all implementations for fill for different ElementsKind. You could even put the default implementation from above on the ElementsAccessor. In the slow case when !obj_value->IsNumber() you fall back to FillSlowPath for typed arrays.
next comment round. Looks good so far, some more nits, but we're almost done. https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:484: if (V8_UNLIKELY(array->WasNeutered())) { Can you add a "TODO(caitp,cbruni): throw on neutered array" and just "return *array" This is a recent spec change that hasn't been implemented yet. https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:2854: if (obj_value->IsNumber()) { nit: I prefer early returns and non-indented code :P if (!obj_value->IsNumber) { return FillNumberSlowPath(isolate, array, obj_value, start, end): } https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:2855: double value = 0.0; ctype value = 0; This way you can avoid an unnecessary double cast in the Smi case for integer arrays, hope this works :) https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:2858: value = Smi::cast(*obj_value)->value(); value = static_cast<ctype>(Smi::cast(...)); https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:2861: value = HeapNumber::cast(*obj_value)->value(); same here.. https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:2871: std::fill(data + start, data + end, cast_value); Please add DCHECKS here as well for the range parameters.
https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:484: if (V8_UNLIKELY(array->WasNeutered())) { On 2017/03/13 08:12:58, Camillo Bruni wrote: > Can you add a "TODO(caitp,cbruni): throw on neutered array" and just "return > *array" > This is a recent spec change that hasn't been implemented yet. Done. https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:2854: if (obj_value->IsNumber()) { On 2017/03/13 08:12:58, Camillo Bruni wrote: > nit: I prefer early returns and non-indented code :P > if (!obj_value->IsNumber) { > return FillNumberSlowPath(isolate, array, obj_value, start, end): > } Done. https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:2855: double value = 0.0; On 2017/03/13 08:12:58, Camillo Bruni wrote: > ctype value = 0; > This way you can avoid an unnecessary double cast in the Smi case for integer > arrays, hope this works :) Done. https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:2858: value = Smi::cast(*obj_value)->value(); On 2017/03/13 08:12:58, Camillo Bruni wrote: > value = static_cast<ctype>(Smi::cast(...)); Done. https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:2861: value = HeapNumber::cast(*obj_value)->value(); On 2017/03/13 08:12:58, Camillo Bruni wrote: > same here.. Done. https://codereview.chromium.org/2735563002/diff/100001/src/elements.cc#newcod... src/elements.cc:2871: std::fill(data + start, data + end, cast_value); On 2017/03/13 08:12:58, Camillo Bruni wrote: > Please add DCHECKS here as well for the range parameters. To ensure array->length_value() is not executed in release build, I fit it in DCHECK.
sorry, some more comments. https://codereview.chromium.org/2735563002/diff/140001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/140001/src/elements.cc#newcod... src/elements.cc:483: // TODO(caitp,cbruni): throw on neutered array and just return *array drop "and just return *array" https://codereview.chromium.org/2735563002/diff/140001/src/elements.cc#newcod... src/elements.cc:488: kMethodName))); sorry, my comment wasn't clear enough :P, we don't throw for now (see other places with checks for WasNeutered)... return *array https://codereview.chromium.org/2735563002/diff/140001/src/elements.cc#newcod... src/elements.cc:2877: FixedTypedArrayBase::cast(array->elements())); Another nit: you can avoid a handle allocation here since there is no gc going to happen. DisallowHeapAllocation no_gc; BackingStore* elements = BackingStore::cast(receiver->elements()); https://codereview.chromium.org/2735563002/diff/140001/test/mjsunit/es6/typed... File test/mjsunit/es6/typedarray-fill.js (right): https://codereview.chromium.org/2735563002/diff/140001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-fill.js:36: assertArrayEquals(new constructor([0, 0, 0, 0, 0]).fill(8, 0, -Infinity), [0, 0, 0, 0, 0]); Please add my Proxy test case as well, to ensure we do ToNumber on ever iteration step.
https://codereview.chromium.org/2735563002/diff/140001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/140001/src/elements.cc#newcod... src/elements.cc:483: // TODO(caitp,cbruni): throw on neutered array and just return *array On 2017/03/13 09:02:30, Camillo Bruni wrote: > drop "and just return *array" Done. https://codereview.chromium.org/2735563002/diff/140001/src/elements.cc#newcod... src/elements.cc:488: kMethodName))); On 2017/03/13 09:02:30, Camillo Bruni wrote: > sorry, my comment wasn't clear enough :P, we don't throw for now (see other > places with checks for WasNeutered)... > > return *array Done. https://codereview.chromium.org/2735563002/diff/140001/src/elements.cc#newcod... src/elements.cc:2877: FixedTypedArrayBase::cast(array->elements())); On 2017/03/13 09:02:30, Camillo Bruni wrote: > Another nit: you can avoid a handle allocation here since there is no gc going > to happen. > > DisallowHeapAllocation no_gc; > BackingStore* elements = BackingStore::cast(receiver->elements()); Done. https://codereview.chromium.org/2735563002/diff/160001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/160001/src/elements.cc#newcod... src/elements.cc:2878: I am sorry for the std::enable_if hack, it is a bit hard to read. Wait till we can use "if constexpr" from C++17.
more nits https://codereview.chromium.org/2735563002/diff/180001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/180001/src/elements.cc#newcod... src/elements.cc:2881: Handle<JSTypedArray> array = Handle<JSTypedArray>::cast(receiver); Another nit, please add: DHECK(!array->WasNeutered()); https://codereview.chromium.org/2735563002/diff/180001/src/elements.cc#newcod... src/elements.cc:2887: ctype value = MaybeClamp<ctype>(obj_value, *array); Sorry, didn't spot that before eather. The compiler will take care of this, that the "beauty" of the CRTP: if (Kind == UINT8_CLAMPED_ELEMENTS) { value = std::min<double>(std::max<double>(0, value), 255); } Kind is already a statically known template param here, so no need to create the helpers. https://codereview.chromium.org/2735563002/diff/180001/test/mjsunit/es6/typed... File test/mjsunit/es6/typedarray-fill.js (right): https://codereview.chromium.org/2735563002/diff/180001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-fill.js:37: Please add my Proxy test case as well, to ensure we do ToNumber on ever iteration step.
https://codereview.chromium.org/2735563002/diff/180001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/180001/src/elements.cc#newcod... src/elements.cc:2881: Handle<JSTypedArray> array = Handle<JSTypedArray>::cast(receiver); On 2017/03/13 15:58:14, Camillo Bruni wrote: > Another nit, please add: > DHECK(!array->WasNeutered()); Done. https://codereview.chromium.org/2735563002/diff/180001/src/elements.cc#newcod... src/elements.cc:2887: ctype value = MaybeClamp<ctype>(obj_value, *array); Use "Kind == UINT8_CLAMPED_ELEMENTS" done. This make the std::enable_if even better, only Uint8ClampedArray will match the first template case. value must only be casted back to uint8_t after clamping, otherwise overflow/underflow will occur before clamping. https://codereview.chromium.org/2735563002/diff/180001/test/mjsunit/es6/typed... File test/mjsunit/es6/typedarray-fill.js (right): https://codereview.chromium.org/2735563002/diff/180001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-fill.js:37: On 2017/03/13 15:58:14, Camillo Bruni wrote: > Please add my Proxy test case as well, to ensure we do ToNumber on ever > iteration step. Done.
https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2847: static inline uint8_t MaybeClamp( ConvertNumberToUint8Clamped(Handle<Object> obj_value) { ... https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2852: value = Smi::cast(*obj_value)->value(); Special case directly on Smi and avoid slow double conversion. int value = Smi::cast(*obj_value)->value(); value = std::min<int>(std::max<int>(0, value), 255); return static_cast<uint8_t>(value); https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2862: static inline T MaybeClamp( static ctype ConvertToNumber(Handle<Object> obj_value) { ... https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2865: T value = 0; ctype value = 0; https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2884: ctype value = MaybeClamp<ctype>(obj_value, *array); I'm not a too big can of the the additional template machinery, I do find it rather hard to follow. How about? ctype value = 0; if (Kind = UINT8_CLAMPED_ELEMENTS) { value = ConvertNumberToUint8Clamped(obj_value) } else { value = ConvertToNumber(obj_value); } https://codereview.chromium.org/2735563002/diff/220001/test/mjsunit/es6/typed... File test/mjsunit/es6/typedarray-fill.js (right): https://codereview.chromium.org/2735563002/diff/220001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-fill.js:72: assertArrayEquals(new Uint8ClampedArray([0, 0, 0, 0, 0]).fill(1000), [255, 255, 255, 255, 255]); assertArrayEquals(new Uint8ClampedArray([0, 0, 0, 0, 0]).fill(0.50001), [1, 1, 1, 1, 1]); assertArrayEquals(new Uint8ClampedArray([0, 0, 0, 0, 0]).fill(0.50000), [0, 0, 0, 0, 0]); assertArrayEquals(new Uint8ClampedArray([0, 0, 0, 0, 0]).fill(0.49999), [0, 0, 0, 0, 0]); // Check round half to even assertArrayEquals(new Uint8ClampedArray([0, 0, 0, 0, 0]).fill(1.50000), [2, 2, 2, 2, 2]); assertArrayEquals(new Uint8ClampedArray([0, 0, 0, 0, 0]).fill(2.50000), [2, 2, 2, 2, 2]); assertArrayEquals(new Uint8ClampedArray([0, 0, 0, 0, 0]).fill(2.50001), [3, 3, 3, 3, 3]); // Check infinity clamping. assertArrayEquals(new Uint8ClampedArray([0, 0, 0, 0, 0]).fill(-Infinity), [0, 0, 0, 0, 0]); assertArrayEquals(new Uint8ClampedArray([0, 0, 0, 0, 0]).fill(Infinity), [255, 255, 255, 255, 255]);
https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2862: static inline T MaybeClamp( On 2017/03/14 at 09:30:36, Camillo Bruni wrote: > static ctype ConvertNumber(Handle<Object> obj_value) { ...
Floating point clamping tests mostly fail, not an expert in floating point precision. Need advice. https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2847: static inline uint8_t MaybeClamp( On 2017/03/14 09:30:35, Camillo Bruni wrote: > ConvertNumberToUint8Clamped(Handle<Object> obj_value) { ... Done. https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2852: value = Smi::cast(*obj_value)->value(); On 2017/03/14 09:30:36, Camillo Bruni wrote: > Special case directly on Smi and avoid slow double conversion. > > int value = Smi::cast(*obj_value)->value(); > value = std::min<int>(std::max<int>(0, value), 255); > return static_cast<uint8_t>(value); Done. https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2862: static inline T MaybeClamp( On 2017/03/14 09:30:36, Camillo Bruni wrote: > static ctype ConvertToNumber(Handle<Object> obj_value) { ... Done. https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2865: T value = 0; On 2017/03/14 09:30:36, Camillo Bruni wrote: > ctype value = 0; Done. https://codereview.chromium.org/2735563002/diff/220001/src/elements.cc#newcod... src/elements.cc:2884: ctype value = MaybeClamp<ctype>(obj_value, *array); Done. I previously tested similar solution but with array->type(), got a lot of warnings, that was why I resorted to template. Not necessary anymore.
https://codereview.chromium.org/2735563002/diff/280001/test/mjsunit/es6/typed... File test/mjsunit/es6/typedarray-fill.js (right): https://codereview.chromium.org/2735563002/diff/280001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-fill.js:19: assertArrayEquals(new constructor([0]).fill(), [0]); This line fails with "Failure (array element at index 0): expected <NaN> found <0>". Also occur in V8 5.7.0
https://codereview.chromium.org/2735563002/diff/280001/test/mjsunit/es6/typed... File test/mjsunit/es6/typedarray-fill.js (right): https://codereview.chromium.org/2735563002/diff/280001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-fill.js:19: assertArrayEquals(new constructor([0]).fill(), [0]); On 2017/03/14 11:17:15, rongjie wrote: > This line fails with "Failure (array element at index 0): expected <NaN> found > <0>". > > Also occur in V8 5.7.0 Test fixed.
https://codereview.chromium.org/2735563002/diff/300001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/300001/src/elements.cc#newcod... src/elements.cc:2850: return static_cast<uint8_t>(value); Could you use FixedTypedArray<Uint8ClampedArrayTraits>::from_int() ? https://codereview.chromium.org/2735563002/diff/300001/src/elements.cc#newcod... src/elements.cc:2857: return static_cast<uint8_t>(value); Could you use FixedTypedArray<Uint8ClampedArrayTraits>::from_double() ?
https://codereview.chromium.org/2735563002/diff/300001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/300001/src/elements.cc#newcod... src/elements.cc:2850: return static_cast<uint8_t>(value); On 2017/03/14 13:31:07, Camillo Bruni wrote: > Could you use FixedTypedArray<Uint8ClampedArrayTraits>::from_int() ? Done. https://codereview.chromium.org/2735563002/diff/300001/src/elements.cc#newcod... src/elements.cc:2857: return static_cast<uint8_t>(value); On 2017/03/14 13:31:07, Camillo Bruni wrote: > Could you use FixedTypedArray<Uint8ClampedArrayTraits>::from_double() ? Done.
Forgot to actually send patch before commenting ^
lgtm
On 2017/03/16 12:48:18, Camillo Bruni wrote: > lgtm bmeurer@ caitp@ PTAL? cbruni@ asked me to wait for you guys first.
On 2017/03/18 13:48:19, rongjie wrote: > On 2017/03/16 12:48:18, Camillo Bruni wrote: > > lgtm > > bmeurer@ caitp@ PTAL? cbruni@ asked me to wait for you guys first. LGTM. In general you don't need to wait for my lgtm if you have one from cbruni@ already
The CQ bit was checked by loorongjie@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/22849) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by loorongjie@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/2735563002/#ps360001 (title: "format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
On 2017/03/06 09:13:45, Camillo Bruni wrote: > first round of comments. > > https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... > File src/builtins/builtins-typedarray.cc (right): > > https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... > src/builtins/builtins-typedarray.cc:467: if (V8_UNLIKELY(array->WasNeutered())) > return *array; > This check has to happen after all the argument conversions are finished! Based on tests/mjsunit/es6/typedarray-copywithin.js:234 , it looks like this check does have to happen before all argument conversions are done?
On 2017/03/19 07:28:39, rongjie wrote: > On 2017/03/06 09:13:45, Camillo Bruni wrote: > > first round of comments. > > > > > https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... > > File src/builtins/builtins-typedarray.cc (right): > > > > > https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... > > src/builtins/builtins-typedarray.cc:467: if > (V8_UNLIKELY(array->WasNeutered())) > > return *array; > > This check has to happen after all the argument conversions are finished! > > Based on tests/mjsunit/es6/typedarray-copywithin.js:234 , it looks like this > check does have to happen before all argument conversions are done? I think the check should be happened in both of before and after arguments conversions. The first check actually happens in ValidateTypedArray, but the current implementation of ValidateTypedArray does not throw TypeError, thus we need a temporary check before arguments conversions. the second check happens in IntegerIndexedElementGet.
The CQ bit was checked by loorongjie@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/2735563002/#ps380001 (title: "Add WasNeutered checks again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19349) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
just found another simplification :) https://codereview.chromium.org/2735563002/diff/380001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/380001/src/elements.cc#newcod... src/elements.cc:2846: static inline uint8_t ConvertNumberToUint8Clamped(Handle<Object> obj_value) { I just realized that we can simplify this further :) => delete this function https://codereview.chromium.org/2735563002/diff/380001/src/elements.cc#newcod... src/elements.cc:2860: value = static_cast<ctype>(Smi::cast(*obj_value)->value()); value = BackingStore::from_int(value); https://codereview.chromium.org/2735563002/diff/380001/src/elements.cc#newcod... src/elements.cc:2863: value = static_cast<ctype>(HeapNumber::cast(*obj_value)->value()); value = BackingStore::from_double(value); https://codereview.chromium.org/2735563002/diff/380001/src/elements.cc#newcod... src/elements.cc:2879: if (Kind == UINT8_CLAMPED_ELEMENTS) { Drop the UINT8_CLAMPED_ELEMENTS check and use the generic ConvertToNumber which will use the proper conversion method from the BackingStore.
In test262/built-ins/TypedArray/prototype/fill/fill-values-conversion-operations, there is a test to cast 4294967295 (0b11111111111111111111111111111111) to different number types. The test failed for Int8Array, Int32Array and Int64Array. The test expects 4294967295 to be converted to -1, but static_cast does not work that way. int64_t bigInt = 4294967295; int32_t i32 = static_cast<int32_t>(bigInt); // -2147483648 Any idea how to fix this?
On 2017/03/20 10:41:36, rongjie wrote: > In > test262/built-ins/TypedArray/prototype/fill/fill-values-conversion-operations, > there is a test to cast 4294967295 (0b11111111111111111111111111111111) to > different number types. The test failed for Int8Array, Int32Array and > Int64Array. The test expects 4294967295 to be converted to -1, but static_cast > does not work that way. > > int64_t bigInt = 4294967295; > int32_t i32 = static_cast<int32_t>(bigInt); // -2147483648 > > Any idea how to fix this? Sorry, static_cast gives -1 as expected. -2147483648 == -0b10000000000000000000000000000000.
https://codereview.chromium.org/2735563002/diff/380001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/380001/src/elements.cc#newcod... src/elements.cc:2846: static inline uint8_t ConvertNumberToUint8Clamped(Handle<Object> obj_value) { On 2017/03/20 10:20:22, Camillo Bruni wrote: > I just realized that we can simplify this further :) => delete this function Done. https://codereview.chromium.org/2735563002/diff/380001/src/elements.cc#newcod... src/elements.cc:2860: value = static_cast<ctype>(Smi::cast(*obj_value)->value()); On 2017/03/20 10:20:22, Camillo Bruni wrote: > value = BackingStore::from_int(value); Done. https://codereview.chromium.org/2735563002/diff/380001/src/elements.cc#newcod... src/elements.cc:2863: value = static_cast<ctype>(HeapNumber::cast(*obj_value)->value()); On 2017/03/20 10:20:22, Camillo Bruni wrote: > value = BackingStore::from_double(value); Done. https://codereview.chromium.org/2735563002/diff/380001/src/elements.cc#newcod... src/elements.cc:2879: if (Kind == UINT8_CLAMPED_ELEMENTS) { On 2017/03/20 10:20:22, Camillo Bruni wrote: > Drop the UINT8_CLAMPED_ELEMENTS check and use the generic ConvertToNumber which > will use the proper conversion method from the BackingStore. Done.
The CQ bit was checked by loorongjie@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/2735563002/#ps400001 (title: "Use BackingStore::from_{int,double}")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
comments....
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/bootstrapper.cc:
While running git apply --index -p1;
error: patch failed: src/bootstrapper.cc:2612
error: src/bootstrapper.cc: patch does not apply
Patch: src/bootstrapper.cc
Index: src/bootstrapper.cc
diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc
index
8e7c0fc0ae0b3a386c354088a96efb22ff4c22ed..d75e7b57804d38f3aaf532b15a4931c41c0cf7cd
100644
--- a/src/bootstrapper.cc
+++ b/src/bootstrapper.cc
@@ -2612,6 +2612,8 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject>
global_object,
// TODO(caitp): alphasort accessors/methods
SimpleInstallFunction(prototype, "copyWithin",
Builtins::kTypedArrayPrototypeCopyWithin, 2, false);
+ SimpleInstallFunction(prototype, "fill",
+ Builtins::kTypedArrayPrototypeFill, 1, false);
SimpleInstallFunction(prototype, "includes",
Builtins::kTypedArrayPrototypeIncludes, 1, false);
}
The CQ bit was checked by loorongjie@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/2735563002/#ps440001 (title: "Merge branch 'master' into fill")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1490014589203080,
"parent_rev": "516374659100628cb171a074a878306cc858691d", "commit_rev":
"cb903e317338798fec3a43fea2ec285d3626c333"}
Message was sent while issue was closed.
Description was changed from ========== Migrate %TypedArray%.prototype.fill to C++ BUG=v8:5929 R=adamk@chromium.org,bmeurer@chromium.org ========== to ========== Migrate %TypedArray%.prototype.fill to C++ BUG=v8:5929 R=adamk@chromium.org,bmeurer@chromium.org Review-Url: https://codereview.chromium.org/2735563002 Cr-Commit-Position: refs/heads/master@{#43934} Committed: https://chromium.googlesource.com/v8/v8/+/cb903e317338798fec3a43fea2ec285d362... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:440001) as https://chromium.googlesource.com/v8/v8/+/cb903e317338798fec3a43fea2ec285d362...
Message was sent while issue was closed.
ulan@chromium.org changed reviewers: + ulan@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2735563002/diff/440001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/440001/src/elements.cc#newcod... src/elements.cc:481: isolate, cast_value, Object::ToNumber(obj_value)); Shouldn't this be hoisted outside the loop? Otherwise, it allocates (end - start) heap numbers.
Message was sent while issue was closed.
https://codereview.chromium.org/2735563002/diff/440001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2735563002/diff/440001/src/elements.cc#newcod... src/elements.cc:481: isolate, cast_value, Object::ToNumber(obj_value)); On 2017/03/22 10:10:54, ulan wrote: > Shouldn't this be hoisted outside the loop? > > Otherwise, it allocates (end - start) heap numbers. Please see cbruni@ explanation at https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... We only resort to FillNumberSlowPath if !obj_value->isNumber() (Symbol,Proxy etc.)
Message was sent while issue was closed.
On 2017/03/22 10:15:02, rongjie wrote: > https://codereview.chromium.org/2735563002/diff/440001/src/elements.cc > File src/elements.cc (right): > > https://codereview.chromium.org/2735563002/diff/440001/src/elements.cc#newcod... > src/elements.cc:481: isolate, cast_value, Object::ToNumber(obj_value)); > On 2017/03/22 10:10:54, ulan wrote: > > Shouldn't this be hoisted outside the loop? > > > > Otherwise, it allocates (end - start) heap numbers. > > Please see cbruni@ explanation at > https://codereview.chromium.org/2735563002/diff/1/src/builtins/builtins-typed... > > We only resort to FillNumberSlowPath if !obj_value->isNumber() (Symbol,Proxy > etc.) Thanks, that makes sense :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
