Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years ago by gsathya Modified:
4 years ago Reviewers:
Igor Sheludko, caitp, jochen (gone - plz use gerrit), jgruber, Benedikt Meurer, Dan Ehrenberg CC:
adamk, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[promises] Port ResolvePromise to TF
-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Fix a non spec async-await test which fails now because we do a map
check for native promise check (instead of IsPromise). Changing the
constructor (in the test) invalidates the map check.
This patch results in a 7.1% performance improvement in the bluebird
benchmark (over 5 runs).
BUG=v8:5343
Committed: https://crrev.com/11359e331ae98978fe91b6c6925a1569dbd61856
Cr-Commit-Position: refs/heads/master@{#41569}
Patch Set 1 #Patch Set 2 : fix test #Patch Set 3 : fix test #Patch Set 4 : fix GetHeaderSize #Patch Set 5 : work work work #Patch Set 6 : rebase #Patch Set 7 : revert test #Patch Set 8 : add comments #
Total comments: 20
Patch Set 9 : stuff #Patch Set 10 : rebase #Patch Set 11 : move hasHandler to inobject #Patch Set 12 : remove unused var #Patch Set 13 : remove ResolvePromise #
Total comments: 17
Patch Set 14 : fix #Patch Set 15 : Fix test #
Total comments: 10
Patch Set 16 : fix nits #
Total comments: 1
Created: 4 years ago
Messages
Total messages: 68 (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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/18776)
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: 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...)
Description was changed from ========== [promises] Port ResolvePromise to TF BUG=v8:5343 ========== to ========== [promises] Port ResolvePromise to TF -- Ports PromiseResolveClosure to TF -- Deletes non spec async-await spec which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. BUG=v8:5343 ==========
gsathya@chromium.org changed reviewers: + bmeurer@chromium.org, caitp@igalia.com, jgruber@chromium.org
Description was changed from ========== [promises] Port ResolvePromise to TF -- Ports PromiseResolveClosure to TF -- Deletes non spec async-await spec which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. BUG=v8:5343 ========== to ========== [promises] Port ResolvePromise to TF -- Ports PromiseResolveClosure to TF -- Deletes non spec async-await spec which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. BUG=v8:5343 ==========
Description was changed from ========== [promises] Port ResolvePromise to TF -- Ports PromiseResolveClosure to TF -- Deletes non spec async-await spec which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. BUG=v8:5343 ========== to ========== [promises] Port ResolvePromise to TF -- Ports PromiseResolveClosure to TF -- Deletes non spec async-await spec which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). BUG=v8:5343 ==========
Until we can enqueue microtasks from JS, doesn't it make more sense for these to just be implemented in c++?
On 2016/12/03 02:44:16, caitp wrote: > Until we can enqueue microtasks from JS, doesn't it make more sense for these to > just be implemented in c++? I assume you mean TF, not JS? I can port them all in the same patch I guess, but that's a lot of code and this isn't a regression. I'd rather do it piecemeal unless there's a regression and avoid porting JS-> C++ and then C++ -> TF again.
On 2016/12/03 03:04:40, gsathya wrote: > On 2016/12/03 02:44:16, caitp wrote: > > Until we can enqueue microtasks from JS, doesn't it make more sense for these > to > > just be implemented in c++? > > I assume you mean TF, not JS? I can port them all in the same patch I guess, but > that's a lot of code and this isn't a regression. I'd rather do it piecemeal > unless there's a regression and avoid porting JS-> C++ and then C++ -> TF > again. Ok, I just wanted to understand the architectural direction this is headed in. Sounds reasonable to me.
lgtm lgtm with comments https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:645: // RegExp fast path implementations rely on unmodified JSPromise instances. Promise https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:673: // TODO(ishell): Update this check once map changes for constant field We can remove this TODO since the mentioned changes will not be happening AFAIK (I should also do the same in builtins-regexp). https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:681: typedef CodeStubAssembler::Variable Variable; As mentioned offline, you might consider switching to the new TF_BUILTIN style sooner than later. You won't need these typedefs in every function in that case. https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:693: a->GotoIf(a->WordEqual(promise, result), &if_cycle); SameValue is more involved than WordEqual. You can use CSA::SameValue. https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:698: a->GotoUnless(a->IsJSReceiverInstanceType(result_instance_type), &fulfill); IsJSReceiver() https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:822: a.GotoIf(a.SmiEqual(has_already_visited, a.SmiConstant(1)), &out); Is line 825 below the only place where we write into kAlreadyVisitedSlot? Would it make sense to store True/FalseConstant instead of 0/1 Smis? https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc... src/js/promise.js:121: %resolve_promise(promise, resolution); Can we replace calls to ResolvePromise by %resolve_promise and remove ResolvePromise? https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc... src/js/promise.js:405: %SetCode(GlobalPromise.prototype.catch, PromiseCatch); This and below + corresponding changes in bootstrapper seem unrelated?
https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:645: // RegExp fast path implementations rely on unmodified JSPromise instances. On 2016/12/05 09:25:17, jgruber wrote: > Promise Done. https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:673: // TODO(ishell): Update this check once map changes for constant field On 2016/12/05 09:25:17, jgruber wrote: > We can remove this TODO since the mentioned changes will not be happening AFAIK > (I should also do the same in builtins-regexp). Done. https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:681: typedef CodeStubAssembler::Variable Variable; On 2016/12/05 09:25:17, jgruber wrote: > As mentioned offline, you might consider switching to the new TF_BUILTIN style > sooner than later. You won't need these typedefs in every function in that case. I'm going to wait until I finish the rest of the builtins before doing the cleanup. I currently have 4 levels of dependent patches so I don't want to deal with too much unnecessary churn right now. https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:693: a->GotoIf(a->WordEqual(promise, result), &if_cycle); On 2016/12/05 09:25:17, jgruber wrote: > SameValue is more involved than WordEqual. You can use CSA::SameValue. Done. https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:698: a->GotoUnless(a->IsJSReceiverInstanceType(result_instance_type), &fulfill); On 2016/12/05 09:25:17, jgruber wrote: > IsJSReceiver() Done. https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:822: a.GotoIf(a.SmiEqual(has_already_visited, a.SmiConstant(1)), &out); On 2016/12/05 09:25:17, jgruber wrote: > Is line 825 below the only place where we write into kAlreadyVisitedSlot? Would > it make sense to store True/FalseConstant instead of 0/1 Smis? PromiseUtils::CreateResolvingFunctions and PromiseRejectClosure also use this. I think Smi is cheaper though anyway https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc... src/js/promise.js:121: %resolve_promise(promise, resolution); On 2016/12/05 09:25:17, jgruber wrote: > Can we replace calls to ResolvePromise by %resolve_promise and remove > ResolvePromise? Nope. We use this in async-await and v8-extras https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc... src/js/promise.js:405: %SetCode(GlobalPromise.prototype.catch, PromiseCatch); On 2016/12/05 09:25:17, jgruber wrote: > This and below + corresponding changes in bootstrapper seem unrelated? I need to install an empty .catch in bootstrapper and set the code here, otherwise the prototype will change here and my map checks will always fail.
https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc... src/js/promise.js:121: %resolve_promise(promise, resolution); On 2016/12/05 16:03:05, gsathya wrote: > On 2016/12/05 09:25:17, jgruber wrote: > > Can we replace calls to ResolvePromise by %resolve_promise and remove > > ResolvePromise? > > Nope. We use this in async-await and v8-extras Couldn't we just export %resolve_promise from the bootstrapper and avoid creating the extra closure in promise.js?
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: 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...)
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.
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...
Description was changed from ========== [promises] Port ResolvePromise to TF -- Ports PromiseResolveClosure to TF -- Deletes non spec async-await spec which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). BUG=v8:5343 ========== to ========== [promises] Port ResolvePromise to TF -- Moves promiseHasHandlerSymbol to inobject property -- Ports PromiseResolveClosure to TF -- Deletes non spec async-await spec which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). BUG=v8:5343 ==========
On 2016/12/05 16:22:51, caitp wrote: > https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js > File src/js/promise.js (right): > > https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc... > src/js/promise.js:121: %resolve_promise(promise, resolution); > On 2016/12/05 16:03:05, gsathya wrote: > > On 2016/12/05 09:25:17, jgruber wrote: > > > Can we replace calls to ResolvePromise by %resolve_promise and remove > > > ResolvePromise? > > > > Nope. We use this in async-await and v8-extras > > Couldn't we just export %resolve_promise from the bootstrapper and avoid > creating the extra closure in promise.js? Done. PTAL
On 2016/12/06 19:10:48, gsathya wrote: > On 2016/12/05 16:22:51, caitp wrote: > > https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js > > File src/js/promise.js (right): > > > > > https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc... > > src/js/promise.js:121: %resolve_promise(promise, resolution); > > On 2016/12/05 16:03:05, gsathya wrote: > > > On 2016/12/05 09:25:17, jgruber wrote: > > > > Can we replace calls to ResolvePromise by %resolve_promise and remove > > > > ResolvePromise? > > > > > > Nope. We use this in async-await and v8-extras > > > > Couldn't we just export %resolve_promise from the bootstrapper and avoid > > creating the extra closure in promise.js? > > Done. > > PTAL Removal of ResolvePromise lgtm
On 2016/12/06 19:16:58, caitp wrote: > On 2016/12/06 19:10:48, gsathya wrote: > > On 2016/12/05 16:22:51, caitp wrote: > > > https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js > > > File src/js/promise.js (right): > > > > > > > > > https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc... > > > src/js/promise.js:121: %resolve_promise(promise, resolution); > > > On 2016/12/05 16:03:05, gsathya wrote: > > > > On 2016/12/05 09:25:17, jgruber wrote: > > > > > Can we replace calls to ResolvePromise by %resolve_promise and remove > > > > > ResolvePromise? > > > > > > > > Nope. We use this in async-await and v8-extras > > > > > > Couldn't we just export %resolve_promise from the bootstrapper and avoid > > > creating the extra closure in promise.js? > > > > Done. > > > > PTAL > > Removal of ResolvePromise lgtm Thanks caitp! benedikt, jgruber, the CL has changed a fair bit, can you PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gsathya@chromium.org changed reviewers: + ishell@chromium.org
+ishell
Nit: s/spec/test/ in commit message. Nice! lgtm modulo comments. https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:681: typedef CodeStubAssembler::Variable Variable; On 2016/12/05 16:03:05, gsathya wrote: > On 2016/12/05 09:25:17, jgruber wrote: > > As mentioned offline, you might consider switching to the new TF_BUILTIN style > > sooner than later. You won't need these typedefs in every function in that > case. > > I'm going to wait until I finish the rest of the builtins before doing the > cleanup. I currently have 4 levels of dependent patches so I don't want to deal > with too much unnecessary churn right now. Acknowledged, sgtm. https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-... src/builtins/builtins-promise.cc:822: a.GotoIf(a.SmiEqual(has_already_visited, a.SmiConstant(1)), &out); On 2016/12/05 16:03:05, gsathya wrote: > On 2016/12/05 09:25:17, jgruber wrote: > > Is line 825 below the only place where we write into kAlreadyVisitedSlot? > Would > > it make sense to store True/FalseConstant instead of 0/1 Smis? > > PromiseUtils::CreateResolvingFunctions and PromiseRejectClosure also use this. I > think Smi is cheaper though anyway Ok. IMHO the fact that we use a Smi 0/1 encoding is not obvious and we should at least document this well. Perhaps in promise-utils.h together with the contents of all other custom slots? It seems kind of off though that only Promise uses Smi 0/1 to represent true/false. Likewise for JSPromise::kHasHandlerOffset. If you do decide to change it, a follow-up CL is also fine. And out of interest, are Smis really cheaper? It should be a pointer comparison against a constant in both cases. https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc... src/js/promise.js:405: %SetCode(GlobalPromise.prototype.catch, PromiseCatch); On 2016/12/05 16:03:05, gsathya wrote: > On 2016/12/05 09:25:17, jgruber wrote: > > This and below + corresponding changes in bootstrapper seem unrelated? > > I need to install an empty .catch in bootstrapper and set the code here, > otherwise the prototype will change here and my map checks will always fail. Acknowledged. https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-... src/builtins/builtins-promise.cc:360: Node* new_elements = a->AllocateFixedArray(kind, new_capacity, mode); Do you think we need to handle LO space allocation here as well? https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-... src/builtins/builtins-promise.cc:669: CodeStubAssembler::Label* out) { I don't think you gain much through the style IRP(.., out) { Goto(out); } caller() { IRP(...) Bind(&out) Return(UndefinedConstant()); } vs. the simpler IRP(...) { Bind(&out); return UndefinedConstant(); } caller() { Return(IRP(...)); } or IRP(...) { Bind(&out); } caller() { IRP(...); Return(UndefinedConstant()); } But I don't feel too strongly about this. https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-... src/builtins/builtins-promise.cc:779: a->GotoUnless(is_promise, &enqueue); This will always evaluate to true (even if you pass in FalseConstant). Branch/Gotos interpret IntPtrConstant(0) as false and everything else as true. Just skip the SelectBooleanConstant call. https://codereview.chromium.org/2541283002/diff/240001/src/js/async-await.js File src/js/async-await.js (right): https://codereview.chromium.org/2541283002/diff/240001/src/js/async-await.js#... src/js/async-await.js:89: %PromiseMarkAsHandled(promise); Should this be %PromiseMarkAsHandled(throwawayCapability.promise)? https://codereview.chromium.org/2541283002/diff/240001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2541283002/diff/240001/src/objects.h#newcode8916 src/objects.h:8916: DECL_INT_ACCESSORS(has_handler) Please add a comment for content meaning and possible values. BTW, you can use the DECL_BOOLEAN_ACCESSORS convenience functions if you haven't seen them yet. https://codereview.chromium.org/2541283002/diff/240001/src/promise-utils.h File src/promise-utils.h (right): https://codereview.chromium.org/2541283002/diff/240001/src/promise-utils.h#ne... src/promise-utils.h:17: enum PromiseResolvingFunctionContextSlot { Please document these (if not already done elsewhere). Thinking about this some more, are there any situations where we will conflict with other code by blindly storing into specific context slots?
lgtm with a nit: https://codereview.chromium.org/2541283002/diff/240001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2541283002/diff/240001/src/api.cc#newcode7243 src/api.cc:7243: return js_promise->has_handler() == 1; Suggestion: != 0 would probably be nicer here. Or a named constant. https://codereview.chromium.org/2541283002/diff/240001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2541283002/diff/240001/src/objects.h#newcode8916 src/objects.h:8916: DECL_INT_ACCESSORS(has_handler) On 2016/12/07 12:59:38, jgruber wrote: > Please add a comment for content meaning and possible values. > > BTW, you can use the DECL_BOOLEAN_ACCESSORS convenience functions if you haven't > seen them yet. +1. Please turn this into smi "flag" field + boolean "has_handler" accessor as Jakob suggested. It will make caller code look nicer.
littledan@chromium.org changed reviewers: + littledan@chromium.org
https://codereview.chromium.org/2541283002/diff/240001/test/mjsunit/harmony/a... File test/mjsunit/harmony/async-await-no-constructor.js (left): https://codereview.chromium.org/2541283002/diff/240001/test/mjsunit/harmony/a... test/mjsunit/harmony/async-await-no-constructor.js:27: assertEquals(0, count); Rather than deleting this test, would it be possible to change it to check for the right behavior that this code sample should've done? This would form a regression test against the spec violation that existed before. This would just be changing 0 to the observed value.
Description was changed from ========== [promises] Port ResolvePromise to TF -- Moves promiseHasHandlerSymbol to inobject property -- Ports PromiseResolveClosure to TF -- Deletes non spec async-await spec which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). BUG=v8:5343 ========== to ========== [promises] Port ResolvePromise to TF -- Moves promiseHasHandlerSymbol to inobject property -- Ports PromiseResolveClosure to TF -- Deletes non spec async-await test which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). 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...
Description was changed from ========== [promises] Port ResolvePromise to TF -- Moves promiseHasHandlerSymbol to inobject property -- Ports PromiseResolveClosure to TF -- Deletes non spec async-await test which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). BUG=v8:5343 ========== to ========== [promises] Port ResolvePromise to TF -- Moves promiseHasHandlerSymbol to inobject property -- Ports PromiseResolveClosure to TF -- Fix a non spec async-await test which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). BUG=v8:5343 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
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/2541283002/diff/240001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2541283002/diff/240001/src/api.cc#newcode7243 src/api.cc:7243: return js_promise->has_handler() == 1; On 2016/12/07 16:23:52, Igor Sheludko wrote: > Suggestion: != 0 would probably be nicer here. Or a named constant. Done. https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-... src/builtins/builtins-promise.cc:360: Node* new_elements = a->AllocateFixedArray(kind, new_capacity, mode); On 2016/12/07 12:59:38, jgruber wrote: > Do you think we need to handle LO space allocation here as well? This is handled in https://codereview.chromium.org/2556483002/ https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-... src/builtins/builtins-promise.cc:669: CodeStubAssembler::Label* out) { On 2016/12/07 12:59:38, jgruber wrote: > I don't think you gain much through the style > > IRP(.., out) { Goto(out); } > caller() { > IRP(...) > Bind(&out) > Return(UndefinedConstant()); > } > > vs. the simpler > > IRP(...) { Bind(&out); return UndefinedConstant(); } > caller() { > Return(IRP(...)); > } > > or > > IRP(...) { Bind(&out); } > caller() { > IRP(...); > Return(UndefinedConstant()); > } > > But I don't feel too strongly about this. I use this here in the follow on patch -- https://codereview.chromium.org/2561443003/diff/60001/src/builtins/builtins-p... https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-... src/builtins/builtins-promise.cc:779: a->GotoUnless(is_promise, &enqueue); On 2016/12/07 12:59:38, jgruber wrote: > This will always evaluate to true (even if you pass in FalseConstant). > Branch/Gotos interpret IntPtrConstant(0) as false and everything else as true. > > Just skip the SelectBooleanConstant call. Done. https://codereview.chromium.org/2541283002/diff/240001/src/js/async-await.js File src/js/async-await.js (right): https://codereview.chromium.org/2541283002/diff/240001/src/js/async-await.js#... src/js/async-await.js:89: %PromiseMarkAsHandled(promise); On 2016/12/07 12:59:38, jgruber wrote: > Should this be %PromiseMarkAsHandled(throwawayCapability.promise)? Yes. https://codereview.chromium.org/2541283002/diff/240001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2541283002/diff/240001/src/objects.h#newcode8916 src/objects.h:8916: DECL_INT_ACCESSORS(has_handler) On 2016/12/07 16:23:52, Igor Sheludko wrote: > On 2016/12/07 12:59:38, jgruber wrote: > > Please add a comment for content meaning and possible values. > > > > BTW, you can use the DECL_BOOLEAN_ACCESSORS convenience functions if you > haven't > > seen them yet. > > +1. Please turn this into smi "flag" field + boolean "has_handler" accessor as > Jakob suggested. It will make caller code look nicer. Done. https://codereview.chromium.org/2541283002/diff/240001/src/promise-utils.h File src/promise-utils.h (right): https://codereview.chromium.org/2541283002/diff/240001/src/promise-utils.h#ne... src/promise-utils.h:17: enum PromiseResolvingFunctionContextSlot { On 2016/12/07 12:59:38, jgruber wrote: > Please document these (if not already done elsewhere). Done. > Thinking about this some more, are there any situations where we will conflict > with other code by blindly storing into specific context slots? These are unused slots, so it's fine https://codereview.chromium.org/2541283002/diff/240001/test/mjsunit/harmony/a... File test/mjsunit/harmony/async-await-no-constructor.js (left): https://codereview.chromium.org/2541283002/diff/240001/test/mjsunit/harmony/a... test/mjsunit/harmony/async-await-no-constructor.js:27: assertEquals(0, count); On 2016/12/07 23:24:58, Dan Ehrenberg wrote: > Rather than deleting this test, would it be possible to change it to check for > the right behavior that this code sample should've done? This would form a > regression test against the spec violation that existed before. This would just > be changing 0 to the observed value. Done
https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:286: namespace { Nit: empty line after namespace { https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:295: void PromiseSetHandler(CodeStubAssembler* a, compiler::Node* promise) { Nit: PromiseSetHasHandler https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:299: Node* const new_flags = a->WordOr( You can perform this operation on the Smi directly. https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:654: namespace { Nit: whitespace after { and before } https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:687: void InternalResolvePromise(CodeStubAssembler* a, compiler::Node* context, Nit: move into the namespace above.
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...
Thanks for the reviews! https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:286: namespace { On 2016/12/08 05:22:30, Benedikt Meurer wrote: > Nit: empty line after namespace { Done. https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:295: void PromiseSetHandler(CodeStubAssembler* a, compiler::Node* promise) { On 2016/12/08 05:22:30, Benedikt Meurer wrote: > Nit: PromiseSetHasHandler Done. https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:299: Node* const new_flags = a->WordOr( On 2016/12/08 05:22:30, Benedikt Meurer wrote: > You can perform this operation on the Smi directly. Done. https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:654: namespace { On 2016/12/08 05:22:30, Benedikt Meurer wrote: > Nit: whitespace after { and before } Done. https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:687: void InternalResolvePromise(CodeStubAssembler* a, compiler::Node* context, On 2016/12/08 05:22:30, Benedikt Meurer wrote: > Nit: move into the namespace above. Done.
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, caitp@igalia.com, jgruber@chromium.org Link to the patchset: https://codereview.chromium.org/2541283002/#ps300001 (title: "fix nits")
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": 300001, "attempt_start_ts": 1481177433659920,
"parent_rev": "03fe0bea54cfbb9e96800bc82c29064b5ac94ede", "commit_rev":
"11f689acfaf2db80a355e4d6660ae7bd3b96c0e0"}
Message was sent while issue was closed.
Description was changed from ========== [promises] Port ResolvePromise to TF -- Moves promiseHasHandlerSymbol to inobject property -- Ports PromiseResolveClosure to TF -- Fix a non spec async-await test which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). BUG=v8:5343 ========== to ========== [promises] Port ResolvePromise to TF -- Moves promiseHasHandlerSymbol to inobject property -- Ports PromiseResolveClosure to TF -- Fix a non spec async-await test which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). BUG=v8:5343 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== [promises] Port ResolvePromise to TF -- Moves promiseHasHandlerSymbol to inobject property -- Ports PromiseResolveClosure to TF -- Fix a non spec async-await test which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). BUG=v8:5343 ========== to ========== [promises] Port ResolvePromise to TF -- Moves promiseHasHandlerSymbol to inobject property -- Ports PromiseResolveClosure to TF -- Fix a non spec async-await test which fails now because we do a map check for native promise check (instead of IsPromise). Changing the constructor (in the test) invalidates the map check. This patch results in a 7.1% performance improvement in the bluebird benchmark (over 5 runs). BUG=v8:5343 Committed: https://crrev.com/11359e331ae98978fe91b6c6925a1569dbd61856 Cr-Commit-Position: refs/heads/master@{#41569} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/11359e331ae98978fe91b6c6925a1569dbd61856 Cr-Commit-Position: refs/heads/master@{#41569}
Message was sent while issue was closed.
jochen@chromium.org changed reviewers: + jochen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2541283002/diff/300001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2541283002/diff/300001/src/js/promise.js#newc... src/js/promise.js:391: "promise_catch", PromiseCatch, fyi, after the %SetCode above, we shouldn't use the original method anymore.. this will crash in the future |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
