Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543553002:
[accessors] handle `writable` changing during ArrayLengthSetter (Closed)
Description[accessors] handle `writable` changing during ArrayLengthSetter
The "writable" property descriptor may legally change during the call to
AnythingToArrayLength(). This change needs to be honoured before calling
JSArray::SetLength(). The change is only honoured when the "length"
property was previously writable, so that changes during a call to
DefineOwnPropertyIgnoreAttributes() is ignored.
BUG=v8:5688
R=cbruni@chromium.org, verwaest@chromium.org, jkummerow@chromium.org
Committed: https://crrev.com/d4918463a93b377be9cbf8325b3d2cfa226985d6
Cr-Commit-Position: refs/heads/master@{#41396}
Patch Set 1 #
Total comments: 2
Patch Set 2 : [accessors] handle `writable` changing during ArrayLengthSetter #Patch Set 3 : use JSArray::HasReadOnlyLength() #Patch Set 4 : s/name_string()/length_string()/ #
Total comments: 2
Patch Set 5 : refactor branch conditions for unlikely case #Messages
Total messages: 36 (23 generated)
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...
Description was changed from ========== [accessors] handle `writable` changing during ArrayLengthSetter The "writable" property descriptor may legally change during the call to AnythingToArrayLength(). This change needs to be honoured before calling JSArray::SetLength(). The change is only honoured when the "length" property was previously writable, so that changes during a call to DefineOwnPropertyIgnoreAttributes() is ignored. BUG=v8:5688 R=cbruni@chromium.org, verwaest@chromium.org ========== to ========== [accessors] handle `writable` changing during ArrayLengthSetter The "writable" property descriptor may legally change during the call to AnythingToArrayLength(). This change needs to be honoured before calling JSArray::SetLength(). The change is only honoured when the "length" property was previously writable, so that changes during a call to DefineOwnPropertyIgnoreAttributes() is ignored. BUG=v8:5688 R=cbruni@chromium.org, verwaest@chromium.org, jkummerow@chromium.org ==========
caitp@igalia.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.
https://codereview.chromium.org/2543553002/diff/1/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/2543553002/diff/1/src/accessors.cc#newcode191 src/accessors.cc:191: LookupIterator it(object, Utils::OpenHandle(*name), LookupIterator::OWN); I think you want to delay the LookupIterator creation as far as possible here. if (!was_readonly && length != array->length()->Number()) { LookupIterator it(...); if (V8_UNLIKELY(...)) {...
https://codereview.chromium.org/2543553002/diff/1/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/2543553002/diff/1/src/accessors.cc#newcode191 src/accessors.cc:191: LookupIterator it(object, Utils::OpenHandle(*name), LookupIterator::OWN); On 2016/11/30 12:08:27, Camillo Bruni wrote: > I think you want to delay the LookupIterator creation as far as possible here. > if (!was_readonly && length != array->length()->Number()) { > LookupIterator it(...); > if (V8_UNLIKELY(...)) {... Done, but I think with that condition, it gets created in the vast majority of cases, so it probably doesn't add much in terms of savings. Maybe PropertyCallbackInfo should include a representation of property details to at least avoid the first lookup iterator on line 174?
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/11/30 at 13:17:59, caitp wrote: > https://codereview.chromium.org/2543553002/diff/1/src/accessors.cc > File src/accessors.cc (right): > > https://codereview.chromium.org/2543553002/diff/1/src/accessors.cc#newcode191 > src/accessors.cc:191: LookupIterator it(object, Utils::OpenHandle(*name), LookupIterator::OWN); > On 2016/11/30 12:08:27, Camillo Bruni wrote: > > I think you want to delay the LookupIterator creation as far as possible here. > > if (!was_readonly && length != array->length()->Number()) { > > LookupIterator it(...); > > if (V8_UNLIKELY(...)) {... > > Done, but I think with that condition, it gets created in the vast majority of cases, so it probably doesn't add much in terms of savings. > > Maybe PropertyCallbackInfo should include a representation of property details to at least avoid the first lookup iterator on line 174? Another strategy is to directly read out from the descriptor array, given that arrays have only one named property added by default which is marked non-configurable (See Generate_FastArrayPush in builtins-array.cc). Maybe you could "quickly" measure the rough performance overhead and check whether the above optimization makes sense or not. I just checked that on my top30 websites I measure regularly, there are no calls to the ArrayLengthSetter, so it might actually not matter so much if we get a minor regression here.
On 2016/11/30 at 16:11:53, Camillo Bruni wrote: > On 2016/11/30 at 13:17:59, caitp wrote: > > https://codereview.chromium.org/2543553002/diff/1/src/accessors.cc > > File src/accessors.cc (right): > > > > https://codereview.chromium.org/2543553002/diff/1/src/accessors.cc#newcode191 > > src/accessors.cc:191: LookupIterator it(object, Utils::OpenHandle(*name), LookupIterator::OWN); > > On 2016/11/30 12:08:27, Camillo Bruni wrote: > > > I think you want to delay the LookupIterator creation as far as possible here. > > > if (!was_readonly && length != array->length()->Number()) { > > > LookupIterator it(...); > > > if (V8_UNLIKELY(...)) {... > > > > Done, but I think with that condition, it gets created in the vast majority of cases, so it probably doesn't add much in terms of savings. > > > > Maybe PropertyCallbackInfo should include a representation of property details to at least avoid the first lookup iterator on line 174? > > Another strategy is to directly read out from the descriptor array, given that arrays have only one named property added by default which is marked non-configurable (See Generate_FastArrayPush in builtins-array.cc). > > Maybe you could "quickly" measure the rough performance overhead and check whether the above optimization makes sense or not. > I just checked that on my top30 websites I measure regularly, there are no calls to the ArrayLengthSetter, so it might actually not matter so much if we get a minor regression here. As Toon pointed out, there's JSArray::HasReadOnlyLength which does all the necessary checks.
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/11/30 16:35:11, Camillo Bruni wrote: > On 2016/11/30 at 16:11:53, Camillo Bruni wrote: > > On 2016/11/30 at 13:17:59, caitp wrote: > > > https://codereview.chromium.org/2543553002/diff/1/src/accessors.cc > > > File src/accessors.cc (right): > > > > > > > https://codereview.chromium.org/2543553002/diff/1/src/accessors.cc#newcode191 > > > src/accessors.cc:191: LookupIterator it(object, Utils::OpenHandle(*name), > LookupIterator::OWN); > > > On 2016/11/30 12:08:27, Camillo Bruni wrote: > > > > I think you want to delay the LookupIterator creation as far as possible > here. > > > > if (!was_readonly && length != array->length()->Number()) { > > > > LookupIterator it(...); > > > > if (V8_UNLIKELY(...)) {... > > > > > > Done, but I think with that condition, it gets created in the vast majority > of cases, so it probably doesn't add much in terms of savings. > > > > > > Maybe PropertyCallbackInfo should include a representation of property > details to at least avoid the first lookup iterator on line 174? > > > > Another strategy is to directly read out from the descriptor array, given that > arrays have only one named property added by default which is marked > non-configurable (See Generate_FastArrayPush in builtins-array.cc). > > > > Maybe you could "quickly" measure the rough performance overhead and check > whether the above optimization makes sense or not. > > I just checked that on my top30 websites I measure regularly, there are no > calls to the ArrayLengthSetter, so it might actually not matter so much if we > get a minor regression here. > > As Toon pointed out, there's JSArray::HasReadOnlyLength which does all the > necessary checks. Using that now
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17092) 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/17146) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17058)
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.
Lgtm with comment https://codereview.chromium.org/2543553002/diff/60001/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/2543553002/diff/60001/src/accessors.cc#newcod... src/accessors.cc:188: // DefineOwnPropertyIgnoreAttributes(). What about if (!was_readonly && V8_UNLIKELY(is read-only && length !=...)) ?
https://codereview.chromium.org/2543553002/diff/60001/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/2543553002/diff/60001/src/accessors.cc#newcod... src/accessors.cc:188: // DefineOwnPropertyIgnoreAttributes(). On 2016/11/30 18:05:52, Toon Verwaest wrote: > What about if (!was_readonly && V8_UNLIKELY(is read-only && length !=...)) ? `I think the combination of !was_readonly && length != current_length` is the most common case, but in that common case, the "is read only" becomes very uncommon
I agree but we don't care for the second condition unless the unlikely thing occurs so shouldn't test it first?
On 2016/11/30 18:16:52, Toon Verwaest wrote: > I agree but we don't care for the second condition unless the unlikely thing > occurs so shouldn't test it first? alright, done
The CQ bit was checked by caitp@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from verwaest@chromium.org Link to the patchset: https://codereview.chromium.org/2543553002/#ps80001 (title: "refactor branch conditions for unlikely case")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480531043327060,
"parent_rev": "82cd050e5bfc521943e68fc542fbd3e75eae3b29", "commit_rev":
"e5bda2052754f0644cc4e349d92be9543aefa95e"}
Message was sent while issue was closed.
Description was changed from ========== [accessors] handle `writable` changing during ArrayLengthSetter The "writable" property descriptor may legally change during the call to AnythingToArrayLength(). This change needs to be honoured before calling JSArray::SetLength(). The change is only honoured when the "length" property was previously writable, so that changes during a call to DefineOwnPropertyIgnoreAttributes() is ignored. BUG=v8:5688 R=cbruni@chromium.org, verwaest@chromium.org, jkummerow@chromium.org ========== to ========== [accessors] handle `writable` changing during ArrayLengthSetter The "writable" property descriptor may legally change during the call to AnythingToArrayLength(). This change needs to be honoured before calling JSArray::SetLength(). The change is only honoured when the "length" property was previously writable, so that changes during a call to DefineOwnPropertyIgnoreAttributes() is ignored. BUG=v8:5688 R=cbruni@chromium.org, verwaest@chromium.org, jkummerow@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [accessors] handle `writable` changing during ArrayLengthSetter The "writable" property descriptor may legally change during the call to AnythingToArrayLength(). This change needs to be honoured before calling JSArray::SetLength(). The change is only honoured when the "length" property was previously writable, so that changes during a call to DefineOwnPropertyIgnoreAttributes() is ignored. BUG=v8:5688 R=cbruni@chromium.org, verwaest@chromium.org, jkummerow@chromium.org ========== to ========== [accessors] handle `writable` changing during ArrayLengthSetter The "writable" property descriptor may legally change during the call to AnythingToArrayLength(). This change needs to be honoured before calling JSArray::SetLength(). The change is only honoured when the "length" property was previously writable, so that changes during a call to DefineOwnPropertyIgnoreAttributes() is ignored. BUG=v8:5688 R=cbruni@chromium.org, verwaest@chromium.org, jkummerow@chromium.org Committed: https://crrev.com/d4918463a93b377be9cbf8325b3d2cfa226985d6 Cr-Commit-Position: refs/heads/master@{#41396} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d4918463a93b377be9cbf8325b3d2cfa226985d6 Cr-Commit-Position: refs/heads/master@{#41396} |
