Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243523002:
[builtins] IndexOf/LastIndexOf implementation for typedarrays (Closed)
|
Created:
4 years, 4 months ago by mattloring Modified:
4 years, 4 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[builtins] IndexOf/LastIndexOf implementation for typedarrays
Removes dependence on InnerArrayIndexOf/InnerArrayLastIndexOf and
reduces type polution caused by sharing these functions between standard
and typed arrays.
BUG=
Committed: https://crrev.com/b052909c2522857ef14440894523b5ab6defd7ea
Cr-Commit-Position: refs/heads/master@{#38637}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Ensure -0 is never returned #
Total comments: 1
Messages
Total messages: 26 (16 generated)
Description was changed from ========== IndexOf/LastIndexOf implementation for typedarrays Removes dependence on InnerArrayIndexOf/InnerArrayLastIndexOf and reduces type polution caused by sharing these functions between standard and typed arrays. BUG= ========== to ========== IndexOf/LastIndexOf implementation for typedarrays Removes dependence on InnerArrayIndexOf/InnerArrayLastIndexOf and reduces type polution caused by sharing these functions between standard and typed arrays. BUG= ==========
mattloring@google.com changed reviewers: + bmeurer@chromium.org, cbruni@chromium.org, ofrobots@google.com
Description was changed from ========== IndexOf/LastIndexOf implementation for typedarrays Removes dependence on InnerArrayIndexOf/InnerArrayLastIndexOf and reduces type polution caused by sharing these functions between standard and typed arrays. BUG= ========== to ========== [builtins] IndexOf/LastIndexOf implementation for typedarrays Removes dependence on InnerArrayIndexOf/InnerArrayLastIndexOf and reduces type polution caused by sharing these functions between standard and typed arrays. BUG= ==========
Thanks. LGTM.
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/10855) 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...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6883) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
LGTM with nits: see my comments (mostly improvements to your existing code) https://codereview.chromium.org/2243523002/diff/1/src/js/typedarray.js File src/js/typedarray.js (right): https://codereview.chromium.org/2243523002/diff/1/src/js/typedarray.js#newcod... src/js/typedarray.js:574: If the input is not a number return -1: if (!IS_NUMBER(element)) return -1; https://codereview.chromium.org/2243523002/diff/1/src/js/typedarray.js#newcod... src/js/typedarray.js:587: if (k in this && element === elementK) { This "in" check is not necessary since the typed array is packed and has no accessors that could modify it's length when reading out an element. https://codereview.chromium.org/2243523002/diff/1/src/js/typedarray.js#newcod... src/js/typedarray.js:609: } Add: if (!IS_NUMBER(element)) return -1; https://codereview.chromium.org/2243523002/diff/1/src/js/typedarray.js#newcod... src/js/typedarray.js:624: if (k in this && element === elementK) { same here: you can remove the "in" check.
I added extra logic to ensure -0 is never returned. Is there a standard way that 0 is normalized by other runtime functions or are these checks preferred? https://codereview.chromium.org/2243523002/diff/1/src/js/typedarray.js File src/js/typedarray.js (right): https://codereview.chromium.org/2243523002/diff/1/src/js/typedarray.js#newcod... src/js/typedarray.js:574: On 2016/08/12 09:35:17, Camillo Bruni wrote: > If the input is not a number return -1: > if (!IS_NUMBER(element)) return -1; Done. https://codereview.chromium.org/2243523002/diff/1/src/js/typedarray.js#newcod... src/js/typedarray.js:587: if (k in this && element === elementK) { On 2016/08/12 09:35:17, Camillo Bruni wrote: > This "in" check is not necessary since the typed array is packed and has no > accessors that could modify it's length when reading out an element. Done. https://codereview.chromium.org/2243523002/diff/1/src/js/typedarray.js#newcod... src/js/typedarray.js:609: } On 2016/08/12 09:35:17, Camillo Bruni wrote: > Add: if (!IS_NUMBER(element)) return -1; Done. https://codereview.chromium.org/2243523002/diff/1/src/js/typedarray.js#newcod... src/js/typedarray.js:624: if (k in this && element === elementK) { On 2016/08/12 09:35:17, Camillo Bruni wrote: > same here: you can remove the "in" check. 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.
The CQ bit was checked by mattloring@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/2243523002/#ps20001 (title: "Ensure -0 is never returned")
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] IndexOf/LastIndexOf implementation for typedarrays Removes dependence on InnerArrayIndexOf/InnerArrayLastIndexOf and reduces type polution caused by sharing these functions between standard and typed arrays. BUG= ========== to ========== [builtins] IndexOf/LastIndexOf implementation for typedarrays Removes dependence on InnerArrayIndexOf/InnerArrayLastIndexOf and reduces type polution caused by sharing these functions between standard and typed arrays. BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [builtins] IndexOf/LastIndexOf implementation for typedarrays Removes dependence on InnerArrayIndexOf/InnerArrayLastIndexOf and reduces type polution caused by sharing these functions between standard and typed arrays. BUG= ========== to ========== [builtins] IndexOf/LastIndexOf implementation for typedarrays Removes dependence on InnerArrayIndexOf/InnerArrayLastIndexOf and reduces type polution caused by sharing these functions between standard and typed arrays. BUG= Committed: https://crrev.com/b052909c2522857ef14440894523b5ab6defd7ea Cr-Commit-Position: refs/heads/master@{#38637} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b052909c2522857ef14440894523b5ab6defd7ea Cr-Commit-Position: refs/heads/master@{#38637}
Message was sent while issue was closed.
adamk@chromium.org changed reviewers: + adamk@chromium.org
Message was sent while issue was closed.
I'm curious what motivated this particular change. Is it a pre-requisite for moving TypedArray methods to C++? Array methods are already designed (spec-wise) to be type polluted due to being applicable to any array-like receiver, so even though this reduces the type pollution in this specific case it doesn't eliminate the possibility of type pollution in general. https://codereview.chromium.org/2243523002/diff/20001/src/js/typedarray.js File src/js/typedarray.js (left): https://codereview.chromium.org/2243523002/diff/20001/src/js/typedarray.js#ol... src/js/typedarray.js:82: InnerArrayIndexOf = from.InnerArrayIndexOf; I suspect these were the last importers of these functions, so array.js should probably be refactored to stop exporting them (and merge them into their single caller)
Message was sent while issue was closed.
On 2016/08/15 20:38:54, adamk wrote: > I'm curious what motivated this particular change. Is it a pre-requisite for > moving TypedArray methods to C++? > > Array methods are already designed (spec-wise) to be type polluted due to being > applicable to any array-like receiver, so even though this reduces the type > pollution in this specific case it doesn't eliminate the possibility of type > pollution in general. > This was a prerequisite refactor to making Array.prototype.indexOf a TF builtin which is being done here: https://codereview.chromium.org/2232063002. The type pollution was raised as a concern during code review here: https://codereview.chromium.org/2232063002/patch/1/10006. > https://codereview.chromium.org/2243523002/diff/20001/src/js/typedarray.js > File src/js/typedarray.js (left): > > https://codereview.chromium.org/2243523002/diff/20001/src/js/typedarray.js#ol... > src/js/typedarray.js:82: InnerArrayIndexOf = from.InnerArrayIndexOf; > I suspect these were the last importers of these functions, so array.js should > probably be refactored to stop exporting them (and merge them into their single > caller) I can take a look and, if they are no longer imported, remove them.
Message was sent while issue was closed.
On 2016/08/15 22:50:13, mattloring wrote: > On 2016/08/15 20:38:54, adamk wrote: > > I'm curious what motivated this particular change. Is it a pre-requisite for > > moving TypedArray methods to C++? > > > > Array methods are already designed (spec-wise) to be type polluted due to > being > > applicable to any array-like receiver, so even though this reduces the type > > pollution in this specific case it doesn't eliminate the possibility of type > > pollution in general. > > > > This was a prerequisite refactor to making Array.prototype.indexOf a TF builtin > which > is being done here: https://codereview.chromium.org/2232063002. The type > pollution was > raised as a concern during code review here: > https://codereview.chromium.org/2232063002/patch/1/10006. Ah, thanks for the context. > > https://codereview.chromium.org/2243523002/diff/20001/src/js/typedarray.js > > File src/js/typedarray.js (left): > > > > > https://codereview.chromium.org/2243523002/diff/20001/src/js/typedarray.js#ol... > > src/js/typedarray.js:82: InnerArrayIndexOf = from.InnerArrayIndexOf; > > I suspect these were the last importers of these functions, so array.js should > > probably be refactored to stop exporting them (and merge them into their > single > > caller) > > I can take a look and, if they are no longer imported, remove them. Thanks, though it looks like your other CL just deletes it anyway? Ok to wait for that to land if it's coming soon. |
