Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2497523002:
[promises] Move promise constructor to TFS (Closed)
Description[promises] Move promise constructor to TFS
BUG=v8:5343, chromium:660947, chromium:658194
Committed: https://crrev.com/b361b59fffe5480ebd3eacda9d4a4274d43f8a95
Cr-Commit-Position: refs/heads/master@{#41438}
Patch Set 1 #Patch Set 2 : remove temp code #
Total comments: 20
Patch Set 3 : create internal promise #Patch Set 4 : add ispromise #Patch Set 5 : remove todo #Patch Set 6 : add asm wrapper #Patch Set 7 : add fast path #Patch Set 8 : more fixes #Patch Set 9 : add catch prediction #Patch Set 10 : fix tests #Patch Set 11 : remove unused var #Patch Set 12 : do exact check #Patch Set 13 : fix test #Patch Set 14 : fixees #
Total comments: 3
Patch Set 15 : use jsbuiltinconstruct stub #
Total comments: 22
Patch Set 16 : fixes #Patch Set 17 : more fixes #
Total comments: 1
Patch Set 18 : add goto #
Total comments: 2
Patch Set 19 : fix test #
Total comments: 17
Patch Set 20 : fix nits #
Messages
Total messages: 79 (59 generated)
Description was changed from ========== [WIP] [promises] Move promise constructor to TFS BUG=v8:5343 ========== to ========== [WIP] [promises] Move promise constructor to TFS BUG=v8:5343 ==========
gsathya@chromium.org changed reviewers: + bmeurer@chromium.org
https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:148: a->CallRuntime(Runtime::kCreateResolvingFunctions, context, instance); Is there a way to call a C++ builtin from TFJ? Setting up a runtime function just for this seems like a bad idea. Should CreateResolvingFunction be a TFS? If CreateResolvingFunction needs to be a TFS, I'm not sure how I'd create do the equivalent of NewFunctionFromSharedFunctionInfo in the code stub? The context creation and setting seems straightforward as its just a FixedArray.
bmeurer@chromium.org changed reviewers: + jgruber@chromium.org
First round of comments. Adding jgruber@ since he did the regexp.js recently, so he has a lot of experience with this porting efforts. https://codereview.chromium.org/2497523002/diff/20001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2497523002/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:3590: global_object, "Promise2", Builtins::kPromiseConstructor, 1, false); You cannot say "dont adapt" arguments here, but have the builtin expect exactly one parameter. That will crash with some stack corruption if you pass less or more than one parameter. https://codereview.chromium.org/2497523002/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:3593: shared->SetConstructStub(*isolate()->builtins()->PromiseConstructor()); I'm not sure we currently support having a TFS builtin as construct stub. I suppose we need some smallish ASM wrapper stub around this, which tail calls into the real PromiseConstructor code. https://codereview.chromium.org/2497523002/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:3595: shared->DontAdaptArguments(); This is reundant (and wrong). https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:98: a->Branch(a->WordEqual(new_target, a->UndefinedConstant()), Instead of dynamically checking that new.target is not undefined, we could have a dedicated construct stub for the Promise constructor and implement the normal code object as a simple call to a CPP builtin that just throws. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:148: a->CallRuntime(Runtime::kCreateResolvingFunctions, context, instance); On 2016/11/11 02:48:18, gsathya wrote: > Is there a way to call a C++ builtin from TFJ? Setting up a runtime function > just for this seems like a bad idea. The runtime function is the correct approach here. (a CPP builtin is also just a runtime function with a special setup). > Should CreateResolvingFunction be a TFS? > > If CreateResolvingFunction needs to be a TFS, I'm not sure how I'd create do the > equivalent of NewFunctionFromSharedFunctionInfo in the code stub? > The context creation and setting seems straightforward as its just a FixedArray. It should in TurboFan yes. Allocating JSFunction instances is straight-forward, you just allocate the JSFunction and fill in the approate map, SFI, and context.
Nice! Just a couple of nits from me. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:94: Variable result(a, MachineRepresentation::kTagged), Nit: The naming convention used elsewhere is to prepend var_. It's not consistently used everywhere, but just wanted to mention it. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:107: // calling toString on UndefinedConstant? You probably don't need to call ToString here, just pass in new_target. We will end up calling Object::NoSideEffectsToString in messages.cc:1147. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:152: // TODO(gsathya): Extract resolving functions from JSArray Once all calls to CreateResolvingFunctions are in JS, do you still need to create a JSArray? Maybe it could be a FixedArray (or just two separate functions). https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:163: { a->Return(result.value()); } Nit: You can remove these braces.
https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:127: Callable callable = CodeFactory::ToString(isolate); Danno added ToString to CSA recently, so all CodeFactory::ToString calls could be replaced by a->ToString(context, input) OTOH this inlines a fairly large amount of code, maybe Benedikt can weigh in what the better choice is for performance-insensitive code paths such as this one.
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...
Description was changed from ========== [WIP] [promises] Move promise constructor to TFS BUG=v8:5343 ========== to ========== [WIP] [promises] Move promise constructor to TFS BUG=v8:5343,660947 ==========
Description was changed from ========== [WIP] [promises] Move promise constructor to TFS BUG=v8:5343,660947 ========== to ========== [WIP] [promises] Move promise constructor to TFS BUG=v8:5343,660947,658194 ==========
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: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16587) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16609) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
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...
PTAL, will fix the other backends as well, but wanted to get thoughts on the x64 wrapper first https://codereview.chromium.org/2497523002/diff/20001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2497523002/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:3590: global_object, "Promise2", Builtins::kPromiseConstructor, 1, false); On 2016/11/11 06:50:30, Benedikt Meurer wrote: > You cannot say "dont adapt" arguments here, but have the builtin expect exactly > one parameter. That will crash with some stack corruption if you pass less or > more than one parameter. Done. https://codereview.chromium.org/2497523002/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:3593: shared->SetConstructStub(*isolate()->builtins()->PromiseConstructor()); On 2016/11/11 06:50:30, Benedikt Meurer wrote: > I'm not sure we currently support having a TFS builtin as construct stub. I > suppose we need some smallish ASM wrapper stub around this, which tail calls > into the real PromiseConstructor code. Done. https://codereview.chromium.org/2497523002/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:3595: shared->DontAdaptArguments(); On 2016/11/11 06:50:30, Benedikt Meurer wrote: > This is reundant (and wrong). Done. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:94: Variable result(a, MachineRepresentation::kTagged), On 2016/11/11 08:05:19, jgruber wrote: > Nit: The naming convention used elsewhere is to prepend var_. It's not > consistently used everywhere, but just wanted to mention it. Done. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:98: a->Branch(a->WordEqual(new_target, a->UndefinedConstant()), On 2016/11/11 06:50:31, Benedikt Meurer wrote: > Instead of dynamically checking that new.target is not undefined, we could have > a dedicated construct stub for the Promise constructor and implement the normal > code object as a simple call to a CPP builtin that just throws. The asm wrapper is set as the construct stub, which calls the constructor function. Not sure how to set up three different constructors. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:107: // calling toString on UndefinedConstant? On 2016/11/11 08:05:19, jgruber wrote: > You probably don't need to call ToString here, just pass in new_target. We will > end up calling Object::NoSideEffectsToString in messages.cc:1147. Done. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:127: Callable callable = CodeFactory::ToString(isolate); On 2016/11/11 09:25:43, jgruber wrote: > Danno added ToString to CSA recently, so all CodeFactory::ToString calls could > be replaced by > > a->ToString(context, input) > > OTOH this inlines a fairly large amount of code, maybe Benedikt can weigh in > what the better choice is for performance-insensitive code paths such as this > one. Done. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:152: // TODO(gsathya): Extract resolving functions from JSArray On 2016/11/11 08:05:19, jgruber wrote: > Once all calls to CreateResolvingFunctions are in JS, do you still need to > create a JSArray? Maybe it could be a FixedArray (or just two separate > functions). Moved to a FixedArray for now. Next step is to move CreateResolvingFunction to be a TFJ which will get rid of the runtime call. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:163: { a->Return(result.value()); } On 2016/11/11 08:05:19, jgruber wrote: > Nit: You can remove these braces. Done. https://codereview.chromium.org/2497523002/diff/260001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/260001/src/builtins/builtins-... src/builtins/builtins-promise.cc:154: a.CallRuntime(Runtime::kSetProperty, context, var_result.value(), key, This will go away once we remove the symbol usage in promises https://codereview.chromium.org/2497523002/diff/260001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2497523002/diff/260001/src/objects.h#newcode5691 src/objects.h:5691: DECL_INT_ACCESSORS(exception_hint) Int is probably over kill for this. But, decided to go with it in case we may want to support other types of catch prediction later. Happy to add a comment if you think that'll be useful.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2497523002/diff/260001/src/builtins/x64/built... File src/builtins/x64/builtins-x64.cc (right): https://codereview.chromium.org/2497523002/diff/260001/src/builtins/x64/built... src/builtins/x64/builtins-x64.cc:2842: void Builtins::Generate_PromiseConstructorHelper(MacroAssembler* masm) { Unless I'm missing something, this is 100% identical to the JSBuiltinsConstructStub. Please just use that instead.
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...
On 2016/11/22 06:03:28, Benedikt Meurer wrote: > https://codereview.chromium.org/2497523002/diff/260001/src/builtins/x64/built... > File src/builtins/x64/builtins-x64.cc (right): > > https://codereview.chromium.org/2497523002/diff/260001/src/builtins/x64/built... > src/builtins/x64/builtins-x64.cc:2842: void > Builtins::Generate_PromiseConstructorHelper(MacroAssembler* masm) { > Unless I'm missing something, this is 100% identical to the > JSBuiltinsConstructStub. Please just use that instead. Oh I wasn't sure if we had to change anything else specific to the promise constructor TFJ or if the extra code had an effect on inlining. Changed to use the JSBuiltinsConstructStub.
Description was changed from ========== [WIP] [promises] Move promise constructor to TFS BUG=v8:5343,660947,658194 ========== to ========== [promises] Move promise constructor to TFS BUG=v8:5343,660947,658194 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1818: shared->set_instance_class_name(isolate->heap()->Object_string()); Shouldn't the instance class name be 'Promise'? If so, that is already set by InstallFunction. https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1830: prototype, // can this be shared? Does the PromiseInternalConstructor need a prototype? Otherwise this could be simplified by using SimpleCreateFunction. https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1841: JS_OBJECT_TYPE, JSObject::kHeaderSize); Same here. https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:102: if_rejectpromise(&a, Label::kDeferred); if_rejectpromise could be moved into the run_executor branch. https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:136: a.CallRuntime(Runtime::kNewObject, context, promise_fun, new_target); Just curious, why is it not possible to call a.AllocateJSObjectFromMap in this case? https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:162: var_reject_call.Bind(reject); Seems like you can just use the Node* reject variable instead of var_reject if you move if_rejectpromise into the run_executor block. You could also move the definition of if_rejectpromise and possibly debug_pop in here. https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:194: a.SmiConstant(Smi::FromInt(MessageTemplate::kNotAPromise)); Here and elsewhere: since recently you can just call a.SmiConstant([int]) instead of a.SmiConstant(Smi::FromInt([int])). https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:196: // a.ToString(a, new_target)); Please remove comment. https://codereview.chromium.org/2497523002/diff/280001/src/contexts.h File src/contexts.h (right): https://codereview.chromium.org/2497523002/diff/280001/src/contexts.h#newcode64 src/contexts.h:64: V(PROMISE_INTERNAL_CONSTRUCTOR, JSFunction, promise_internal_constructor) \ It's not followed consistently above, but the general naming convention seems to be [NAME]_INDEX here. https://codereview.chromium.org/2497523002/diff/280001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2497523002/diff/280001/src/js/promise.js#newc... src/js/promise.js:576: %AddNamedProperty(GlobalPromise.prototype, toStringTagSymbol, "Promise", You could also move this to bootstrapper. https://codereview.chromium.org/2497523002/diff/280001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2497523002/diff/280001/src/objects.h#newcode5691 src/objects.h:5691: DECL_INT_ACCESSORS(exception_hint) If this is just a boolean, perhaps it could be turned into a flag bit instead. E.g. KindSpecificFlags1 still seems to have a few available bits.
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/2497523002/diff/280001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1818: shared->set_instance_class_name(isolate->heap()->Object_string()); On 2016/11/22 08:55:55, jgruber wrote: > Shouldn't the instance class name be 'Promise'? If so, that is already set by > InstallFunction. Nope there's a test for this : https://cs.chromium.org/chromium/src/v8/test/mjsunit/es6/mirror-promises.js?q... https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1830: prototype, // can this be shared? On 2016/11/22 08:55:55, jgruber wrote: > Does the PromiseInternalConstructor need a prototype? Otherwise this could be > simplified by using SimpleCreateFunction. Done. https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1841: JS_OBJECT_TYPE, JSObject::kHeaderSize); On 2016/11/22 08:55:55, jgruber wrote: > Same here. Done. https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:102: if_rejectpromise(&a, Label::kDeferred); On 2016/11/22 08:55:55, jgruber wrote: > if_rejectpromise could be moved into the run_executor branch. Done. https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:136: a.CallRuntime(Runtime::kNewObject, context, promise_fun, new_target); On 2016/11/22 08:55:55, jgruber wrote: > Just curious, why is it not possible to call a.AllocateJSObjectFromMap in this > case? The subclass setup seemed more involved and I wasn't sure if it was worth it? I guess we don't have to worry about Proxy subclassing though. https://cs.chromium.org/chromium/src/v8/src/objects.cc?sq=package:chromium&rc... I could call FastNewObjectStub though. Changed. https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:162: var_reject_call.Bind(reject); On 2016/11/22 08:55:55, jgruber wrote: > Seems like you can just use the Node* reject variable instead of var_reject if > you move if_rejectpromise into the run_executor block. You could also move the > definition of if_rejectpromise and possibly debug_pop in here. Done. https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:194: a.SmiConstant(Smi::FromInt(MessageTemplate::kNotAPromise)); On 2016/11/22 08:55:55, jgruber wrote: > Here and elsewhere: since recently you can just call a.SmiConstant([int]) > instead of a.SmiConstant(Smi::FromInt([int])). Done. https://codereview.chromium.org/2497523002/diff/280001/src/builtins/builtins-... src/builtins/builtins-promise.cc:196: // a.ToString(a, new_target)); On 2016/11/22 08:55:55, jgruber wrote: > Please remove comment. Done. https://codereview.chromium.org/2497523002/diff/280001/src/contexts.h File src/contexts.h (right): https://codereview.chromium.org/2497523002/diff/280001/src/contexts.h#newcode64 src/contexts.h:64: V(PROMISE_INTERNAL_CONSTRUCTOR, JSFunction, promise_internal_constructor) \ On 2016/11/22 08:55:55, jgruber wrote: > It's not followed consistently above, but the general naming convention seems to > be [NAME]_INDEX here. Done. https://codereview.chromium.org/2497523002/diff/280001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2497523002/diff/280001/src/js/promise.js#newc... src/js/promise.js:576: %AddNamedProperty(GlobalPromise.prototype, toStringTagSymbol, "Promise", On 2016/11/22 08:55:55, jgruber wrote: > You could also move this to bootstrapper. Done. https://codereview.chromium.org/2497523002/diff/280001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2497523002/diff/280001/src/objects.h#newcode5691 src/objects.h:5691: DECL_INT_ACCESSORS(exception_hint) On 2016/11/22 08:55:55, jgruber wrote: > If this is just a boolean, perhaps it could be turned into a flag bit instead. > E.g. KindSpecificFlags1 still seems to have a few available bits. Yes, agreed. I had a comment on my previous patch about this. I guess it could be a bit for now and refactored later if we have to support other catch prediction https://codereview.chromium.org/2497523002/diff/320001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2497523002/diff/320001/src/objects.h#newcode5557 src/objects.h:5557: inline bool is_promise_rejection(); let me know if you can think of a better name, i couldn't think of anything better.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16741) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16701)
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_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16794) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
Very nice, lgtm modulo test failures. https://codereview.chromium.org/2497523002/diff/340001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/340001/src/builtins/builtins-... src/builtins/builtins-promise.cc:133: Callable fast_new_object_stub = CodeFactory::FastNewObject(isolate); Perhaps Benedikt can chime in whether the usage of FastNewObjectStub is appropriate here. https://codereview.chromium.org/2497523002/diff/340001/src/factory.cc File src/factory.cc (left): https://codereview.chromium.org/2497523002/diff/340001/src/factory.cc#oldcode... src/factory.cc:1603: Nit: Unnecessary whitespace change.
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...
On 2016/11/24 07:42:30, jgruber wrote: > Very nice, lgtm modulo test failures. > > https://codereview.chromium.org/2497523002/diff/340001/src/builtins/builtins-... > File src/builtins/builtins-promise.cc (right): > > https://codereview.chromium.org/2497523002/diff/340001/src/builtins/builtins-... > src/builtins/builtins-promise.cc:133: Callable fast_new_object_stub = > CodeFactory::FastNewObject(isolate); > Perhaps Benedikt can chime in whether the usage of FastNewObjectStub is > appropriate here. > > https://codereview.chromium.org/2497523002/diff/340001/src/factory.cc > File src/factory.cc (left): > > https://codereview.chromium.org/2497523002/diff/340001/src/factory.cc#oldcode... > src/factory.cc:1603: > Nit: Unnecessary whitespace change. Fixed. Benedikt, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bmeurer@chromium.org changed reviewers: + ishell@chromium.org
bmeurer@chromium.org changed required reviewers: + ishell@chromium.org
LGTM from my side. Let Igor have a final look.
caitp@igalia.com changed reviewers: + caitp@igalia.com
some drive-by ideas: https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:236: &if_ispromise, &if_isnotpromise); Why not just `a.Branch(a.HasInstanceType(maybe_promise, JS_PROMISE_TYPE, &if_ispromise, &if_isnotpromise))`? shrinks the function by a couple lines https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins.h File src/builtins/builtins.h (right): https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins.... src/builtins/builtins.h:568: TFJ(IsPromise, 1) \ Maybe for the internal functions, we should skip the arguments adaptor frame by using SharedFunctionInfo::kDontAdaptArgumentsSentinel as the arg count, and just add a CSA_SLOW_ASSERT() to ensure it's the right count in debug builds
Description was changed from ========== [promises] Move promise constructor to TFS BUG=v8:5343,660947,658194 ========== to ========== [promises] Move promise constructor to TFS BUG=v8:5343,chromium:660947,chromium:658194 ==========
Mostly nits. lgtm once IsDebugActive() is addressed. I'm sorry for the delay. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:98: a.GotoIf(a.WordEqual(new_target, a.UndefinedConstant()), You can also write IsUndefined(new_target). https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:152: a.HeapConstant(a.isolate()->factory()->promise_state_symbol()); Nit: LoadRoot(Heap::kpromise_state_symbolRootIndex) to make CSA decide how to generate this constant. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:155: a.CallRuntime(Runtime::kSetProperty, context, var_result.value(), key, // TODO(ishell): Use SetProperty stub once available. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:170: a.Branch(a.IsDebugActive(), &debug_pop, &out); Is it possible that the debugger gets inactive between lines 138 and 170? I'm not sure that the state will stay consistent if that happens. Maybe it's better to call IsDebugActive() only once. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:177: a.Branch(a.IsDebugActive(), &debug_pop, &out); Same here. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:236: &if_ispromise, &if_isnotpromise); On 2016/11/29 13:33:00, caitp wrote: > Why not just `a.Branch(a.HasInstanceType(maybe_promise, JS_PROMISE_TYPE, > &if_ispromise, &if_isnotpromise))`? shrinks the function by a couple lines +1
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. caitp, PTAL https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:98: a.GotoIf(a.WordEqual(new_target, a.UndefinedConstant()), On 2016/12/01 09:25:31, Igor Sheludko wrote: > You can also write IsUndefined(new_target). Done. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:152: a.HeapConstant(a.isolate()->factory()->promise_state_symbol()); On 2016/12/01 09:25:31, Igor Sheludko wrote: > Nit: LoadRoot(Heap::kpromise_state_symbolRootIndex) to make CSA decide how to > generate this constant. Done. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:155: a.CallRuntime(Runtime::kSetProperty, context, var_result.value(), key, On 2016/12/01 09:25:31, Igor Sheludko wrote: > // TODO(ishell): Use SetProperty stub once available. Done. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:170: a.Branch(a.IsDebugActive(), &debug_pop, &out); On 2016/12/01 09:25:31, Igor Sheludko wrote: > Is it possible that the debugger gets inactive between lines 138 and 170? I'm > not sure that the state will stay consistent if that happens. > > Maybe it's better to call IsDebugActive() only once. Inconsistent state is handled so it shouldn't matter (there are other places where we can't help it). But agreed that it's better to be as consistent as possible. Changed. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:177: a.Branch(a.IsDebugActive(), &debug_pop, &out); On 2016/12/01 09:25:31, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:236: &if_ispromise, &if_isnotpromise); On 2016/12/01 09:25:31, Igor Sheludko wrote: > On 2016/11/29 13:33:00, caitp wrote: > > Why not just `a.Branch(a.HasInstanceType(maybe_promise, JS_PROMISE_TYPE, > > &if_ispromise, &if_isnotpromise))`? shrinks the function by a couple lines > > +1 Thanks, done https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins.h File src/builtins/builtins.h (right): https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins.... src/builtins/builtins.h:568: TFJ(IsPromise, 1) \ On 2016/11/29 13:33:00, caitp wrote: > Maybe for the internal functions, we should skip the arguments adaptor frame by > using SharedFunctionInfo::kDontAdaptArgumentsSentinel as the arg count, and just > add a CSA_SLOW_ASSERT() to ensure it's the right count in debug builds AFAIK, this sets up the number of stack parameters which needs to be 1 here. The adaptor frame creation is determined by SharedFunctionInfo::internal_formal_parameter_count (which is set to kDontAdaptArgumentsSentinel in the bootstrapper for this function).
https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... src/builtins/builtins-promise.cc:236: &if_ispromise, &if_isnotpromise); On 2016/12/01 20:03:53, gsathya wrote: > On 2016/12/01 09:25:31, Igor Sheludko wrote: > > On 2016/11/29 13:33:00, caitp wrote: > > > Why not just `a.Branch(a.HasInstanceType(maybe_promise, JS_PROMISE_TYPE, > > > &if_ispromise, &if_isnotpromise))`? shrinks the function by a couple lines > > > > +1 > > Thanks, done jgruber had mentioned on the other CL that you can simplify this even more with `a.Return(a.Select(a.HasInstanceType(...), a.TrueConstant(), a.FalseConstant()))`, or something like that --- but I'm okay with it either way https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins.h File src/builtins/builtins.h (right): https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins.... src/builtins/builtins.h:568: TFJ(IsPromise, 1) \ On 2016/12/01 20:03:53, gsathya wrote: > On 2016/11/29 13:33:00, caitp wrote: > > Maybe for the internal functions, we should skip the arguments adaptor frame > by > > using SharedFunctionInfo::kDontAdaptArgumentsSentinel as the arg count, and > just > > add a CSA_SLOW_ASSERT() to ensure it's the right count in debug builds > > AFAIK, this sets up the number of stack parameters which needs to be 1 here. The > adaptor frame creation is determined by > SharedFunctionInfo::internal_formal_parameter_count (which is set to > kDontAdaptArgumentsSentinel in the bootstrapper for this function). well, what's going on with StringFromCharCode then? I don't know, wasn't CC'd on that bug when that change landed. I'd have to take a closer look to see what this really does.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/01 20:12:38, caitp wrote: > https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... > File src/builtins/builtins-promise.cc (right): > > https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-... > src/builtins/builtins-promise.cc:236: &if_ispromise, &if_isnotpromise); > On 2016/12/01 20:03:53, gsathya wrote: > > On 2016/12/01 09:25:31, Igor Sheludko wrote: > > > On 2016/11/29 13:33:00, caitp wrote: > > > > Why not just `a.Branch(a.HasInstanceType(maybe_promise, JS_PROMISE_TYPE, > > > > &if_ispromise, &if_isnotpromise))`? shrinks the function by a couple lines > > > > > > +1 > > > > Thanks, done > > jgruber had mentioned on the other CL that you can simplify this even more with > `a.Return(a.Select(a.HasInstanceType(...), a.TrueConstant(), > a.FalseConstant()))`, or something like that --- but I'm okay with it either way > > https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins.h > File src/builtins/builtins.h (right): > > https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins.... > src/builtins/builtins.h:568: TFJ(IsPromise, 1) > \ > On 2016/12/01 20:03:53, gsathya wrote: > > On 2016/11/29 13:33:00, caitp wrote: > > > Maybe for the internal functions, we should skip the arguments adaptor frame > > by > > > using SharedFunctionInfo::kDontAdaptArgumentsSentinel as the arg count, and > > just > > > add a CSA_SLOW_ASSERT() to ensure it's the right count in debug builds > > > > AFAIK, this sets up the number of stack parameters which needs to be 1 here. > The > > adaptor frame creation is determined by > > SharedFunctionInfo::internal_formal_parameter_count (which is set to > > kDontAdaptArgumentsSentinel in the bootstrapper for this function). > > well, what's going on with StringFromCharCode then? I don't know, wasn't CC'd on > that bug when that change landed. I'd have to take a closer look to see what > this really does. Hmm you're right. This seems like a more involved change though, added separate CL -- https://codereview.chromium.org/2548633002
The CQ bit was checked by gsathya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jgruber@chromium.org, bmeurer@chromium.org, ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2497523002/#ps380001 (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": 380001, "attempt_start_ts": 1480626464228100,
"parent_rev": "176aa48cf52d29a4a410634e8b11d38ffe4c0589", "commit_rev":
"c94c4090b6c1f59815279a49cd0d3687f5cec534"}
Message was sent while issue was closed.
Description was changed from ========== [promises] Move promise constructor to TFS BUG=v8:5343,chromium:660947,chromium:658194 ========== to ========== [promises] Move promise constructor to TFS BUG=v8:5343,chromium:660947,chromium:658194 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== [promises] Move promise constructor to TFS BUG=v8:5343,chromium:660947,chromium:658194 ========== to ========== [promises] Move promise constructor to TFS BUG=v8:5343,chromium:660947,chromium:658194 Committed: https://crrev.com/b361b59fffe5480ebd3eacda9d4a4274d43f8a95 Cr-Commit-Position: refs/heads/master@{#41438} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/b361b59fffe5480ebd3eacda9d4a4274d43f8a95 Cr-Commit-Position: refs/heads/master@{#41438} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
