Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1949863002:
Fix TypedArray Property optimizations (Closed)
DescriptionFix TypedArray Property optimizations
This patch installs %TypedArray% and its prototype on the native
context, and wires them up to each TypedArray subclass. This is later
used to check the holder of length, byteLength and byteOffset is
%Typedarray% and apply the appropriate optimizations.
BUG=chromium:593634
LOG=Y
Committed: https://crrev.com/41d571dfe87d70bb04a59e6e6dbe732b6df56277
Cr-Commit-Position: refs/heads/master@{#36116}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Addressed review comments #
Total comments: 2
Patch Set 3 : Use CreateFunction #
Total comments: 3
Messages
Total messages: 27 (10 generated)
Description was changed from ========== Fix TypedArray Property optimizations This patch installs %TypedArray% and it's prototype on the native context, and wires them up to each TypedArray subclass. This is later used to check the holder of length, byteLength and byteOffset is %Typedarray% and apply the appropriate optimizations. BUG=chromium:593634 LOG=Y ========== to ========== Fix TypedArray Property optimizations This patch installs %TypedArray% and it's prototype on the native context, and wires them up to each TypedArray subclass. This is later used to check the holder of length, byteLength and byteOffset is %Typedarray% and apply the appropriate optimizations. BUG=chromium:593634 LOG=Y ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org, littledan@chromium.org
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949863002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949863002/1
Description was changed from ========== Fix TypedArray Property optimizations This patch installs %TypedArray% and it's prototype on the native context, and wires them up to each TypedArray subclass. This is later used to check the holder of length, byteLength and byteOffset is %Typedarray% and apply the appropriate optimizations. BUG=chromium:593634 LOG=Y ========== to ========== Fix TypedArray Property optimizations This patch installs %TypedArray% and its prototype on the native context, and wires them up to each TypedArray subclass. This is later used to check the holder of length, byteLength and byteOffset is %Typedarray% and apply the appropriate optimizations. BUG=chromium:593634 LOG=Y ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1963: DCHECK(false); Nit: DCHECK(false) is rather unidiomatic (although it is used elsewhere in this file). Maybe save the result of SetPrototype in a Maybe<bool> and then DCHECK(!result.IsNothing()) ? https://codereview.chromium.org/1949863002/diff/1/src/js/array-iterator.js File src/js/array-iterator.js (right): https://codereview.chromium.org/1949863002/diff/1/src/js/array-iterator.js#ne... src/js/array-iterator.js:25: var GlobalTypedArray = %object_get_prototype_of(global.Uint8Array); While this change is clearly valid, it would be nice to know why it is necessary in this patch.
https://codereview.chromium.org/1949863002/diff/1/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/1949863002/diff/1/src/accessors.cc#newcode119 src/accessors.cc:119: HandleScope scope(isolate); You shouldn't need to create a HandleScope here, you already have one on the stack somewhere (else the above Handle creation wouldn't work). https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1614: global, "TypedArray", JS_OBJECT_TYPE, JSObject::kHeaderSize, I don't think you want to install this on the global, do you? I thought this was the %TypedArray% function, which doesn't have a name. https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1943: Handle<JSObject>(native_context()->typed_array_prototype()); You can use the shorter "isolate()->typed_array_prototype()" accessor which will give you back a handle. https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1945: Handle<JSFunction>(native_context()->typed_array_function()); Same here, isolate()->typed_array_function(). https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1960: if (JSReceiver::SetPrototype(result, typed_array_function, false, I'd use JSObject::SetPrototype() here since you know you're not operating on a Proxy. https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1963: DCHECK(false); On 2016/05/04 18:11:13, Dan Ehrenberg wrote: > Nit: DCHECK(false) is rather unidiomatic (although it is used elsewhere in this > file). Maybe save the result of SetPrototype in a Maybe<bool> and then > DCHECK(!result.IsNothing()) ? I agree with this approach, though a slight modification: change to pass Object::DONT_THROW, and then restructure to surround in a CHECK (not a DCHECK; this should never fail, and since it happens during mksnapshot it will just crash mksnapshot, never actually crash Chrome). CHECK(JSObject::SetPrototype(..., Object::DONT_THROW).FromJust()); https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1966: if (JSReceiver::SetPrototype(prototype, typed_array_prototype, false, Same here, JSObject::SetPrototype.
Thanks for the comments. PTAL. https://codereview.chromium.org/1949863002/diff/1/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/1949863002/diff/1/src/accessors.cc#newcode119 src/accessors.cc:119: HandleScope scope(isolate); On 2016/05/04 18:44:36, adamk wrote: > You shouldn't need to create a HandleScope here, you already have one on the > stack somewhere (else the above Handle creation wouldn't work). Done. https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1614: global, "TypedArray", JS_OBJECT_TYPE, JSObject::kHeaderSize, On 2016/05/04 18:44:36, adamk wrote: > I don't think you want to install this on the global, do you? I thought this was > the %TypedArray% function, which doesn't have a name. Makes sense. But, I still need to create this JSFunction which requires a target. I chatted with Dan offline and we decided to leave it as such since it's only the bootstrapper's global object. Is there a better way to do this? https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1943: Handle<JSObject>(native_context()->typed_array_prototype()); On 2016/05/04 18:44:36, adamk wrote: > You can use the shorter "isolate()->typed_array_prototype()" accessor which will > give you back a handle. Done. https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1945: Handle<JSFunction>(native_context()->typed_array_function()); On 2016/05/04 18:44:36, adamk wrote: > Same here, isolate()->typed_array_function(). Done. https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1960: if (JSReceiver::SetPrototype(result, typed_array_function, false, On 2016/05/04 18:44:36, adamk wrote: > I'd use JSObject::SetPrototype() here since you know you're not operating on a > Proxy. Done. https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1963: DCHECK(false); On 2016/05/04 18:44:36, adamk wrote: > On 2016/05/04 18:11:13, Dan Ehrenberg wrote: > > Nit: DCHECK(false) is rather unidiomatic (although it is used elsewhere in > this > > file). Maybe save the result of SetPrototype in a Maybe<bool> and then > > DCHECK(!result.IsNothing()) ? > > I agree with this approach, though a slight modification: change to pass > Object::DONT_THROW, and then restructure to surround in a CHECK (not a DCHECK; > this should never fail, and since it happens during mksnapshot it will just > crash mksnapshot, never actually crash Chrome). > > CHECK(JSObject::SetPrototype(..., Object::DONT_THROW).FromJust()); Done. https://codereview.chromium.org/1949863002/diff/1/src/bootstrapper.cc#newcode... src/bootstrapper.cc:1966: if (JSReceiver::SetPrototype(prototype, typed_array_prototype, false, On 2016/05/04 18:44:36, adamk wrote: > Same here, JSObject::SetPrototype. Done. https://codereview.chromium.org/1949863002/diff/1/src/js/array-iterator.js File src/js/array-iterator.js (right): https://codereview.chromium.org/1949863002/diff/1/src/js/array-iterator.js#ne... src/js/array-iterator.js:25: var GlobalTypedArray = %object_get_prototype_of(global.Uint8Array); On 2016/05/04 18:11:13, Dan Ehrenberg wrote: > While this change is clearly valid, it would be nice to know why it is necessary > in this patch. Not having this change doesn't break anything. I just reran the test suite without this change and it works. I guess I had multiple broken things at one point which made me suspect this. Going to leave it as such.
https://codereview.chromium.org/1949863002/diff/20001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1949863002/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:1615: typed_array_prototype, Builtins::kIllegal); Use CreateFunction rather than InstallFunction in order to not expose this to the global object.
PTAL https://codereview.chromium.org/1949863002/diff/20001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1949863002/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:1615: typed_array_prototype, Builtins::kIllegal); On 2016/05/05 23:37:31, Dan Ehrenberg wrote: > Use CreateFunction rather than InstallFunction in order to not expose this to > the global object. Thanks Dan! Done.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949863002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949863002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/1949863002/diff/40001/src/js/array-iterator.js File src/js/array-iterator.js (right): https://codereview.chromium.org/1949863002/diff/40001/src/js/array-iterator.j... src/js/array-iterator.js:25: var GlobalTypedArray = %object_get_prototype_of(global.Uint8Array); Did we ever figure out why this had to change? It still sounds worrying to me; I'd suggest trying reverting this change and seeing if everything works.
https://codereview.chromium.org/1949863002/diff/40001/src/js/array-iterator.js File src/js/array-iterator.js (right): https://codereview.chromium.org/1949863002/diff/40001/src/js/array-iterator.j... src/js/array-iterator.js:25: var GlobalTypedArray = %object_get_prototype_of(global.Uint8Array); On 2016/05/06 at 17:56:12, adamk wrote: > Did we ever figure out why this had to change? It still sounds worrying to me; I'd suggest trying reverting this change and seeing if everything works. From off-line communication, sounds like it didn't have to change, and the old way worked. However, I think this phrasing is cleaner--it depends on less, and requires less thinking about the state of the world to prove that it's correct.
lgtm https://codereview.chromium.org/1949863002/diff/40001/src/js/array-iterator.js File src/js/array-iterator.js (right): https://codereview.chromium.org/1949863002/diff/40001/src/js/array-iterator.j... src/js/array-iterator.js:25: var GlobalTypedArray = %object_get_prototype_of(global.Uint8Array); On 2016/05/06 18:09:33, Dan Ehrenberg wrote: > On 2016/05/06 at 17:56:12, adamk wrote: > > Did we ever figure out why this had to change? It still sounds worrying to me; > I'd suggest trying reverting this change and seeing if everything works. > > From off-line communication, sounds like it didn't have to change, and the old > way worked. However, I think this phrasing is cleaner--it depends on less, and > requires less thinking about the state of the world to prove that it's correct. Sounds good, and I agree that this way of spelling it is clearer.
The CQ bit was checked by gsathya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949863002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949863002/40001
Message was sent while issue was closed.
Description was changed from ========== Fix TypedArray Property optimizations This patch installs %TypedArray% and its prototype on the native context, and wires them up to each TypedArray subclass. This is later used to check the holder of length, byteLength and byteOffset is %Typedarray% and apply the appropriate optimizations. BUG=chromium:593634 LOG=Y ========== to ========== Fix TypedArray Property optimizations This patch installs %TypedArray% and its prototype on the native context, and wires them up to each TypedArray subclass. This is later used to check the holder of length, byteLength and byteOffset is %Typedarray% and apply the appropriate optimizations. BUG=chromium:593634 LOG=Y ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix TypedArray Property optimizations This patch installs %TypedArray% and its prototype on the native context, and wires them up to each TypedArray subclass. This is later used to check the holder of length, byteLength and byteOffset is %Typedarray% and apply the appropriate optimizations. BUG=chromium:593634 LOG=Y ========== to ========== Fix TypedArray Property optimizations This patch installs %TypedArray% and its prototype on the native context, and wires them up to each TypedArray subclass. This is later used to check the holder of length, byteLength and byteOffset is %Typedarray% and apply the appropriate optimizations. BUG=chromium:593634 LOG=Y Committed: https://crrev.com/41d571dfe87d70bb04a59e6e6dbe732b6df56277 Cr-Commit-Position: refs/heads/master@{#36116} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/41d571dfe87d70bb04a59e6e6dbe732b6df56277 Cr-Commit-Position: refs/heads/master@{#36116} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
