Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2232063002:
[builtins] WIP: Array indexOf in TurboFan/Runtime (Closed)
|
Created:
4 years, 4 months ago by mattloring Modified:
4 years ago CC:
v8-reviews_googlegroups.com, Jakob Kummerow Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[builtins] Array indexOf in TurboFan/Runtime
Includes fast paths in the runtime for
DictionaryElementsAccessor, FastSmiOrObjectElementsAccessor,
FastDoubleElementsAccessor, TypedElementsAccessor, and
SloppyArgumentsElementsAccessor.
BUG=
Committed: https://crrev.com/da5d713d73d43aec66ff45c29d06b4857182578b
Cr-Commit-Position: refs/heads/master@{#38800}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Rebase #Patch Set 3 : Address comments #Patch Set 4 : Introducing the TF builtin #Patch Set 5 : Update test to look for new error message #
Total comments: 4
Patch Set 6 : Add elements fastpaths #
Total comments: 58
Patch Set 7 : Punctuation and improved error message #Patch Set 8 : Fix receiver dcheck #Patch Set 9 : Address remaining comments #Patch Set 10 : Fix improper use of Maybe #Patch Set 11 : Specialization for FastDoubleElementsAccessor #
Total comments: 10
Patch Set 12 : Eliminate specialized numeric path #Patch Set 13 : Move fast path FastElements -> FastSmiOrObjectElements #
Messages
Total messages: 68 (51 generated)
Description was changed from ========== [builtins] WIP: Array indexOf in TurboFan/Runtime BUG= ========== to ========== [builtins] WIP: Array indexOf in TurboFan/Runtime BUG= ==========
mattloring@google.com changed reviewers: + bmeurer@chromium.org, ofrobots@google.com
bmeurer@chromium.org changed reviewers: + cbruni@chromium.org
Adding Camillo for Array builtins review. I suspect he wants you to an ElementsAccessor version of that. TurboFan trampoline looks good so far ;-) https://codereview.chromium.org/2232063002/diff/1/src/js/typedarray.js File src/js/typedarray.js (right): https://codereview.chromium.org/2232063002/diff/1/src/js/typedarray.js#newcod... src/js/typedarray.js:573: You could do this change to typedarray.js as a separate CL. It has value beyond your change, as it also fixes some type feedback pollution caused by sharing InnerArrayIndexOf for both %TypedArray%.prototype.indexOf and and Array.prototype.indexOf. https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.cc File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:378: if (object->WouldConvertToSlowElements(index)) { You seem to undo a correctness fix here.
https://codereview.chromium.org/2232063002/diff/1/src/js/typedarray.js File src/js/typedarray.js (right): https://codereview.chromium.org/2232063002/diff/1/src/js/typedarray.js#newcod... src/js/typedarray.js:573: On 2016/08/11 04:02:24, Benedikt Meurer wrote: > You could do this change to typedarray.js as a separate CL. It has value beyond > your change, as it also fixes some type feedback pollution caused by sharing > InnerArrayIndexOf for both %TypedArray%.prototype.indexOf and and > Array.prototype.indexOf. Opened here: https://codereview.chromium.org/2243523002/
some nits https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.cc File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:556: isolate, object, Object::ToObject(isolate, handle(args[0], isolate))); nit: use args.at<Object>(0) https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:561: if (object->map()->instance_type() == JS_ARRAY_TYPE) { nit: object->IsJSArray() https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:580: if (len == 0) return *isolate->factory()->NewNumberFromInt64(-1); nit: return Smi::FromInt(-1); https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:589: if (fp > len) return *isolate->factory()->NewNumberFromInt64(-1); nit: return Smi::FromInt(-1); https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:603: // TODO(mattloring): specialize for reciever types as with include. Pretty please ;) https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:625: return *isolate->factory()->NewNumberFromInt64(-1); nit: return Smi::FromInt(-1);
https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.cc File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:378: if (object->WouldConvertToSlowElements(index)) { On 2016/08/11 04:02:24, Benedikt Meurer wrote: > You seem to undo a correctness fix here. Done. https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:556: isolate, object, Object::ToObject(isolate, handle(args[0], isolate))); On 2016/08/12 10:46:49, Camillo Bruni wrote: > nit: use args.at<Object>(0) Done. https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:561: if (object->map()->instance_type() == JS_ARRAY_TYPE) { On 2016/08/12 10:46:49, Camillo Bruni wrote: > nit: object->IsJSArray() Done. https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:580: if (len == 0) return *isolate->factory()->NewNumberFromInt64(-1); On 2016/08/12 10:46:49, Camillo Bruni wrote: > nit: return Smi::FromInt(-1); Done. https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:589: if (fp > len) return *isolate->factory()->NewNumberFromInt64(-1); On 2016/08/12 10:46:49, Camillo Bruni wrote: > nit: return Smi::FromInt(-1); Done. https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:625: return *isolate->factory()->NewNumberFromInt64(-1); On 2016/08/12 10:46:49, Camillo Bruni wrote: > nit: return Smi::FromInt(-1); Done.
LGTM on the C++ slow-path. Please send follow-up CLs / patches to jkummerow@ he has possesses the magic power to understand elements.cc as well.
The CQ bit was checked by mattloring@google.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_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
https://codereview.chromium.org/2232063002/diff/80001/test/webkit/fast/js/arr... File test/webkit/fast/js/array-prototype-properties-expected.txt (right): https://codereview.chromium.org/2232063002/diff/80001/test/webkit/fast/js/arr... test/webkit/fast/js/array-prototype-properties-expected.txt:44: PASS Array.prototype.indexOf.call(undefined, 0) threw exception TypeError: Cannot convert undefined or null to object. Alternatively, is there a way to modify the error generated by ASSIGN_RETURN_FAILURE_ON_EXCEPTION? https://codereview.chromium.org/2232063002/diff/80001/test/webkit/fast/js/arr... test/webkit/fast/js/array-prototype-properties-expected.txt:45: PASS Array.prototype.indlastIndexOfexOf.call(undefined, 0) threw exception TypeError: Cannot read property 'call' of undefined. While unrelated to this cl, this looks like a typo.
The CQ bit was checked by mattloring@google.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.
The CQ bit was checked by mattloring@google.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...
mattloring@google.com changed reviewers: + jkummerow@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patch set 6 needs more work. High-level comment: if you need excessive elements kind checks in FastElementsAccessor, you're holding it wrong; that's what its subclasses (e.g. FastHoleyDoubleElementsAccessor) are for. I'm fine with not changing that aspect for now though, seeing that you modeled this after the existing A.p.includes() implementation. They'll both need cleanup at some point. (I also realize that many of the nits below are copied; but if you choose to copy-paste lint-ridden code, you get to fix its issues :-P ) https://codereview.chromium.org/2232063002/diff/80001/test/webkit/fast/js/arr... File test/webkit/fast/js/array-prototype-properties-expected.txt (right): https://codereview.chromium.org/2232063002/diff/80001/test/webkit/fast/js/arr... test/webkit/fast/js/array-prototype-properties-expected.txt:44: PASS Array.prototype.indexOf.call(undefined, 0) threw exception TypeError: Cannot convert undefined or null to object. On 2016/08/16 16:57:32, mattloring wrote: > Alternatively, is there a way to modify the error generated by > ASSIGN_RETURN_FAILURE_ON_EXCEPTION? See BUILTIN(ArrayConcat) in builtins-array.cc for a workaround. https://codereview.chromium.org/2232063002/diff/80001/test/webkit/fast/js/arr... test/webkit/fast/js/array-prototype-properties-expected.txt:45: PASS Array.prototype.indlastIndexOfexOf.call(undefined, 0) threw exception TypeError: Cannot read property 'call' of undefined. On 2016/08/16 16:57:32, mattloring wrote: > While unrelated to this cl, this looks like a typo. It does. The test is correct insofar as an appropriate error message is both actually thrown and expected to be thrown, but it clearly doesn't test what it intends to test ;-) https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1618: // observable nit: trailing full stop. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1630: return Just(static_cast<int32_t>(k)); Who guarantees that k is in int32_t range? Also, nit: if (it doesn't fit on one line) { then it requires braces; } https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1645: return Just(static_cast<int32_t>(k)); nit: braces https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1647: // Bailout to slow path if elements on prototype changed nit: trailing full stop. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1653: // Continue if elements unchanged nit: trailing full stop. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1656: // Otherwise, bailout or update elements nit: trailing full stop. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2198: // FAST_HOLEY_DOUBLE_ELEMENTS can have `undefined` as a value. The comment below contradicts this, saying that FAST_HOLEY_DOUBLE_ELEMENTS cannot contain undefined (which I agree with; due to the HasProperty check the hole does not count as undefined for the purposes of indexOf()). If they can't, then neither can FAST_HOLEY_SMI_ELEMENTS, leaving only FAST_{,HOLEY_}ELEMENTS, and the code below can be simplified a lot. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2200: !IsFastHoleyElementsKind(Subclass::kind())) { This line can go. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2206: if (IsFastSmiOrObjectElementsKind(Subclass::kind())) { This check (including its comment) can go. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2207: auto elements = FixedArray::cast(receiver->elements()); nit: s/auto/FixedArray*/ https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2210: Object* element_k = elements->get(k); I'd inline this. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2211: if (IsFastObjectElementsKind(Subclass::kind()) && No elements kind check necessary here https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2217: } else { The entire else-block is unnecessary https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2222: } else if (!IsFastObjectElementsKind(Subclass::kind())) { This entire block is subsumed by the above. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2232: auto elements = FixedArray::cast(receiver->elements()); nit: s/auto/FixedArray*/ https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2248: if (!value->IsNaN()) { nit: reduce indentation by reversing order: // NaN can never be found by strict equality. if (value->IsNaN()) return Just(-1); // <continue with non-NaN case> https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2251: // Search for non-NaN Number in FAST_DOUBLE_ELEMENTS or I don't understand what value this comment adds? https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2254: auto elements = FixedDoubleArray::cast(receiver->elements()); nit: s/auto/FixedDoubleArray*/ https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2269: // and trust UCOMISD or similar operation for result nit: if you think this comment is useful enough to stay, add a trailing full stop. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2270: auto elements = FixedArray::cast(receiver->elements()); nit: s/auto/FixedArray*/ https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2811: // Integral types cannot represent +Inf or NaN nit: trailing full stop. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2818: // Return false if value can't be represented in this space nit: trailing full stop. s/space/ElementsKind/? https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2828: if (!std::isnan(search_value)) { Again, reverse the order, just return -1 early in case of NaN. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2830: double element_k = elements->get_scalar(k); s/double/ctype/; search_value should be of type |ctype| at this point too https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2831: if (element_k == search_value) return Just(static_cast<int32_t>(k)); Who guarantees that k is in int32_t range? https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:3141: return Just(static_cast<int32_t>(k)); nit: braces https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:3144: // Some mutation occurred in accessor. Abort "fast" path nit: trailing full stop. https://codereview.chromium.org/2232063002/diff/100001/src/elements.h File src/elements.h (right): https://codereview.chromium.org/2232063002/diff/100001/src/elements.h#newcode163 src/elements.h:163: // Check an Object's own elements for the index ofan element (using SameValue nit: s/ofan/of an/ https://codereview.chromium.org/2232063002/diff/100001/src/runtime/runtime-ar... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/2232063002/diff/100001/src/runtime/runtime-ar... src/runtime/runtime-array.cc:615: return Smi::FromInt(result.FromJust()); What if the result is outside Smi range?
Addressed first half of patch set 6 comments. https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.cc File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/2232063002/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:603: // TODO(mattloring): specialize for reciever types as with include. On 2016/08/12 10:46:49, Camillo Bruni (OOO) wrote: > Pretty please ;) Added in the most recent patch set. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1618: // observable On 2016/08/17 15:42:58, Jakob wrote: > nit: trailing full stop. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1645: return Just(static_cast<int32_t>(k)); On 2016/08/17 15:42:58, Jakob wrote: > nit: braces Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1647: // Bailout to slow path if elements on prototype changed On 2016/08/17 15:42:58, Jakob wrote: > nit: trailing full stop. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1653: // Continue if elements unchanged On 2016/08/17 15:42:58, Jakob wrote: > nit: trailing full stop. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1656: // Otherwise, bailout or update elements On 2016/08/17 15:42:59, Jakob wrote: > nit: trailing full stop. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2207: auto elements = FixedArray::cast(receiver->elements()); On 2016/08/17 15:42:58, Jakob wrote: > nit: s/auto/FixedArray*/ Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2210: Object* element_k = elements->get(k); On 2016/08/17 15:42:59, Jakob wrote: > I'd inline this. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2232: auto elements = FixedArray::cast(receiver->elements()); On 2016/08/17 15:42:58, Jakob wrote: > nit: s/auto/FixedArray*/ Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2254: auto elements = FixedDoubleArray::cast(receiver->elements()); On 2016/08/17 15:42:58, Jakob wrote: > nit: s/auto/FixedDoubleArray*/ Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2270: auto elements = FixedArray::cast(receiver->elements()); On 2016/08/17 15:42:59, Jakob wrote: > nit: s/auto/FixedArray*/ Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2811: // Integral types cannot represent +Inf or NaN On 2016/08/17 15:42:58, Jakob wrote: > nit: trailing full stop. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2818: // Return false if value can't be represented in this space On 2016/08/17 15:42:58, Jakob wrote: > nit: trailing full stop. s/space/ElementsKind/? Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:3141: return Just(static_cast<int32_t>(k)); On 2016/08/17 15:42:58, Jakob wrote: > nit: braces Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:3144: // Some mutation occurred in accessor. Abort "fast" path On 2016/08/17 15:42:58, Jakob wrote: > nit: trailing full stop. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.h File src/elements.h (right): https://codereview.chromium.org/2232063002/diff/100001/src/elements.h#newcode163 src/elements.h:163: // Check an Object's own elements for the index ofan element (using SameValue On 2016/08/17 15:42:59, Jakob wrote: > nit: s/ofan/of an/ Done.
The CQ bit was checked by mattloring@google.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/11098) 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 mattloring@google.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.
https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:1630: return Just(static_cast<int32_t>(k)); On 2016/08/17 15:42:58, Jakob wrote: > Who guarantees that k is in int32_t range? > > Also, nit: > > if (it doesn't fit on one line) { > then it requires braces; > } Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2198: // FAST_HOLEY_DOUBLE_ELEMENTS can have `undefined` as a value. On 2016/08/17 15:42:58, Jakob wrote: > The comment below contradicts this, saying that FAST_HOLEY_DOUBLE_ELEMENTS > cannot contain undefined (which I agree with; due to the HasProperty check the > hole does not count as undefined for the purposes of indexOf()). If they can't, > then neither can FAST_HOLEY_SMI_ELEMENTS, leaving only FAST_{,HOLEY_}ELEMENTS, > and the code below can be simplified a lot. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2200: !IsFastHoleyElementsKind(Subclass::kind())) { On 2016/08/17 15:42:59, Jakob wrote: > This line can go. Shouldn't the check for FAST_ELEMENTS/FAST_HOLEY_ELEMENTS remain since these are the cases where we can bail out without searching for undefined? https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2206: if (IsFastSmiOrObjectElementsKind(Subclass::kind())) { On 2016/08/17 15:42:58, Jakob wrote: > This check (including its comment) can go. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2211: if (IsFastObjectElementsKind(Subclass::kind()) && On 2016/08/17 15:42:58, Jakob wrote: > No elements kind check necessary here Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2217: } else { On 2016/08/17 15:42:58, Jakob wrote: > The entire else-block is unnecessary Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2222: } else if (!IsFastObjectElementsKind(Subclass::kind())) { On 2016/08/17 15:42:58, Jakob wrote: > This entire block is subsumed by the above. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2248: if (!value->IsNaN()) { On 2016/08/17 15:42:58, Jakob wrote: > nit: reduce indentation by reversing order: > > // NaN can never be found by strict equality. > if (value->IsNaN()) return Just(-1); > > // <continue with non-NaN case> Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2251: // Search for non-NaN Number in FAST_DOUBLE_ELEMENTS or On 2016/08/17 15:42:58, Jakob wrote: > I don't understand what value this comment adds? Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2269: // and trust UCOMISD or similar operation for result On 2016/08/17 15:42:59, Jakob wrote: > nit: if you think this comment is useful enough to stay, add a trailing full > stop. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2828: if (!std::isnan(search_value)) { On 2016/08/17 15:42:58, Jakob wrote: > Again, reverse the order, just return -1 early in case of NaN. Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2830: double element_k = elements->get_scalar(k); On 2016/08/17 15:42:58, Jakob wrote: > s/double/ctype/; search_value should be of type |ctype| at this point too Done. https://codereview.chromium.org/2232063002/diff/100001/src/elements.cc#newcod... src/elements.cc:2831: if (element_k == search_value) return Just(static_cast<int32_t>(k)); On 2016/08/17 15:42:59, Jakob wrote: > Who guarantees that k is in int32_t range? Done. https://codereview.chromium.org/2232063002/diff/100001/src/runtime/runtime-ar... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/2232063002/diff/100001/src/runtime/runtime-ar... src/runtime/runtime-array.cc:615: return Smi::FromInt(result.FromJust()); On 2016/08/17 15:42:59, Jakob wrote: > What if the result is outside Smi range? Done.
The CQ bit was checked by mattloring@google.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...)
The CQ bit was checked by mattloring@google.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.
The CQ bit was checked by mattloring@google.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.
LGTM with two more suggestions, and two non-actionable remarks. https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... src/elements.cc:2195: if (!value->IsNumber()) { Try this simplification: // Only FAST_{,HOLEY_}ELEMENTS can store non-numbers. if (!value->IsNumber() && !IsFastObjectElementsKind(Subclass::kind())) { return Just<int64_t>(-1); } // NaN can never be found by strict equality. if (value->IsNaN()) return Just<int64_t>(-1); FixedArray* elements = FixedArray::cast(receiver->elements()); for (uint32_t k = start_from; k < length; ++k) { if (value->StrictEquals(elements->get(k))) return Just<int64_t>(k); } return Just<int64_t>(-1); I'd special-case numbers only if it makes a measurable performance difference. Does it? Also, now that this is not correct for FAST_DOUBLE_ELEMENTS any more, it should move to the FastSmiOrObjectElementsAccessor class. https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... src/elements.cc:2199: if (!IsFastElementsKind(Subclass::kind()) && This is the FastElementsAccessor. All Subclass::kind()s are fast. ("IsFastElementsKind(kind)" is very different from "kind == FAST_ELEMENTS"!) https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... src/elements.cc:2588: if (elements->is_the_hole(k)) { (Just for the record: if everything does fit on one line, e.g. "if (foo) bar;", then keeping it on one line without braces is fine. That said, splitting it into two lines and adding braces like you've done here is fine too.) https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... src/elements.cc:2826: if (element_k == search_value) return Just<int64_t>(k); This still does an implicit cast to double, which is silly for int/uint typed elements. I'd suggest: ctype typed_search_value = static_cast<ctype>(search_value); if (static_cast<double>(typed_search_value) != search_value) { return Just<int64_t>(-1); // Loss of precision. } outside the loop, and then comparing against typed_search_value.
https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... src/elements.cc:2195: if (!value->IsNumber()) { On 2016/08/19 09:49:56, Jakob wrote: > Try this simplification: > > // Only FAST_{,HOLEY_}ELEMENTS can store non-numbers. > if (!value->IsNumber() && > !IsFastObjectElementsKind(Subclass::kind())) { > return Just<int64_t>(-1); > } > // NaN can never be found by strict equality. > if (value->IsNaN()) return Just<int64_t>(-1); > > FixedArray* elements = FixedArray::cast(receiver->elements()); > for (uint32_t k = start_from; k < length; ++k) { > if (value->StrictEquals(elements->get(k))) return Just<int64_t>(k); > } > return Just<int64_t>(-1); > > I'd special-case numbers only if it makes a measurable performance difference. > Does it? > > Also, now that this is not correct for FAST_DOUBLE_ELEMENTS any more, it should > move to the FastSmiOrObjectElementsAccessor class. I did not observe a performance difference so I've eliminated the number specific path. In what way is this incorrect for FAST_DOUBLE_ELEMENTS? https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... src/elements.cc:2199: if (!IsFastElementsKind(Subclass::kind()) && On 2016/08/19 09:49:56, Jakob wrote: > This is the FastElementsAccessor. All Subclass::kind()s are fast. > ("IsFastElementsKind(kind)" is very different from "kind == FAST_ELEMENTS"!) Thank you for the clarification. This check is no longer needed in the updated function. https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... src/elements.cc:2588: if (elements->is_the_hole(k)) { On 2016/08/19 09:49:56, Jakob wrote: > (Just for the record: if everything does fit on one line, e.g. "if (foo) bar;", > then keeping it on one line without braces is fine. That said, splitting it into > two lines and adding braces like you've done here is fine too.) Ack. Thanks for clarifying. https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... src/elements.cc:2826: if (element_k == search_value) return Just<int64_t>(k); On 2016/08/19 09:49:56, Jakob wrote: > This still does an implicit cast to double, which is silly for int/uint typed > elements. I'd suggest: > > ctype typed_search_value = static_cast<ctype>(search_value); > if (static_cast<double>(typed_search_value) != search_value) { > return Just<int64_t>(-1); // Loss of precision. > } > > outside the loop, and then comparing against typed_search_value. Done.
The CQ bit was checked by mattloring@google.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.
LGTM, but please move that method to the FastSmiOrObjectElementsAccessor class. Also, please don't forget to update the CL description before landing. https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... src/elements.cc:2195: if (!value->IsNumber()) { On 2016/08/19 17:22:33, mattloring wrote: > In what way is this incorrect for FAST_DOUBLE_ELEMENTS? FixedArray::cast(receiver->elements()) only works for smi/object elements; for double elements it would have to be FixedDoubleArray::cast(...).
The CQ bit was checked by mattloring@google.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...
Description was changed from ========== [builtins] WIP: Array indexOf in TurboFan/Runtime BUG= ========== to ========== [builtins] Array indexOf in TurboFan/Runtime Includes fast paths in the runtime for DictionaryElementsAccessor, FastSmiOrObjectElementsAccessor, FastDoubleElementsAccessor, TypedElementsAccessor, and SloppyArgumentsElementsAccessor. BUG= ==========
Description was changed from ========== [builtins] Array indexOf in TurboFan/Runtime Includes fast paths in the runtime for DictionaryElementsAccessor, FastSmiOrObjectElementsAccessor, FastDoubleElementsAccessor, TypedElementsAccessor, and SloppyArgumentsElementsAccessor. BUG= ========== to ========== [builtins] Array indexOf in TurboFan/Runtime Includes fast paths in the runtime for DictionaryElementsAccessor, FastSmiOrObjectElementsAccessor, FastDoubleElementsAccessor, TypedElementsAccessor, and SloppyArgumentsElementsAccessor. BUG= ==========
On 2016/08/22 09:54:10, Jakob wrote: > LGTM, but please move that method to the FastSmiOrObjectElementsAccessor class. > > Also, please don't forget to update the CL description before landing. > > https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc > File src/elements.cc (right): > > https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... > src/elements.cc:2195: if (!value->IsNumber()) { > On 2016/08/19 17:22:33, mattloring wrote: > > In what way is this incorrect for FAST_DOUBLE_ELEMENTS? > > FixedArray::cast(receiver->elements()) only works for smi/object elements; for > double elements it would have to be FixedDoubleArray::cast(...). Method moved and description updated.
https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2232063002/diff/200001/src/elements.cc#newcod... src/elements.cc:2195: if (!value->IsNumber()) { On 2016/08/22 09:54:10, Jakob wrote: > On 2016/08/19 17:22:33, mattloring wrote: > > In what way is this incorrect for FAST_DOUBLE_ELEMENTS? > > FixedArray::cast(receiver->elements()) only works for smi/object elements; for > double elements it would have to be FixedDoubleArray::cast(...). > Ah ok, that makes sense. Thanks!
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 mattloring@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org, jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2232063002/#ps240001 (title: "Move fast path FastElements -> FastSmiOrObjectElements")
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] Array indexOf in TurboFan/Runtime Includes fast paths in the runtime for DictionaryElementsAccessor, FastSmiOrObjectElementsAccessor, FastDoubleElementsAccessor, TypedElementsAccessor, and SloppyArgumentsElementsAccessor. BUG= ========== to ========== [builtins] Array indexOf in TurboFan/Runtime Includes fast paths in the runtime for DictionaryElementsAccessor, FastSmiOrObjectElementsAccessor, FastDoubleElementsAccessor, TypedElementsAccessor, and SloppyArgumentsElementsAccessor. BUG= ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [builtins] Array indexOf in TurboFan/Runtime Includes fast paths in the runtime for DictionaryElementsAccessor, FastSmiOrObjectElementsAccessor, FastDoubleElementsAccessor, TypedElementsAccessor, and SloppyArgumentsElementsAccessor. BUG= ========== to ========== [builtins] Array indexOf in TurboFan/Runtime Includes fast paths in the runtime for DictionaryElementsAccessor, FastSmiOrObjectElementsAccessor, FastDoubleElementsAccessor, TypedElementsAccessor, and SloppyArgumentsElementsAccessor. BUG= Committed: https://crrev.com/da5d713d73d43aec66ff45c29d06b4857182578b Cr-Commit-Position: refs/heads/master@{#38800} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/da5d713d73d43aec66ff45c29d06b4857182578b Cr-Commit-Position: refs/heads/master@{#38800}
Message was sent while issue was closed.
Patchset #14 (id:260001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
