Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2405253006:
[builtins] implement Array.prototype[@@iterator] in TFJ builtins (Closed)
Description[builtins] implement Array.prototype[@@iterator] in TFJ builtins
Implements the variations of CreateArrayIterator() in TFJ builtins
(ArrayPrototypeValues, ArrayPrototypeEntries and ArrayPrototypeKeys), and
provides two new Object types with numerous maps which identify certain
behaviours, which will be useful for inlining.
Removes src/js/array-iterator.js entirely
Also adds support for printing Symbol literals inserted by the Parser during
desugaring when FLAG_print_builtin_ast is set to true.
BUG=v8:5388
R=bmeurer@chromium.org, cbruni@chromium.org
TBR=ulan@chromium.org
Committed: https://crrev.com/86d0dd362f627a7831b042d4fe8baa121c976fc4
Cr-Commit-Position: refs/heads/master@{#40373}
Patch Set 1 #
Total comments: 29
Patch Set 2 : V1 #
Total comments: 1
Patch Set 3 : V1 rebased :) #Patch Set 4 : revert unneeded prologue.js changes #
Total comments: 1
Patch Set 5 : revert incorrect test262.status changes #Patch Set 6 : remove apparently redundant var bindings and fix other bugs #
Total comments: 11
Patch Set 7 : and fix Context::array_values_iterator() #Patch Set 8 : rebase and get tests passing #
Total comments: 4
Patch Set 9 : add array_iterator_protector, and check array_protector in holey fast arrays #
Total comments: 5
Patch Set 10 : speedups that probably still pass tests #Patch Set 11 : fix JSArrayIteratorVerify() #
Total comments: 25
Patch Set 12 : latest round #
Messages
Total messages: 89 (43 generated)
If possible, it would be nice to get a look at this so far (but this needs a few more days before it's seriously ready for anything) https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array.cc File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2088: Node* receiver = assembler->Parameter(0); For reasons I don't completely understand, the for-of loop in PostNatives() in src/js/prologue.js results in this being called with an undefined receiver. This could be caused by the way this function is bootstrapped, but afaik it's set up correctly in runtime-array.cc. Not entirely sure. Once that's fixed, most of the Array-related tests should be passing, but the TypedArray stuff isn't done yet.
bmeurer@chromium.org changed reviewers: + yangguo@chromium.org - mstarzinger@chromium.org
First round of comments. +yangguo@ for bootstrapping Array stuff. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array.cc File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2071: void Builtins::Generate_ArrayPrototypeValues(CodeStubAssembler* assembler) { Do you plan to do a separate TypedArrayPrototypeValues? Because that just throws if the receiver is not a typed array. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2077: Variable var_result(assembler, MachineRepresentation::kTagged); This seems to be unused. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2088: Node* receiver = assembler->Parameter(0); Hm, this looks like a bug somewhere. You could replace that for-of loop with a simple for loop to mitigate the problem if it's due to the funky JS+runtime bootstrapping process. Maybe Yang can help here too. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2095: var_map_index.Bind(assembler->IntPtrConstant(0)); You don't need to bind var_map / var_map_index here. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2119: Node* map_index = assembler->IntPtrAdd( Neat! But please add some STATIC_ASSERTs to guard this and a comment explaining what this does :-) https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2150: Node* iterator = assembler->Allocate(JSArrayIterator::kSize); Can you add a helper method AllocateArrayIterator(map, array, array_map) for this? https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2198: void Builtins::Generate_ArrayPrototypeEntries(CodeStubAssembler* assembler) { Can you unify this with the ArrayPrototypeValues above? The only difference seems to be the map index in the context. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2346: var_map.Bind(assembler->UndefinedConstant()); You don't need to Bind var_result and var_map here. These bindings are essentially dead code. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2362: &allocate_array_iterator, &allocate_typed_array_iterator); You need to handle the generic as well. Also non-fast JSArray need to do "length" access in each iteration. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2452: // FIXME: Use bitmasks instead of integer ranges You can subtract the base and then do a single unsigned comparison. That way you need only a single comparison, and don't need to fiddle with bitmasks. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2483: assembler->GotoIf(assembler->Word32Equal( How about just having a single switch over all relevant instance types here? https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2493: { // JSArray-specialized algorithm for fast JSArrays You can only take the fast JSArray case if the array_map still matches the recorded initial map, also for the key iterator as someone could have installed a "length" getter or something like that. https://codereview.chromium.org/2405253006/diff/1/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2405253006/diff/1/src/isolate.cc#newcode1099 src/isolate.cc:1099: if (false && bootstrapper()->IsActive()) { Undo this.
https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array.cc File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2088: Node* receiver = assembler->Parameter(0); On 2016/10/13 05:07:19, Benedikt Meurer wrote: > Hm, this looks like a bug somewhere. You could replace that for-of loop with a > simple for loop to mitigate the problem if it's due to the funky JS+runtime > bootstrapping process. Maybe Yang can help here too. Doesn't ring a bell, sorry. But I'm not all that surprised that things behave weirdly while setting up stuff in the bootstrapper. But maybe we could port everything in prologue.js to C++ \o/
https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array.cc File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2088: Node* receiver = assembler->Parameter(0); On 2016/10/13 13:52:37, Yang wrote: > On 2016/10/13 05:07:19, Benedikt Meurer wrote: > > Hm, this looks like a bug somewhere. You could replace that for-of loop with a > > simple for loop to mitigate the problem if it's due to the funky JS+runtime > > bootstrapping process. Maybe Yang can help here too. > > Doesn't ring a bell, sorry. But I'm not all that surprised that things behave > weirdly while setting up stuff in the bootstrapper. > > But maybe we could port everything in prologue.js to C++ \o/ It turns out, including the receiver in the internal formal parameter count broke this. Fixed by changing the `1` to a `0` for the length parameter to InstallBuiltin() calls in runtime-array.cc. I figured it was something like that.
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/14573) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/14522) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26451)
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've added the TypedArray stuff, seems to be working pretty nicely and passes some previously failing test262 tests https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array.cc File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2071: void Builtins::Generate_ArrayPrototypeValues(CodeStubAssembler* assembler) { On 2016/10/13 05:07:19, Benedikt Meurer wrote: > Do you plan to do a separate TypedArrayPrototypeValues? Because that just throws > if the receiver is not a typed array. I've made it so that the triplet methods (keys/values/entries) can all share common code (TypedArrays get their own version with the extra typechecking) https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2077: Variable var_result(assembler, MachineRepresentation::kTagged); On 2016/10/13 05:07:20, Benedikt Meurer wrote: > This seems to be unused. I _think_ this is no longer an issue after the big refactoring, but I may be wrong https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2095: var_map_index.Bind(assembler->IntPtrConstant(0)); On 2016/10/13 05:07:19, Benedikt Meurer wrote: > You don't need to bind var_map / var_map_index here. This still happens in the new version, in CodeStubAssembler::CreateArrayIterator() --- due to check failures when it's not initially bound (even though it's always bound before use). https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2119: Node* map_index = assembler->IntPtrAdd( On 2016/10/13 05:07:19, Benedikt Meurer wrote: > Neat! But please add some STATIC_ASSERTs to guard this and a comment explaining > what this does :-) ah, I think I added some comments to this stuff, but I'll probably clarify it in a future patch. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2150: Node* iterator = assembler->Allocate(JSArrayIterator::kSize); On 2016/10/13 05:07:19, Benedikt Meurer wrote: > Can you add a helper method AllocateArrayIterator(map, array, array_map) for > this? I've added CodeStubAssembler::AllocateJSArrayIterator() and AllocateJSTypedArrayIterator() https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2198: void Builtins::Generate_ArrayPrototypeEntries(CodeStubAssembler* assembler) { On 2016/10/13 05:07:19, Benedikt Meurer wrote: > Can you unify this with the ArrayPrototypeValues above? The only difference > seems to be the map index in the context. Done in CodeStubAssembler::CreateArrayIterator() https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2346: var_map.Bind(assembler->UndefinedConstant()); On 2016/10/13 05:07:19, Benedikt Meurer wrote: > You don't need to Bind var_result and var_map here. These bindings are > essentially dead code. They are, but as I mentioned, they still hit CHECK failures if not bound https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2362: &allocate_array_iterator, &allocate_typed_array_iterator); On 2016/10/13 05:07:19, Benedikt Meurer wrote: > You need to handle the generic as well. Also non-fast JSArray need to do > "length" access in each iteration. I'm not sure it's possible to skip the length check, changing the length field doesn't seem to guarantee that the map or elements kind changes (some length changes always do, but some don't). https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2452: // FIXME: Use bitmasks instead of integer ranges On 2016/10/13 05:07:19, Benedikt Meurer wrote: > You can subtract the base and then do a single unsigned comparison. That way you > need only a single comparison, and don't need to fiddle with bitmasks. Done, seems to be working nicely. https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2483: assembler->GotoIf(assembler->Word32Equal( On 2016/10/13 05:07:19, Benedikt Meurer wrote: > How about just having a single switch over all relevant instance types here? Done https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2493: { // JSArray-specialized algorithm for fast JSArrays On 2016/10/13 05:07:19, Benedikt Meurer wrote: > You can only take the fast JSArray case if the array_map still matches the > recorded initial map, also for the key iterator as someone could have installed > a "length" getter or something like that. I'll add some new interesting tests to ensure all of this in a followup https://codereview.chromium.org/2405253006/diff/1/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2405253006/diff/1/src/isolate.cc#newcode1099 src/isolate.cc:1099: if (false && bootstrapper()->IsActive()) { On 2016/10/13 05:07:20, Benedikt Meurer wrote: > Undo this. Done. https://codereview.chromium.org/2405253006/diff/20001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2405253006/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:1335: array_iterator_function->shared()->set_instance_class_name( This doesn't change allows me to not alter some tests which check the result of %ClassOf() in test/mjsunit/es6/array-iterator.js, but other than that it doesn't buy much (and allocates some extra stuff that gets thrown away :(). Happy to remove it if the test change is preferred.
https://codereview.chromium.org/2405253006/diff/60001/test/test262/test262.st... File test/test262/test262.status (left): https://codereview.chromium.org/2405253006/diff/60001/test/test262/test262.st... test/test262/test262.status:138: 'built-ins/TypedArrays/internals/DefineOwnProperty/detached-buffer': [FAIL], Oop, these removals are wrong (I only re-ran TypedArrays/prototype/* and didn't notice)
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/14577) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: 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
Dry run: 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 2016/10/13 22:39:42, caitp wrote: > https://codereview.chromium.org/2405253006/diff/60001/test/test262/test262.st... > File test/test262/test262.status (left): > > https://codereview.chromium.org/2405253006/diff/60001/test/test262/test262.st... > test/test262/test262.status:138: > 'built-ins/TypedArrays/internals/DefineOwnProperty/detached-buffer': [FAIL], > Oop, these removals are wrong (I only re-ran TypedArrays/prototype/* and didn't > notice) hmm, thought I fixed all the issues, but it seems like there are some new ones now... will investigate tomorrow
https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array.cc File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/1/src/builtins/builtins-array... src/builtins/builtins-array.cc:2362: &allocate_array_iterator, &allocate_typed_array_iterator); What I'm saying is you need to do the map check definitely in either case. But for fast JSArray then map check is sufficient to know that it's safe to access the JSArray::length field directly. For the generic case you need to use the GetPropertyStub to read "length" in each iteration. https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... src/builtins/builtins-array.cc:2093: assembler->Branch( You can use CodeStubAssembler::IsJSReceiverInstanceType here. https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... src/builtins/builtins-array.cc:2193: assembler->GotoIf(assembler->Word32Equal( What is the motivation to not do a big switch on the iterator instance type? Do you think it would be too slow/bad for cache? For example: var index = iterator->index; var done = true; var value = undefined; switch (iterator->instance_type) { case FAST_JSARRAY_KEY: { if (object->map != iterator->object_map) goto generic_key; if (SmiLessThan(index, object->length)) { done = false; value = index; index = SmiInc(index); } break; } case FAST_JSARRAY_SMI_VALUE: { if (object->map != iterator->object_map) goto generic_value; if (SmiLessThan(index, object->length)) { done = false; value = object[index]; index = SmiInc(index); } break; } ... case GENERIC_JSARRAY_KEY: generic_key: { length = ToLengthStub(GetPropertyStub(object, "length")); if (NumberLessThan(index, length)) { done = false; value = index; index = NumberInc(index); } break; } } iterator->index = index; return CreateIterResultObject(done,value); Not saying you have to/should do it this way. Just wondering if this would be easier and faster? (given some generator helper functions) https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... src/builtins/builtins-array.cc:2206: assembler->LoadObjectField(array, JSArray::kLengthOffset)); This is only valid if the array map didn't change (and you previously checked that it's fast, and length was not messed up). You have to do the map check first, otherwise it's not guaranteed that the JSArray::length access gives you the value that you'd get when loading "length". https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:105: const char* operation) { Nit: s/operation/method_name/ for consistenvy https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:126: Node* receiver_buffer = Can you extract this into a helper, it's also used in the methods above. https://codereview.chromium.org/2405253006/diff/100001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2405253006/diff/100001/src/code-stub-assemble... src/code-stub-assembler.cc:7565: // Fast Array iterator map index: Nice, thanks!
https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... src/builtins/builtins-array.cc:2193: assembler->GotoIf(assembler->Word32Equal( On 2016/10/14 03:46:50, Benedikt Meurer wrote: > What is the motivation to not do a big switch on the iterator instance type? Do > you think it would be too slow/bad for cache? > > For example: > > var index = iterator->index; > var done = true; > var value = undefined; > switch (iterator->instance_type) { > case FAST_JSARRAY_KEY: { > if (object->map != iterator->object_map) goto generic_key; > if (SmiLessThan(index, object->length)) { > done = false; > value = index; > index = SmiInc(index); > } > break; > } > case FAST_JSARRAY_SMI_VALUE: { > if (object->map != iterator->object_map) goto generic_value; > if (SmiLessThan(index, object->length)) { > done = false; > value = object[index]; > index = SmiInc(index); > } > break; > } > ... > case GENERIC_JSARRAY_KEY: > generic_key: { > length = ToLengthStub(GetPropertyStub(object, "length")); > if (NumberLessThan(index, length)) { > done = false; > value = index; > index = NumberInc(index); > } > break; > } > } > iterator->index = index; > return CreateIterResultObject(done,value); > > > Not saying you have to/should do it this way. Just wondering if this would be > easier and faster? (given some generator helper functions) I suppose there's no reason not to do it, other than taking up more memory and not necessarily adding that much in terms of performance (but I'll give it a try to see, I suppose) https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... src/builtins/builtins-array.cc:2206: assembler->LoadObjectField(array, JSArray::kLengthOffset)); On 2016/10/14 03:46:50, Benedikt Meurer wrote: > This is only valid if the array map didn't change (and you previously checked > that it's fast, and length was not messed up). You have to do the map check > first, otherwise it's not guaranteed that the JSArray::length access gives you > the value that you'd get when loading "length". I don't think it's possible to alter the way "length" works for Array exotic objects, per spec (EG step 10 of ArrayCreate(), Perform ! OrdinaryDefineOwnProperty(A, "length", PropertyDescriptor{[[Value]]: length, [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: false})). This is always an own property, so it doesn't matter if an accessor is installed on a subclass or anything. If instance_type is JS_ARRAY_TYPE, loading the in object "length" field should be sound https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:126: Node* receiver_buffer = On 2016/10/14 03:46:51, Benedikt Meurer wrote: > Can you extract this into a helper, it's also used in the methods above. Acknowledged.
https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... src/builtins/builtins-array.cc:2193: assembler->GotoIf(assembler->Word32Equal( As said, I'm perfectly happy if you say the other solution is better for reasons. https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... src/builtins/builtins-array.cc:2206: assembler->LoadObjectField(array, JSArray::kLengthOffset)); Ah, indeed, you are right. Ok, so this reduces to the question of whether to do the big switch and only a map check, or your solution with an instance type check on the object first, and then only do the map check for the value and value/key iterators. I'd fine with either; whatever is easier / more efficient.
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/14589) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/14 04:11:18, Benedikt Meurer wrote: > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > File src/builtins/builtins-array.cc (right): > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > src/builtins/builtins-array.cc:2193: assembler->GotoIf(assembler->Word32Equal( > As said, I'm perfectly happy if you say the other solution is better for > reasons. > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > src/builtins/builtins-array.cc:2206: assembler->LoadObjectField(array, > JSArray::kLengthOffset)); > Ah, indeed, you are right. > > Ok, so this reduces to the question of whether to do the big switch and only a > map check, or your solution with an instance type check on the object first, and > then only do the map check for the value and value/key iterators. > > I'd fine with either; whatever is easier / more efficient.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/14 04:11:18, Benedikt Meurer wrote: > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > File src/builtins/builtins-array.cc (right): > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > src/builtins/builtins-array.cc:2193: assembler->GotoIf(assembler->Word32Equal( > As said, I'm perfectly happy if you say the other solution is better for > reasons. > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > src/builtins/builtins-array.cc:2206: assembler->LoadObjectField(array, > JSArray::kLengthOffset)); > Ah, indeed, you are right. > > Ok, so this reduces to the question of whether to do the big switch and only a > map check, or your solution with an instance type check on the object first, and > then only do the map check for the value and value/key iterators. > > I'd fine with either; whatever is easier / more efficient. So, I've done some testing this morning using an adaptation of your simple StringIterator microbenchmark: ``` "use strict"; function test(a) { for (var x of a) {} return a; } var a = "Hello World!".repeat(500000).split(""); // Warmup for (var i = 0; i < 3; ++i) test(a); // Benchmark let startTime = Date.now(); for (let i = 0; i < 10; ++i) test(a); let endTime = Date.now(); print("Time: " + (endTime - startTime) + "ms."); ``` Currently, the old JS implementation seems to consistently beat the TFJ implementation by about 150-200ms. I'll try out the single dispatch version to see if that does any better.
On 2016/10/14 14:51:38, caitp wrote: > On 2016/10/14 04:11:18, Benedikt Meurer wrote: > > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > > File src/builtins/builtins-array.cc (right): > > > > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > > src/builtins/builtins-array.cc:2193: assembler->GotoIf(assembler->Word32Equal( > > As said, I'm perfectly happy if you say the other solution is better for > > reasons. > > > > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > > src/builtins/builtins-array.cc:2206: assembler->LoadObjectField(array, > > JSArray::kLengthOffset)); > > Ah, indeed, you are right. > > > > Ok, so this reduces to the question of whether to do the big switch and only a > > map check, or your solution with an instance type check on the object first, > and > > then only do the map check for the value and value/key iterators. > > > > I'd fine with either; whatever is easier / more efficient. > > So, I've done some testing this morning using an adaptation of your simple > StringIterator microbenchmark: > > ``` > "use strict"; > > function test(a) { > for (var x of a) {} > return a; > } > > var a = "Hello World!".repeat(500000).split(""); > > // Warmup > for (var i = 0; i < 3; ++i) test(a); > > // Benchmark > let startTime = Date.now(); > for (let i = 0; i < 10; ++i) test(a); > let endTime = Date.now(); > print("Time: " + (endTime - startTime) + "ms."); > ``` > > Currently, the old JS implementation seems to consistently beat the TFJ > implementation by about 150-200ms. I'll try out the single dispatch version to > see if that does any better. As long as we're roughly the same speed with baseline, that's OK. You're comparing against the JS version with monomorphic feedback. Try the same with also warming up the array iterator with TypedArrays and other (fast) JSArrays. You'll see the JS version becoming slower, while your version remains the same speed.
On 2016/10/14 17:11:34, Benedikt Meurer wrote: > On 2016/10/14 14:51:38, caitp wrote: > > On 2016/10/14 04:11:18, Benedikt Meurer wrote: > > > > > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > > > File src/builtins/builtins-array.cc (right): > > > > > > > > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > > > src/builtins/builtins-array.cc:2193: > assembler->GotoIf(assembler->Word32Equal( > > > As said, I'm perfectly happy if you say the other solution is better for > > > reasons. > > > > > > > > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > > > src/builtins/builtins-array.cc:2206: assembler->LoadObjectField(array, > > > JSArray::kLengthOffset)); > > > Ah, indeed, you are right. > > > > > > Ok, so this reduces to the question of whether to do the big switch and only > a > > > map check, or your solution with an instance type check on the object first, > > and > > > then only do the map check for the value and value/key iterators. > > > > > > I'd fine with either; whatever is easier / more efficient. > > > > So, I've done some testing this morning using an adaptation of your simple > > StringIterator microbenchmark: > > > > ``` > > "use strict"; > > > > function test(a) { > > for (var x of a) {} > > return a; > > } > > > > var a = "Hello World!".repeat(500000).split(""); > > > > // Warmup > > for (var i = 0; i < 3; ++i) test(a); > > > > // Benchmark > > let startTime = Date.now(); > > for (let i = 0; i < 10; ++i) test(a); > > let endTime = Date.now(); > > print("Time: " + (endTime - startTime) + "ms."); > > ``` > > > > Currently, the old JS implementation seems to consistently beat the TFJ > > implementation by about 150-200ms. I'll try out the single dispatch version to > > see if that does any better. > > As long as we're roughly the same speed with baseline, that's OK. You're > comparing against the JS version with monomorphic feedback. Try the same with > also warming up the array iterator with TypedArrays and other (fast) JSArrays. > You'll see the JS version becoming slower, while your version remains the same > speed. That's true --- as an update, I've built a version with the single dispatch behaviour (which generates a much bigger 'next' function, due to a lot of repeated stuff for each section), and it is measuring up to be significantly slower (~3000ms on that thing vs the ~700ms of the version in this CL). This could be caused by making a lot of poor choices in the implementation, but yeah, I'm not sure that's the way to go for the baseline.
On 2016/10/14 17:42:03, caitp wrote: > On 2016/10/14 17:11:34, Benedikt Meurer wrote: > > On 2016/10/14 14:51:38, caitp wrote: > > > On 2016/10/14 04:11:18, Benedikt Meurer wrote: > > > > > > > > > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > > > > File src/builtins/builtins-array.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > > > > src/builtins/builtins-array.cc:2193: > > assembler->GotoIf(assembler->Word32Equal( > > > > As said, I'm perfectly happy if you say the other solution is better for > > > > reasons. > > > > > > > > > > > > > > https://codereview.chromium.org/2405253006/diff/100001/src/builtins/builtins-... > > > > src/builtins/builtins-array.cc:2206: assembler->LoadObjectField(array, > > > > JSArray::kLengthOffset)); > > > > Ah, indeed, you are right. > > > > > > > > Ok, so this reduces to the question of whether to do the big switch and > only > > a > > > > map check, or your solution with an instance type check on the object > first, > > > and > > > > then only do the map check for the value and value/key iterators. > > > > > > > > I'd fine with either; whatever is easier / more efficient. > > > > > > So, I've done some testing this morning using an adaptation of your simple > > > StringIterator microbenchmark: > > > > > > ``` > > > "use strict"; > > > > > > function test(a) { > > > for (var x of a) {} > > > return a; > > > } > > > > > > var a = "Hello World!".repeat(500000).split(""); > > > > > > // Warmup > > > for (var i = 0; i < 3; ++i) test(a); > > > > > > // Benchmark > > > let startTime = Date.now(); > > > for (let i = 0; i < 10; ++i) test(a); > > > let endTime = Date.now(); > > > print("Time: " + (endTime - startTime) + "ms."); > > > ``` > > > > > > Currently, the old JS implementation seems to consistently beat the TFJ > > > implementation by about 150-200ms. I'll try out the single dispatch version > to > > > see if that does any better. > > > > As long as we're roughly the same speed with baseline, that's OK. You're > > comparing against the JS version with monomorphic feedback. Try the same with > > also warming up the array iterator with TypedArrays and other (fast) JSArrays. > > You'll see the JS version becoming slower, while your version remains the same > > speed. > > That's true --- as an update, I've built a version with the single dispatch > behaviour (which generates a much bigger 'next' function, due to a lot of > repeated stuff for each section), and it is measuring up to be significantly > slower (~3000ms on that thing vs the ~700ms of the version in this CL). > > This could be caused by making a lot of poor choices in the implementation, but > yeah, I'm not sure that's the way to go for the baseline. (For comparison, a CL with that slower version is available at https://codereview.chromium.org/2419983003)
Thanks for prototyping this! Your intuition was right and the simpler implementation clearly beats the switch based one. I'll have a close look at this guy now.
I'm looking at this simple benchmark:
==============================================
"use strict";
function test(a) {
for (var x of a) { }
return x;
}
var a1 = [];
for (var i = 0; i < 500000; ++i) {
a1.push(i);
}
// Warmup
for (var i = 0; i < 1000; ++i) {
test([1,2,3,4]);
}
function time(test, a) {
let startTime = Date.now();
for (let i = 0; i < 10; ++i) test(a);
let endTime = Date.now();
print("Time: " + (endTime - startTime) + "ms.");
}
time(test, a1);
// Pollute feedback
for (var x of [1,2]) {}
for (var x of [1,,3]) {}
for (var x of [1.1,2.2]) {}
for (var x of new Uint8Array(10)) {}
for (var x of {length: 7, __proto__: Array.prototype}) {}
time(test, a1);
==============================================
Running with the old JS implementation I have
Time: 68ms.
Time: 125ms.
while with yours I get
Time: 76ms.
Time: 74ms.
which is slightly slower than the "optimal" monomorphic, but almost 2x faster
than the polluted version. This is what matters the most, as any non-trivial ES6
version will use the array iterator with different array kinds (or even array
like objects).
So the question is: Can we tweak the baseline version to also catch up with the
missing 10%?
On 2016/10/14 18:46:45, Benedikt Meurer wrote:
> I'm looking at this simple benchmark:
>
> ==============================================
> "use strict";
>
> function test(a) {
> for (var x of a) { }
> return x;
> }
>
> var a1 = [];
> for (var i = 0; i < 500000; ++i) {
> a1.push(i);
> }
>
> // Warmup
> for (var i = 0; i < 1000; ++i) {
> test([1,2,3,4]);
> }
>
> function time(test, a) {
> let startTime = Date.now();
> for (let i = 0; i < 10; ++i) test(a);
> let endTime = Date.now();
> print("Time: " + (endTime - startTime) + "ms.");
> }
>
> time(test, a1);
> // Pollute feedback
> for (var x of [1,2]) {}
> for (var x of [1,,3]) {}
> for (var x of [1.1,2.2]) {}
> for (var x of new Uint8Array(10)) {}
> for (var x of {length: 7, __proto__: Array.prototype}) {}
> time(test, a1);
> ==============================================
>
> Running with the old JS implementation I have
>
> Time: 68ms.
> Time: 125ms.
>
> while with yours I get
>
> Time: 76ms.
> Time: 74ms.
>
> which is slightly slower than the "optimal" monomorphic, but almost 2x faster
> than the polluted version. This is what matters the most, as any non-trivial
ES6
> version will use the array iterator with different array kinds (or even array
> like objects).
>
> So the question is: Can we tweak the baseline version to also catch up with
the
> missing 10%?
Something that might help this particular test, is treating packed fast elements
differently from holey packed elements. Currently they both take the same branch
to save some space. It's worth a try.
You need to do the holey cases separately, as you also need to check the protector cell. https://codereview.chromium.org/2405253006/diff/140001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/140001/src/builtins/builtins-... src/builtins/builtins-array.cc:2313: Label keys(assembler, Label::kDeferred); These shouldn't be marked as deferred, otherwise they are all penalized by the register allocator. All of them are non-exceptional paths, so keep them non-deferred. https://codereview.chromium.org/2405253006/diff/140001/src/builtins/builtins-... src/builtins/builtins-array.cc:2355: assembler->Branch(assembler->TaggedIsSmi(index), &is_smi, &is_heapnum); This can actually be an assembler->Assert(assembler->TaggedIsSmi(index)), since fast array lengths are always in unsigned smi range. Then you can also use the SMI_PARAMETERS mode for the LoadFixedArrayElement below. https://codereview.chromium.org/2405253006/diff/140001/src/builtins/builtins-... src/builtins/builtins-array.cc:2376: var_value.Bind(assembler->UndefinedConstant()); This is only correct for holey arrays if the protector cell is valid. Otherwise you need to do a lookup in the prototype chain (i.e. use the generic path).
https://codereview.chromium.org/2405253006/diff/140001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/140001/src/builtins/builtins-... src/builtins/builtins-array.cc:2191: load_length_slow(assembler), did_get_length(assembler), The load_length_slow label should be marked as deferred.
Ok, with some hacking I made the baseline version faster than the JS version. Here's roughly what I did: For TypedArrays, set the array_map on the iterator to some sentinel (i.e. undefined or some oddball). Then instead of dispatching on the instance type, first dispatch on the map check, i.e. check if the map is still the same as the recorded one, which will only be true for fast JSArray then (which also means you don't need to do the map check later, reducing the number of live ranges). Loading the JSArray::length will always yield a Smi then (same for index obviously), which you can use jump directly to the if_smi case. If the map check fails, check based on instance type whether you have JSTypedArray or generic (as Adam pointed out in the document, the TypedArray case isn't that relevant, plus for inlining we don't need the original map there). Also make sure to mark the slow labels as deferred (but not the other ones) as mentioned earlier. This together with my previous comments yields 65-67ms for me, thus already beating the "optimal JS" version. :-)
On 2016/10/14 19:20:51, Benedikt Meurer wrote: > Ok, with some hacking I made the baseline version faster than the JS version. > Here's roughly what I did: > > For TypedArrays, set the array_map on the iterator to some sentinel (i.e. > undefined or some oddball). Then instead of dispatching on the instance type, > first dispatch on the map check, i.e. check if the map is still the same as the > recorded one, which will only be true for fast JSArray then (which also means > you don't need to do the map check later, reducing the number of live ranges). > Loading the JSArray::length will always yield a Smi then (same for index > obviously), which you can use jump directly to the if_smi case. > If the map check fails, check based on instance type whether you have > JSTypedArray or generic (as Adam pointed out in the document, the TypedArray > case isn't that relevant, plus for inlining we don't need the original map > there). Also make sure to mark the slow labels as deferred (but not the other > ones) as mentioned earlier. > > This together with my previous comments yields 65-67ms for me, thus already > beating the "optimal JS" version. :-) I've given something like that a try --- Unfortunately, something goes wrong during compilation
On 2016/10/15 04:14:57, caitp wrote: > On 2016/10/14 19:20:51, Benedikt Meurer wrote: > > Ok, with some hacking I made the baseline version faster than the JS version. > > Here's roughly what I did: > > > > For TypedArrays, set the array_map on the iterator to some sentinel (i.e. > > undefined or some oddball). Then instead of dispatching on the instance type, > > first dispatch on the map check, i.e. check if the map is still the same as > the > > recorded one, which will only be true for fast JSArray then (which also means > > you don't need to do the map check later, reducing the number of live ranges). > > Loading the JSArray::length will always yield a Smi then (same for index > > obviously), which you can use jump directly to the if_smi case. > > If the map check fails, check based on instance type whether you have > > JSTypedArray or generic (as Adam pointed out in the document, the TypedArray > > case isn't that relevant, plus for inlining we don't need the original map > > there). Also make sure to mark the slow labels as deferred (but not the other > > ones) as mentioned earlier. > > > > This together with my previous comments yields 65-67ms for me, thus already > > beating the "optimal JS" version. :-) > > I've given something like that a try --- Unfortunately, something goes wrong > during compilation I'll take another look in the evening, and send you a proposal, I think my hack was not fully correct yet.
On 2016/10/15 08:43:22, Benedikt Meurer (Google) wrote:
> On 2016/10/15 04:14:57, caitp wrote:
> > On 2016/10/14 19:20:51, Benedikt Meurer wrote:
> > > Ok, with some hacking I made the baseline version faster than the JS
> version.
> > > Here's roughly what I did:
> > >
> > > For TypedArrays, set the array_map on the iterator to some sentinel (i.e.
> > > undefined or some oddball). Then instead of dispatching on the instance
> type,
> > > first dispatch on the map check, i.e. check if the map is still the same
as
> > the
> > > recorded one, which will only be true for fast JSArray then (which also
> means
> > > you don't need to do the map check later, reducing the number of live
> ranges).
> > > Loading the JSArray::length will always yield a Smi then (same for index
> > > obviously), which you can use jump directly to the if_smi case.
> > > If the map check fails, check based on instance type whether you have
> > > JSTypedArray or generic (as Adam pointed out in the document, the
TypedArray
> > > case isn't that relevant, plus for inlining we don't need the original map
> > > there). Also make sure to mark the slow labels as deferred (but not the
> other
> > > ones) as mentioned earlier.
> > >
> > > This together with my previous comments yields 65-67ms for me, thus
already
> > > beating the "optimal JS" version. :-)
> >
> > I've given something like that a try --- Unfortunately, something goes wrong
> > during compilation
>
> I'll take another look in the evening, and send you a proposal, I think my
hack
> was not fully correct yet.
Anyways, some details on the crash, some RelocInfo item is screwed up. A
CODE_TARGET RelocItem returns Smi::kZero for `target_object_handle()`, which
somehow doesn't cause a CHECK failure in Code::cast(), but leads to weird memory
accesses after that.
```
14117 Handle<Object> p = it.rinfo()->target_object_handle(origin);
-> 14118 Code* code = Code::cast(*p);
14119 it.rinfo()->set_target_address(code->instruction_start(),
14120 UPDATE_WRITE_BARRIER,
SKIP_ICACHE_FLUSH);
14121 } else if (RelocInfo::IsRuntimeEntry(mode)) {
(lldb) p p
warning: could not load any Objective-C class information. This will
significantly reduce the quality of type information available.
(v8::internal::Handle<v8::internal::Object>) $189 = {
v8::internal::HandleBase = {
location_ = 0x0000000103014be8
}
}
(lldb) p *p
(v8::internal::Object *) $190 = 0x0000000000000000
(lldb) p p.is_null()
(bool) $191 = false
(lldb) p p->IsSmi()
(bool) $192 = true
(lldb) p *p == Smi::kZero
(bool) $193 = true
```
I think this started happening after adding the protector cell, but it may have
been something else.
You mean when you added the ArrayIteratorProtector? https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... src/builtins/builtins-array.cc:2168: assembler->GotoIf(assembler->WordEqual(array, assembler->UndefinedConstant()), You can move this check to the if_isnotfastarray case. That removes another check from the fast array common case. Undefined will always fail the JSArray map check. https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... src/builtins/builtins-array.cc:2292: Label if_istypedarray(assembler), if_isgeneric(assembler); Mark the if_isgeneric with kDeferred. https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... src/builtins/builtins-array.cc:2326: assembler->Bind(&invalidate_array_iterator_protector); This is only required for the TurboFan inline version; let's add this later in the CL for the TurboFan inlining.
https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... src/builtins/builtins-array.cc:2309: Callable less_than = CodeFactory::LessThan(assembler->isolate()); Ah, this is the issue: You use LessThan, which is generated (in the builtins) after this builtin. So the builtin table still contains Smi zero when you use it here. In this case you know that both length and index are either HeapNumber or Smi, so you can just convert both to Float64 and do a direct Float64LessThan comparison, that should be (a) more efficient and will (b) fix your bug.
Otherwise I think your implementation looks good (modulo the undefined check that can still be moved out of the common path). What's the performance so far?
Ok, I gave it a go. Very impressive, already down to 63ms in some cases, so comfortably beats the JS implementation even in the monomorphic case. https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... src/builtins/builtins-array.cc:2194: assembler->GotoIf(assembler->SmiBelow(index, length), &set_done); s/GotoIf/GotoUnless
On 2016/10/15 18:07:01, Benedikt Meurer wrote: > Ok, I gave it a go. Very impressive, already down to 63ms in some cases, so > comfortably beats the JS implementation even in the monomorphic case. > > https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... > File src/builtins/builtins-array.cc (right): > > https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... > src/builtins/builtins-array.cc:2194: > assembler->GotoIf(assembler->SmiBelow(index, length), &set_done); > s/GotoIf/GotoUnless With the changes you've suggested (and, the LessThan stub not being created yet should have been obvious, I guess I assumed it would be created when asked for by CodeFactory), I see similar results: ``` EchoBeach:v8 caitp$ out/x64.release/d8 test.js Time: 57ms. Time: 64ms. EchoBeach:v8 caitp$ tf_stringiter/x64.release/d8 test.js Time: 62ms. Time: 83ms. ``` where the monomorphic and polymorphic runs beat, or are at least close to the JS implementation. (tf_stringiter is an older build, so it may lack other patches that could benefit --- but it seems close to the results you were seeing). It looks pretty good at least for the fast cases, at least.
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/16 01:49:24, caitp wrote: > On 2016/10/15 18:07:01, Benedikt Meurer wrote: > > Ok, I gave it a go. Very impressive, already down to 63ms in some cases, so > > comfortably beats the JS implementation even in the monomorphic case. > > > > > https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... > > File src/builtins/builtins-array.cc (right): > > > > > https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... > > src/builtins/builtins-array.cc:2194: > > assembler->GotoIf(assembler->SmiBelow(index, length), &set_done); > > s/GotoIf/GotoUnless > > With the changes you've suggested (and, the LessThan stub not being created yet > should have been obvious, I guess I assumed it would be created when asked for > by CodeFactory), I see similar results: > > ``` > EchoBeach:v8 caitp$ out/x64.release/d8 test.js > Time: 57ms. > Time: 64ms. > EchoBeach:v8 caitp$ tf_stringiter/x64.release/d8 test.js > Time: 62ms. > Time: 83ms. > ``` > > where the monomorphic and polymorphic runs beat, or are at least close to the JS > implementation. (tf_stringiter is an older build, so it may lack other patches > that could benefit --- but it seems close to the results you were seeing). > > It looks pretty good at least for the fast cases, at least. The "generic" case does a lot worse than the fast JSArray case, unfortunately (~300ms in the TFJ case vs ~60ms in the JS case). hopefully the GetElement stub can do a better job of this in the future, though.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/14663) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
On 2016/10/16 02:37:24, caitp wrote: > On 2016/10/16 01:49:24, caitp wrote: > > On 2016/10/15 18:07:01, Benedikt Meurer wrote: > > > Ok, I gave it a go. Very impressive, already down to 63ms in some cases, so > > > comfortably beats the JS implementation even in the monomorphic case. > > > > > > > > > https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... > > > File src/builtins/builtins-array.cc (right): > > > > > > > > > https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... > > > src/builtins/builtins-array.cc:2194: > > > assembler->GotoIf(assembler->SmiBelow(index, length), &set_done); > > > s/GotoIf/GotoUnless > > > > With the changes you've suggested (and, the LessThan stub not being created > yet > > should have been obvious, I guess I assumed it would be created when asked for > > by CodeFactory), I see similar results: > > > > ``` > > EchoBeach:v8 caitp$ out/x64.release/d8 test.js > > Time: 57ms. > > Time: 64ms. > > EchoBeach:v8 caitp$ tf_stringiter/x64.release/d8 test.js > > Time: 62ms. > > Time: 83ms. > > ``` > > > > where the monomorphic and polymorphic runs beat, or are at least close to the > JS > > implementation. (tf_stringiter is an older build, so it may lack other patches > > that could benefit --- but it seems close to the results you were seeing). > > > > It looks pretty good at least for the fast cases, at least. > > The "generic" case does a lot worse than the fast JSArray case, unfortunately > (~300ms in the TFJ case vs ~60ms in the JS case). hopefully the GetElement stub > can do a better job of this in the future, though. That's probably the difference between the monomorphic LoadIC vs. the GetPropertyStub, most likely for "length".
On 2016/10/16 17:10:17, Benedikt Meurer wrote: > On 2016/10/16 02:37:24, caitp wrote: > > On 2016/10/16 01:49:24, caitp wrote: > > > On 2016/10/15 18:07:01, Benedikt Meurer wrote: > > > > Ok, I gave it a go. Very impressive, already down to 63ms in some cases, > so > > > > comfortably beats the JS implementation even in the monomorphic case. > > > > > > > > > > > > > > https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... > > > > File src/builtins/builtins-array.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... > > > > src/builtins/builtins-array.cc:2194: > > > > assembler->GotoIf(assembler->SmiBelow(index, length), &set_done); > > > > s/GotoIf/GotoUnless > > > > > > With the changes you've suggested (and, the LessThan stub not being created > > yet > > > should have been obvious, I guess I assumed it would be created when asked > for > > > by CodeFactory), I see similar results: > > > > > > ``` > > > EchoBeach:v8 caitp$ out/x64.release/d8 test.js > > > Time: 57ms. > > > Time: 64ms. > > > EchoBeach:v8 caitp$ tf_stringiter/x64.release/d8 test.js > > > Time: 62ms. > > > Time: 83ms. > > > ``` > > > > > > where the monomorphic and polymorphic runs beat, or are at least close to > the > > JS > > > implementation. (tf_stringiter is an older build, so it may lack other > patches > > > that could benefit --- but it seems close to the results you were seeing). > > > > > > It looks pretty good at least for the fast cases, at least. > > > > The "generic" case does a lot worse than the fast JSArray case, unfortunately > > (~300ms in the TFJ case vs ~60ms in the JS case). hopefully the GetElement > stub > > can do a better job of this in the future, though. > > That's probably the difference between the monomorphic LoadIC vs. the > GetPropertyStub, most likely for "length". What if length were always stored as an in-object property for JSObjects? it would help a lot for many internal algorithms. maybe not worth it for the extra word, but just a thought. so many functions in the standard lib make use of it.
On 2016/10/16 19:12:16, caitp wrote: > On 2016/10/16 17:10:17, Benedikt Meurer wrote: > > On 2016/10/16 02:37:24, caitp wrote: > > > On 2016/10/16 01:49:24, caitp wrote: > > > > On 2016/10/15 18:07:01, Benedikt Meurer wrote: > > > > > Ok, I gave it a go. Very impressive, already down to 63ms in some cases, > > so > > > > > comfortably beats the JS implementation even in the monomorphic case. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... > > > > > File src/builtins/builtins-array.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2405253006/diff/150001/src/builtins/builtins-... > > > > > src/builtins/builtins-array.cc:2194: > > > > > assembler->GotoIf(assembler->SmiBelow(index, length), &set_done); > > > > > s/GotoIf/GotoUnless > > > > > > > > With the changes you've suggested (and, the LessThan stub not being > created > > > yet > > > > should have been obvious, I guess I assumed it would be created when asked > > for > > > > by CodeFactory), I see similar results: > > > > > > > > ``` > > > > EchoBeach:v8 caitp$ out/x64.release/d8 test.js > > > > Time: 57ms. > > > > Time: 64ms. > > > > EchoBeach:v8 caitp$ tf_stringiter/x64.release/d8 test.js > > > > Time: 62ms. > > > > Time: 83ms. > > > > ``` > > > > > > > > where the monomorphic and polymorphic runs beat, or are at least close to > > the > > > JS > > > > implementation. (tf_stringiter is an older build, so it may lack other > > patches > > > > that could benefit --- but it seems close to the results you were seeing). > > > > > > > > It looks pretty good at least for the fast cases, at least. > > > > > > The "generic" case does a lot worse than the fast JSArray case, > unfortunately > > > (~300ms in the TFJ case vs ~60ms in the JS case). hopefully the GetElement > > stub > > > can do a better job of this in the future, though. > > > > That's probably the difference between the monomorphic LoadIC vs. the > > GetPropertyStub, most likely for "length". > > What if length were always stored as an in-object property for JSObjects? it > would help a lot for many internal algorithms. maybe not worth it for the extra > word, but just a thought. so many functions in the standard lib make use of it. I suppose most generic uses of the Array iterator will face an accessor length anyways, i.e. iterating a DOM NodeList, where the word wouldn't help you. But maybe it's worth having a specialized GetLengthStub at some point that has an internal cache for the last seen maps, and tries the most common cases for "length" first.
Awesome, very nice! Thanks a lot. Final round of comments; LGTM once comments addressed. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2093: assembler->Branch( Use CodeStubAssembler::IsJSReceiverInstanceType https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2112: } Nit: Add empty line between the }s https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2139: // Required, or else `throw_bad_receiver` fails a DCHECK due to thse variables Nit: s/thse/these https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2312: assembler->StoreObjectFieldNoWriteBarrier( The NumberInc can produce a HeapNumber and the iterator can be in old space, which means you need a write barrier here. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2349: Node* elements = assembler->LoadElements(array); You need to test whether the buffer is neutered here, and if so throw an exception. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2471: assembler->Bind(&did_set_done); It seems you don't need this did_set_done Label, as the var_value and var_done are bound initially exactly to these values, and are not altered along any path to this label. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:23: static void GotoIfIsDetachedBuffer(CodeStubAssembler* assembler, Nit: Remove static, it's implicitly static via the anonymous namespace. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins.cc File src/builtins/builtins.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins.... src/builtins/builtins.cc:89: if (!::strcmp(name, "ArrayIteratorPrototypeNext")) { Please remove this left over test code. https://codereview.chromium.org/2405253006/diff/190001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/isolate.cc#newcode... src/isolate.cc:1101: if (false && bootstrapper()->IsActive()) { Please undo this change. https://codereview.chromium.org/2405253006/diff/190001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2405253006/diff/190001/src/objects.h#newcode448 src/objects.h:448: V(JS_GENERIC_ARRAY_KEY_ITERATOR_TYPE) \ We still need a JS_FAST_ARRAY_KEY_ITERATOR_TYPE here, otherwise TurboFan will not be able to inline fast key iteration, where we can assume that "length" is JSArray::length.
https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2349: Node* elements = assembler->LoadElements(array); On 2016/10/17 04:02:42, Benedikt Meurer wrote: > You need to test whether the buffer is neutered here, and if so throw an > exception. I'm not sure about this. I think we should treat length as 0 if the buffer turns out to be neutered. That's how Chrome currently behaves anyways: ``` V8 version 5.5.0 (candidate) d8> var a = new Uint8Array([1,2,3,4,5]) undefined d8> %ArrayBufferNeuter(a.buffer) undefined d8> a.length 0 d8> [...a] [] ``` The spec should probably be rewritten as: ``` 8. If a has a [[TypedArrayName]] internal slot, then a. let `buffer` be a.[[ViewedArrayBuffer]]. b. If IsDetachedBuffer(buffer) is true, then i. let `len` be 0. c.else i. let `len` be a.[[ArrayLength]]. 9. else a. let `len` be ? ToLength(? Get(a, "length")). ``` This would match Chrome's current, behaviour, match the polyfillable behaviour that Babel/Traceur/TypeScript might use, and seems to match the intention of the spec.
On 2016/10/17 13:18:11, caitp wrote: > ``` > V8 version 5.5.0 (candidate) > d8> var a = new Uint8Array([1,2,3,4,5]) > undefined > d8> %ArrayBufferNeuter(a.buffer) > undefined > d8> a.length > 0 > d8> [...a] > [] > ``` Edit: that case should actually throw since it uses %TypedArray%.prototype[@@iterator], but we get the same behaviour with `[... Array.prototype[Symbol.iterator].call(a)]`, so the point stands, currently we treat the length as 0.
On 2016/10/17 13:26:37, caitp wrote: > On 2016/10/17 13:18:11, caitp wrote: > > ``` > > V8 version 5.5.0 (candidate) > > d8> var a = new Uint8Array([1,2,3,4,5]) > > undefined > > d8> %ArrayBufferNeuter(a.buffer) > > undefined > > d8> a.length > > 0 > > d8> [...a] > > [] > > ``` > > > Edit: that case should actually throw since it uses > %TypedArray%.prototype[@@iterator], but we get the same behaviour with `[... > Array.prototype[Symbol.iterator].call(a)]`, so the point stands, currently we > treat the length as 0. Agreed.
cbruni@chromium.org changed reviewers: + cbruni@chromium.org
some drive-by comments https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2504: CodeStubAssembler::INTPTR_PARAMETERS); nit: You can use LoadContextElement() here https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2524: CodeStubAssembler::INTPTR_PARAMETERS); nit: You can use LoadContextElement() here
https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2082: Variable var_array(assembler, MachineRepresentation::kTagged); Would anyone object to my adding an optional "name" parameter to the Variable constructor, to simplify figuring out what's wrong with the graph when a branch is reached without a value? It's useful locally, but maybe nobody else needs something like that.
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... File src/builtins/builtins-array.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2093: assembler->Branch( On 2016/10/17 04:02:42, Benedikt Meurer wrote: > Use CodeStubAssembler::IsJSReceiverInstanceType Done. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2112: } On 2016/10/17 04:02:42, Benedikt Meurer wrote: > Nit: Add empty line between the }s Done. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2139: // Required, or else `throw_bad_receiver` fails a DCHECK due to thse variables On 2016/10/17 04:02:42, Benedikt Meurer wrote: > Nit: s/thse/these Oops, done. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2312: assembler->StoreObjectFieldNoWriteBarrier( On 2016/10/17 04:02:42, Benedikt Meurer wrote: > The NumberInc can produce a HeapNumber and the iterator can be in old space, > which means you need a write barrier here. And now there is a write barrier. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2349: Node* elements = assembler->LoadElements(array); On 2016/10/17 13:18:11, caitp wrote: > On 2016/10/17 04:02:42, Benedikt Meurer wrote: > > You need to test whether the buffer is neutered here, and if so throw an > > exception. > > I'm not sure about this. I think we should treat length as 0 if the buffer turns > out to be neutered. That's how Chrome currently behaves anyways: > > ``` > V8 version 5.5.0 (candidate) > d8> var a = new Uint8Array([1,2,3,4,5]) > undefined > d8> %ArrayBufferNeuter(a.buffer) > undefined > d8> a.length > 0 > d8> [...a] > [] > ``` > > The spec should probably be rewritten as: > > ``` > 8. If a has a [[TypedArrayName]] internal slot, then > a. let `buffer` be a.[[ViewedArrayBuffer]]. > b. If IsDetachedBuffer(buffer) is true, then > i. let `len` be 0. > c.else > i. let `len` be a.[[ArrayLength]]. > 9. else > a. let `len` be ? ToLength(? Get(a, "length")). > ``` > > This would match Chrome's current, behaviour, match the polyfillable behaviour > that Babel/Traceur/TypeScript might use, and seems to match the intention of the > spec. the if_istypedarray path now treats length as 0 if the buffer is neutered. I _think_ that's compatible with SpiderMonkey at least, based on short discussion on IRC (but I'm not sure how I can actually test this myself to verify in SpiderMonkey). If it is compatible, it's probably the obvious spec bug fix. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2471: assembler->Bind(&did_set_done); On 2016/10/17 04:02:42, Benedikt Meurer wrote: > It seems you don't need this did_set_done Label, as the var_value and var_done > are bound initially exactly to these values, and are not altered along any path > to this label. It's gone https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-array.cc:2504: CodeStubAssembler::INTPTR_PARAMETERS); On 2016/10/17 14:56:39, Camillo Bruni wrote: > nit: You can use LoadContextElement() here Done, thanks for the tip https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:23: static void GotoIfIsDetachedBuffer(CodeStubAssembler* assembler, On 2016/10/17 04:02:42, Benedikt Meurer wrote: > Nit: Remove static, it's implicitly static via the anonymous namespace. I've just removed this helper and added CodeStubAssembler::IsDetachedBuffer(), since it's used by builtins-array.cc now too per the other comment thread. https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins.cc File src/builtins/builtins.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/builtins/builtins.... src/builtins/builtins.cc:89: if (!::strcmp(name, "ArrayIteratorPrototypeNext")) { On 2016/10/17 04:02:42, Benedikt Meurer wrote: > Please remove this left over test code. Done :x https://codereview.chromium.org/2405253006/diff/190001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2405253006/diff/190001/src/isolate.cc#newcode... src/isolate.cc:1101: if (false && bootstrapper()->IsActive()) { On 2016/10/17 04:02:42, Benedikt Meurer wrote: > Please undo this change. Done (again T_T). https://codereview.chromium.org/2405253006/diff/190001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2405253006/diff/190001/src/objects.h#newcode448 src/objects.h:448: V(JS_GENERIC_ARRAY_KEY_ITERATOR_TYPE) \ On 2016/10/17 04:02:42, Benedikt Meurer wrote: > We still need a JS_FAST_ARRAY_KEY_ITERATOR_TYPE here, otherwise TurboFan will > not be able to inline fast key iteration, where we can assume that "length" is > JSArray::length. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by caitp@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2405253006/#ps210001 (title: "latest round")
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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26687)
Description was changed from ========== [builtins] implement Array.prototype[@@iterator] in TFJ builtins Implements the variations of CreateArrayIterator() in TFJ builtins (ArrayPrototypeValues, ArrayPrototypeEntries and ArrayPrototypeKeys), and provides two new Object types with numerous maps which identify certain behaviours, which will be useful for inlining. Removes src/js/array-iterator.js entirely Also adds support for printing Symbol literals inserted by the Parser during desugaring when FLAG_print_builtin_ast is set to true. BUG=v8:5388 R=bmeurer@chromium.org, mstarzinger@chromium.org, adamk@chromium.org ========== to ========== [builtins] implement Array.prototype[@@iterator] in TFJ builtins Implements the variations of CreateArrayIterator() in TFJ builtins (ArrayPrototypeValues, ArrayPrototypeEntries and ArrayPrototypeKeys), and provides two new Object types with numerous maps which identify certain behaviours, which will be useful for inlining. Removes src/js/array-iterator.js entirely Also adds support for printing Symbol literals inserted by the Parser during desugaring when FLAG_print_builtin_ast is set to true. BUG=v8:5388 R=bmeurer@chromium.org, cbruni@chromium.org TBR=ulan@chromium.org ==========
The CQ bit was checked by caitp@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [builtins] implement Array.prototype[@@iterator] in TFJ builtins Implements the variations of CreateArrayIterator() in TFJ builtins (ArrayPrototypeValues, ArrayPrototypeEntries and ArrayPrototypeKeys), and provides two new Object types with numerous maps which identify certain behaviours, which will be useful for inlining. Removes src/js/array-iterator.js entirely Also adds support for printing Symbol literals inserted by the Parser during desugaring when FLAG_print_builtin_ast is set to true. BUG=v8:5388 R=bmeurer@chromium.org, cbruni@chromium.org TBR=ulan@chromium.org ========== to ========== [builtins] implement Array.prototype[@@iterator] in TFJ builtins Implements the variations of CreateArrayIterator() in TFJ builtins (ArrayPrototypeValues, ArrayPrototypeEntries and ArrayPrototypeKeys), and provides two new Object types with numerous maps which identify certain behaviours, which will be useful for inlining. Removes src/js/array-iterator.js entirely Also adds support for printing Symbol literals inserted by the Parser during desugaring when FLAG_print_builtin_ast is set to true. BUG=v8:5388 R=bmeurer@chromium.org, cbruni@chromium.org TBR=ulan@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #12 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== [builtins] implement Array.prototype[@@iterator] in TFJ builtins Implements the variations of CreateArrayIterator() in TFJ builtins (ArrayPrototypeValues, ArrayPrototypeEntries and ArrayPrototypeKeys), and provides two new Object types with numerous maps which identify certain behaviours, which will be useful for inlining. Removes src/js/array-iterator.js entirely Also adds support for printing Symbol literals inserted by the Parser during desugaring when FLAG_print_builtin_ast is set to true. BUG=v8:5388 R=bmeurer@chromium.org, cbruni@chromium.org TBR=ulan@chromium.org ========== to ========== [builtins] implement Array.prototype[@@iterator] in TFJ builtins Implements the variations of CreateArrayIterator() in TFJ builtins (ArrayPrototypeValues, ArrayPrototypeEntries and ArrayPrototypeKeys), and provides two new Object types with numerous maps which identify certain behaviours, which will be useful for inlining. Removes src/js/array-iterator.js entirely Also adds support for printing Symbol literals inserted by the Parser during desugaring when FLAG_print_builtin_ast is set to true. BUG=v8:5388 R=bmeurer@chromium.org, cbruni@chromium.org TBR=ulan@chromium.org Committed: https://crrev.com/86d0dd362f627a7831b042d4fe8baa121c976fc4 Cr-Commit-Position: refs/heads/master@{#40373} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/86d0dd362f627a7831b042d4fe8baa121c976fc4 Cr-Commit-Position: refs/heads/master@{#40373}
Message was sent while issue was closed.
This broke https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder... - no revert needed, but please fix asap |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
