Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15504002:
Array.observe emit splices for array length change and update index >= length (Closed)
|
Created:
7 years, 7 months ago by rafaelw Modified:
7 years, 6 months ago CC:
v8-dev Visibility:
Public. |
DescriptionArray.observe emit splices for array length change and update index >= length
R=adamk@chromium.org, rossberg@chromium.org
Committed: https://code.google.com/p/v8/source/detail?r=14944
Patch Set 1 #Patch Set 2 : remove todos #
Total comments: 18
Patch Set 3 : cr changes #Patch Set 4 : take JSArrays #Patch Set 5 : missed header files (JSArray) #
Total comments: 17
Patch Set 6 : cr comments #
Total comments: 2
Patch Set 7 : whitespace #
Total comments: 23
Patch Set 8 : cr changes #Patch Set 9 : cr comments #
Messages
Total messages: 15 (0 generated)
https://codereview.chromium.org/15504002/diff/2001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode1993 src/objects.cc:1993: uint32_t index, Nit: indentation seems off here. https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode1999 src/objects.cc:1999: if (object->IsJSGlobalObject()) { Can this even happen? Do we ever emit splices for non-arrays? https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode2004 src/objects.cc:2004: Handle<Object> delete_count_object = It's not obvious why we need the delete_count here...why don't we just pass the correct deleted array? https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode2022 src/objects.cc:2022: if (object->IsJSGlobalObject()) { Same question as above, can this happen? https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode2038 src/objects.cc:2038: if (object->IsJSGlobalObject()) { ditto https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode10811 src/objects.cc:10811: int_indices.Add(i); It seems odd to keep track of the indices twice. Any reason not to store them only as ints and then generate strings below when needed? https://codereview.chromium.org/15504002/diff/2001/test/mjsunit/harmony/objec... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/15504002/diff/2001/test/mjsunit/harmony/objec... test/mjsunit/harmony/object-observe.js:1040: assertSame(spliceRecords.length, 1); Any reason not to use assertEquals() for everything except object identity? https://codereview.chromium.org/15504002/diff/2001/test/mjsunit/harmony/objec... test/mjsunit/harmony/object-observe.js:1050: assertSame(splice.removed[499999900], 'hello'); Probably also want to check splice.removed.length (which should be 999999900)
All done. https://codereview.chromium.org/15504002/diff/2001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode1993 src/objects.cc:1993: uint32_t index, On 2013/05/20 23:13:13, adamk wrote: > Nit: indentation seems off here. Done. https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode1999 src/objects.cc:1999: if (object->IsJSGlobalObject()) { Replaced with assert. On 2013/05/20 23:13:13, adamk wrote: > Can this even happen? Do we ever emit splices for non-arrays? https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode2004 src/objects.cc:2004: Handle<Object> delete_count_object = You are correct. This is an artifact of having split the original (large) patch into smaller ones. The receiver side of EnqueueSpliceRecord has already landed, and there are two patches in flight that depend on that signature. I'll follow-up with a clean-up and remove the delete_count argument. On 2013/05/20 23:13:13, adamk wrote: > It's not obvious why we need the delete_count here...why don't we just pass the > correct deleted array? https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode2022 src/objects.cc:2022: if (object->IsJSGlobalObject()) { On 2013/05/20 23:13:13, adamk wrote: > Same question as above, can this happen? Done. https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode2038 src/objects.cc:2038: if (object->IsJSGlobalObject()) { On 2013/05/20 23:13:13, adamk wrote: > ditto Done. https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode10811 src/objects.cc:10811: int_indices.Add(i); On 2013/05/20 23:13:13, adamk wrote: > It seems odd to keep track of the indices twice. Any reason not to store them > only as ints and then generate strings below when needed? Done. https://codereview.chromium.org/15504002/diff/2001/test/mjsunit/harmony/objec... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/15504002/diff/2001/test/mjsunit/harmony/objec... test/mjsunit/harmony/object-observe.js:1040: assertSame(spliceRecords.length, 1); On 2013/05/20 23:13:13, adamk wrote: > Any reason not to use assertEquals() for everything except object identity? Done. https://codereview.chromium.org/15504002/diff/2001/test/mjsunit/harmony/objec... test/mjsunit/harmony/object-observe.js:1050: assertSame(splice.removed[499999900], 'hello'); On 2013/05/20 23:13:13, adamk wrote: > Probably also want to check splice.removed.length (which should be 999999900) Done.
https://codereview.chromium.org/15504002/diff/2001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode1999 src/objects.cc:1999: if (object->IsJSGlobalObject()) { On 2013/05/20 23:52:06, rafaelw wrote: > Replaced with assert. Now that you've added the assert I'm wondering what happens with: var global = this; global.length = 0; Array.prototype.push.call(global, 75); So perhaps I was wrong... > On 2013/05/20 23:13:13, adamk wrote: > > Can this even happen? Do we ever emit splices for non-arrays? >
ptal https://codereview.chromium.org/15504002/diff/2001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/2001/src/objects.cc#newcode1999 src/objects.cc:1999: if (object->IsJSGlobalObject()) { These only get called from within objects.cc, but I've made it so that all three take Handle<JSArray> On 2013/05/21 19:11:11, adamk wrote: > On 2013/05/20 23:52:06, rafaelw wrote: > > Replaced with assert. > > Now that you've added the assert I'm wondering what happens with: > > var global = this; > global.length = 0; > Array.prototype.push.call(global, 75); > > So perhaps I was wrong... > > > On 2013/05/20 23:13:13, adamk wrote: > > > Can this even happen? Do we ever emit splices for non-arrays? > > >
https://codereview.chromium.org/15504002/diff/16001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode1992 src/objects.cc:1992: v8 style nit: need two blank lines between functions. https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode1996 src/objects.cc:1996: uint32_t delete_count, I know we discussed this before, but can you avoid passing delete_count around? I don't think you even need to set length explicitly, since you're going through SetElement() anyway (which will update the length appropriately). https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode2011 src/objects.cc:2011: isolate->factory()->undefined_value(), 5, args, I'd use ARRAY_SIZE(args) here, and below. https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode2015 src/objects.cc:2015: same whitespace nit, and so on below https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode11010 src/objects.cc:11010: return *hresult; v8 style puts this on the previous line https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode11025 src/objects.cc:11025: uint32_t index = new_length > old_length ? old_length : new_length; this would be slightly more readable as min(old_length, new_length) https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode11030 src/objects.cc:11030: for (int i = 0; i < indices.length(); ++i) Note that these indices are in descending order due to the way they were generated. So you might be better off iterating back-to-front. https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode12094 src/objects.cc:12094: CHECK(new_length_handle->ToArrayIndex(&new_length)); We should really find a better way to avoid dropping CHECKs everywhere. Not in this patch, though. https://codereview.chromium.org/15504002/diff/16001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/15504002/diff/16001/src/objects.h#newcode2403 src/objects.h:2403: static void EnqueueSpliceRecord(Handle<JSArray> object, Is there any need to expose these as part of the JSObject interface? If they're only used in objects.cc, they could just be free file-scoped functions instead. I mention it because now that they take JSArrays, they seem a bit odd in JSObject.
https://codereview.chromium.org/15504002/diff/16001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode1992 src/objects.cc:1992: On 2013/05/24 00:15:30, adamk wrote: > v8 style nit: need two blank lines between functions. Done. https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode1996 src/objects.cc:1996: uint32_t delete_count, I've put a todo to go back and remove it. There are now three CLs pending that aren't dependent on each other. I prefer to land them and then go back and clean this up. On 2013/05/24 00:15:30, adamk wrote: > I know we discussed this before, but can you avoid passing delete_count around? > I don't think you even need to set length explicitly, since you're going through > SetElement() anyway (which will update the length appropriately). https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode2011 src/objects.cc:2011: isolate->factory()->undefined_value(), 5, args, On 2013/05/24 00:15:30, adamk wrote: > I'd use ARRAY_SIZE(args) here, and below. Done. https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode2015 src/objects.cc:2015: On 2013/05/24 00:15:30, adamk wrote: > same whitespace nit, and so on below Done. https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode11010 src/objects.cc:11010: return *hresult; On 2013/05/24 00:15:30, adamk wrote: > v8 style puts this on the previous line Done. https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode11025 src/objects.cc:11025: uint32_t index = new_length > old_length ? old_length : new_length; On 2013/05/24 00:15:30, adamk wrote: > this would be slightly more readable as min(old_length, new_length) Done. https://codereview.chromium.org/15504002/diff/16001/src/objects.cc#newcode11030 src/objects.cc:11030: for (int i = 0; i < indices.length(); ++i) On 2013/05/24 00:15:30, adamk wrote: > Note that these indices are in descending order due to the way they were > generated. So you might be better off iterating back-to-front. Done. https://codereview.chromium.org/15504002/diff/16001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/15504002/diff/16001/src/objects.h#newcode2403 src/objects.h:2403: static void EnqueueSpliceRecord(Handle<JSArray> object, On 2013/05/24 00:15:30, adamk wrote: > Is there any need to expose these as part of the JSObject interface? If they're > only used in objects.cc, they could just be free file-scoped functions instead. > I mention it because now that they take JSArrays, they seem a bit odd in > JSObject. Done.
lgtm with one more whitespace nit (will of course need review from rossberg) https://codereview.chromium.org/15504002/diff/23001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/23001/src/objects.cc#newcode10951 src/objects.cc:10951: needs moar whitespace
https://codereview.chromium.org/15504002/diff/23001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/23001/src/objects.cc#newcode10951 src/objects.cc:10951: On 2013/05/24 21:39:35, adamk wrote: > needs moar whitespace Done.
https://codereview.chromium.org/15504002/diff/33001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11031 src/objects.cc:11031: uint32_t add_count = new_length > old_length ? new_length - old_length : 0; Max(new_length - old_length, 0) https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11032 src/objects.cc:11032: uint32_t delete_count = new_length < old_length ? old_length - new_length : 0; Max(old_length - new_length, 0) https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11034 src/objects.cc:11034: if (delete_count) { Nit: delete_count > 0 https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11036 src/objects.cc:11036: while (i--) { Can we make this a for loop? https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode12069 src/objects.cc:12069: Handle<Object> new_length_handle; Move this and new_length into conditional below, to avoid additional register pressure on ia32. https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode12095 src/objects.cc:12095: bool array_extended = false; Can we initialize this properly instead of mutating later, i.e.: bool array_extended = self->IsJSArray() && ...; But actually, I'd simplify control flow by making it one 'if/else' and duplicate the single EnqueueChangeRecord call. https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js File src/v8natives.js (right): https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js#newcode929 src/v8natives.js:929: new_length < old_length ? new_length : old_length, Perhaps use Math.min https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js#newcode932 src/v8natives.js:932: new_length > old_length ? new_length - old_length : 0); Perhaps use Math.max https://codereview.chromium.org/15504002/diff/33001/test/mjsunit/harmony/obje... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/15504002/diff/33001/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:1005: { object: arr3, name: 'length', type: 'reconfigured' } Not sure I understand, why is it correct that this a separate change now?
PTAL. Also, please land if lgtm. https://codereview.chromium.org/15504002/diff/33001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11031 src/objects.cc:11031: uint32_t add_count = new_length > old_length ? new_length - old_length : 0; new_length and old_length are both uint so, without casting, (new_length - old_length) is never going to be less than 0. I could cast to ints, but I'm not sure that buys much since Max is just a macro. Leaving for now. Please clarify if you want something else. On 2013/05/27 14:48:41, rossberg wrote: > Max(new_length - old_length, 0) https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11032 src/objects.cc:11032: uint32_t delete_count = new_length < old_length ? old_length - new_length : 0; Same here. On 2013/05/27 14:48:41, rossberg wrote: > Max(old_length - new_length, 0) https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11034 src/objects.cc:11034: if (delete_count) { On 2013/05/27 14:48:41, rossberg wrote: > Nit: delete_count > 0 Done. https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11036 src/objects.cc:11036: while (i--) { I'm curious why you prefer the for loop, but I've changed it for you. On 2013/05/27 14:48:41, rossberg wrote: > Can we make this a for loop? https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode12069 src/objects.cc:12069: Handle<Object> new_length_handle; On 2013/05/27 14:48:41, rossberg wrote: > Move this and new_length into conditional below, to avoid additional register > pressure on ia32. Done. https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode12095 src/objects.cc:12095: bool array_extended = false; On 2013/05/27 14:48:41, rossberg wrote: > Can we initialize this properly instead of mutating later, i.e.: > > bool array_extended = self->IsJSArray() && ...; > > But actually, I'd simplify control flow by making it one 'if/else' and duplicate > the single EnqueueChangeRecord call. Done. https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js File src/v8natives.js (right): https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js#newcode929 src/v8natives.js:929: new_length < old_length ? new_length : old_length, Math. doesn't seem to be available from here. If there's a way to access the impl, let me know how. On 2013/05/27 14:48:41, rossberg wrote: > Perhaps use Math.min https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js#newcode932 src/v8natives.js:932: new_length > old_length ? new_length - old_length : 0); Ditto. On 2013/05/27 14:48:41, rossberg wrote: > Perhaps use Math.max https://codereview.chromium.org/15504002/diff/33001/test/mjsunit/harmony/obje... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/15504002/diff/33001/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:1005: { object: arr3, name: 'length', type: 'reconfigured' } It isn't correct, but Adam and I discussed and decided that a) It's fine to break with spec compliance here because it's an extreme edge case and even if hit, two records vs one is almost certain not to confuse any observer. b) Making it spec compliant in this minor way isn't worth layering hacks on top of more hacks here. Once the todo mentioned by mstarzinger is fixed, the right change record sequence will return. On 2013/05/27 14:48:41, rossberg wrote: > Not sure I understand, why is it correct that this a separate change now?
ping
LGTM with one comment. (Adam, please feel free to land should I not be around later.) https://codereview.chromium.org/15504002/diff/33001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11031 src/objects.cc:11031: uint32_t add_count = new_length > old_length ? new_length - old_length : 0; On 2013/05/28 11:38:53, rafaelw wrote: > new_length and old_length are both uint so, without casting, (new_length - > old_length) is never going to be less than 0. I could cast to ints, but I'm not > sure that buys much since Max is just a macro. > > Leaving for now. Please clarify if you want something else. > > On 2013/05/27 14:48:41, rossberg wrote: > > Max(new_length - old_length, 0) > OK, I see. https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11036 src/objects.cc:11036: while (i--) { On 2013/05/28 11:38:53, rafaelw wrote: > I'm curious why you prefer the for loop, but I've changed it for you. > > On 2013/05/27 14:48:41, rossberg wrote: > > Can we make this a for loop? I'm curious why you're curious. ;) It is just an indexed loop from i-1 to 0, isn't it? Why would it be preferable not to express this as a for loop? https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js File src/v8natives.js (right): https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js#newcode929 src/v8natives.js:929: new_length < old_length ? new_length : old_length, On 2013/05/28 11:38:53, rafaelw wrote: > Math. doesn't seem to be available from here. If there's a way to access the > impl, let me know how. You're right, Math is defined later in the bootstrapping process. https://codereview.chromium.org/15504002/diff/33001/test/mjsunit/harmony/obje... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/15504002/diff/33001/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:1005: { object: arr3, name: 'length', type: 'reconfigured' } On 2013/05/28 11:38:53, rafaelw wrote: > It isn't correct, but Adam and I discussed and decided that > > a) It's fine to break with spec compliance here because it's an extreme edge > case and even if hit, two records vs one is almost certain not to confuse any > observer. > > b) Making it spec compliant in this minor way isn't worth layering hacks on top > of more hacks here. Once the todo mentioned by mstarzinger is fixed, the right > change record sequence will return. > > On 2013/05/27 14:48:41, rossberg wrote: > > Not sure I understand, why is it correct that this a separate change now? > OK, can you add a TODO then?
https://codereview.chromium.org/15504002/diff/33001/test/mjsunit/harmony/obje... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/15504002/diff/33001/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:1005: { object: arr3, name: 'length', type: 'reconfigured' } On 2013/06/04 10:56:58, rossberg wrote: > On 2013/05/28 11:38:53, rafaelw wrote: > > It isn't correct, but Adam and I discussed and decided that > > > > a) It's fine to break with spec compliance here because it's an extreme edge > > case and even if hit, two records vs one is almost certain not to confuse any > > observer. > > > > b) Making it spec compliant in this minor way isn't worth layering hacks on > top > > of more hacks here. Once the todo mentioned by mstarzinger is fixed, the right > > change record sequence will return. > > > > On 2013/05/27 14:48:41, rossberg wrote: > > > Not sure I understand, why is it correct that this a separate change now? > > > > OK, can you add a TODO then? Done.
Message was sent while issue was closed.
Committed patchset #9 manually as r14944 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
