Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592933004:
[promises] Move Promise.resolve to TF (Closed)
Description[promises] Move Promise.resolve to TF
Add a more low level BranchIfFastPath to take the native_context and
promise_fun as args and change the existing one to use this.
BUG=v8:5343
Review-Url: https://codereview.chromium.org/2592933004
Cr-Commit-Position: refs/heads/master@{#42075}
Committed: https://chromium.googlesource.com/v8/v8/+/79ae8f17442bf61e9655fa8b27d4ddd529cccb0e
Patch Set 1 #Patch Set 2 : faaaast pathssssssss #Patch Set 3 : check identity of promise #Patch Set 4 : add promisehook #Patch Set 5 : throw correct typerror #
Total comments: 5
Patch Set 6 : rebase #Patch Set 7 : remove native_context #Patch Set 8 : add new branchiffastpath #Patch Set 9 : add asserts #Patch Set 10 : fix test #
Total comments: 8
Patch Set 11 : fix nits #Patch Set 12 : fix rebase #
Messages
Total messages: 56 (47 generated)
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/v2/patch-status/codereview.chromium.or...
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/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.
Description was changed from ========== [promises] Move Promise.resolve to TF BUG=v8:5343 ========== to ========== [promises] Move Promise.resolve to TF BUG=v8:5343 ==========
gsathya@chromium.org changed reviewers: + ishell@chromium.org, jgruber@chromium.org
Description was changed from ========== [promises] Move Promise.resolve to TF BUG=v8:5343 ========== to ========== [promises] Move Promise.resolve to TF This improves the fast path by 10% but regresses the slow path because of having to use the getproperty stub. This should go away once we land https://codereview.chromium.org/2567333002/ BUG=v8:5343 ==========
Description was changed from ========== [promises] Move Promise.resolve to TF This improves the fast path by 10% but regresses the slow path because of having to use the getproperty stub. This should go away once we land https://codereview.chromium.org/2567333002/ BUG=v8:5343 ========== to ========== [promises] Move Promise.resolve to TF This improves the fast path by 10% but regresses the slow path by about 25% because of having to use the getproperty stub. This should go away once we land https://codereview.chromium.org/2567333002/ BUG=v8:5343 ==========
adamk@chromium.org changed reviewers: + adamk@chromium.org
Mostly trying to learn best practices here, feel free to educate me if my comments are off-base :) https://codereview.chromium.org/2592933004/diff/80001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2592933004/diff/80001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:1072: Node* const native_context = LoadNativeContext(context); Newbie question: Doesn't this duplicate the context load that's already being done in BranchIfFastPath? Same for the load of PROMISE_FUNCTION_INDEX below. Not sure what the preferred code organization strategy is (maybe factor our part of BranchIfFastPath out into something that takes the promise constructor as an argument?), but it seems like the fast path should be as fast as possible. https://codereview.chromium.org/2592933004/diff/80001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:1126: Node* const native_context = LoadNativeContext(context); Possibly another newbie question: seems like this shouldn't need to reload the native context if you do so earlier in the code?
lgtm with a nit: https://codereview.chromium.org/2592933004/diff/80001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2592933004/diff/80001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:1072: Node* const native_context = LoadNativeContext(context); On 2016/12/22 19:39:35, adamk wrote: > Newbie question: Doesn't this duplicate the context load that's already being > done in BranchIfFastPath? Same for the load of PROMISE_FUNCTION_INDEX below. Not > sure what the preferred code organization strategy is (maybe factor our part of > BranchIfFastPath out into something that takes the promise constructor as an > argument?), but it seems like the fast path should be as fast as possible. Good point. Maybe BranchIfFastPath() should return a native context.
Description was changed from ========== [promises] Move Promise.resolve to TF This improves the fast path by 10% but regresses the slow path by about 25% because of having to use the getproperty stub. This should go away once we land https://codereview.chromium.org/2567333002/ BUG=v8:5343 ========== to ========== [promises] Move Promise.resolve to TF This also changes BranchIfFastPath to take the native_context instead of the context to prevent loading of native context twice. BUG=v8:5343 ==========
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/v2/patch-status/codereview.chromium.or...
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/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.
Description was changed from ========== [promises] Move Promise.resolve to TF This also changes BranchIfFastPath to take the native_context instead of the context to prevent loading of native context twice. BUG=v8:5343 ========== to ========== [promises] Move Promise.resolve to TF Add a more low level BranchIfFastPath to take the native_context and promise_fun as args and change the existing one to use this. BUG=v8:5343 ==========
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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2592933004/diff/80001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2592933004/diff/80001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:1072: Node* const native_context = LoadNativeContext(context); On 2016/12/27 14:30:28, Igor Sheludko wrote: > On 2016/12/22 19:39:35, adamk wrote: > > Newbie question: Doesn't this duplicate the context load that's already being > > done in BranchIfFastPath? Same for the load of PROMISE_FUNCTION_INDEX below. > Not > > sure what the preferred code organization strategy is (maybe factor our part > of > > BranchIfFastPath out into something that takes the promise constructor as an > > argument?), but it seems like the fast path should be as fast as possible. > > Good point. Maybe BranchIfFastPath() should return a native context. Added a BranchIfFastPath that accepts a native context and promise constructor. https://codereview.chromium.org/2592933004/diff/80001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:1126: Node* const native_context = LoadNativeContext(context); On 2016/12/22 19:39:35, adamk wrote: > Possibly another newbie question: seems like this shouldn't need to reload the > native context if you do so earlier in the code? Only certain code paths use the native_context so I couldn't move this to the top of the function, otherwise they would pay the cost of loading the native context. I could refactor this out to be a separate function that takes a native_context or load if it's null, not sure if that's worth the savings in the slow path?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/14954) 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...)
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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:200001) has been deleted
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... src/builtins/builtins-promise.cc:209: Isolate* isolate = this->isolate(); I don't think you need this, can just use "isolate()" below https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... src/builtins/builtins-promise.cc:210: Node* method; Node* const for consistency. https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... src/builtins/builtins-promise.cc:211: method = method_name == NULL s/NULL/nullptr/ And you can merge this with the declaration above. https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... File src/builtins/builtins-promise.h (right): https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... src/builtins/builtins-promise.h:23: const char* method_name = NULL); nit: s/NULL/nullptr/
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... src/builtins/builtins-promise.cc:209: Isolate* isolate = this->isolate(); On 2017/01/04 19:19:21, adamk wrote: > I don't think you need this, can just use "isolate()" below Done. https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... src/builtins/builtins-promise.cc:210: Node* method; On 2017/01/04 19:19:21, adamk wrote: > Node* const for consistency. Done. https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... src/builtins/builtins-promise.cc:211: method = method_name == NULL On 2017/01/04 19:19:21, adamk wrote: > s/NULL/nullptr/ > > And you can merge this with the declaration above. Done. https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... File src/builtins/builtins-promise.h (right): https://codereview.chromium.org/2592933004/diff/180001/src/builtins/builtins-... src/builtins/builtins-promise.h:23: const char* method_name = NULL); On 2017/01/04 19:19:21, adamk wrote: > nit: s/NULL/nullptr/ Done.
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 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/v2/patch-status/codereview.chromium.or...
still lgtm
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 gsathya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2592933004/#ps240001 (title: "fix rebase")
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": 240001, "attempt_start_ts": 1483569318566370,
"parent_rev": "e878ea9dcefc1fa1511af60cda3dfcaf02675856", "commit_rev":
"79ae8f17442bf61e9655fa8b27d4ddd529cccb0e"}
Message was sent while issue was closed.
Description was changed from ========== [promises] Move Promise.resolve to TF Add a more low level BranchIfFastPath to take the native_context and promise_fun as args and change the existing one to use this. BUG=v8:5343 ========== to ========== [promises] Move Promise.resolve to TF Add a more low level BranchIfFastPath to take the native_context and promise_fun as args and change the existing one to use this. BUG=v8:5343 Review-Url: https://codereview.chromium.org/2592933004 Cr-Commit-Position: refs/heads/master@{#42075} Committed: https://chromium.googlesource.com/v8/v8/+/79ae8f17442bf61e9655fa8b27d4ddd529c... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/v8/v8/+/79ae8f17442bf61e9655fa8b27d4ddd529c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
