Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2459283004:
[promises] Move CreateResolvingFunctions to c++ (Closed)
Description[promises] Move CreateResolvingFunctions to c++
- A new runtime function (%create_resolving_functions) is installed to
call the CreateResolvingFunctions builtin from JS.
- Three new builtins are created - resolve and reject functions and a
third function that creates a new JSFunctions from these
resolve/reject builtins.
- The promise reject function is installed on the context temporarily
as internal_promise_reject. This should go away once we remove
PromiseSet.
BUG=v8:5343
Committed: https://crrev.com/cb6c8e48cc8bd31d44d08592cb6c8a6beee3777a
Cr-Commit-Position: refs/heads/master@{#40903}
Patch Set 1 #Patch Set 2 : use promise specific context #Patch Set 3 : refactor #Patch Set 4 : fix test #
Total comments: 19
Patch Set 5 : address comments #Patch Set 6 : rebase #Patch Set 7 : Remove header #
Total comments: 20
Patch Set 8 : review comments #Patch Set 9 : fix enum #
Total comments: 12
Patch Set 10 : Address comments #
Total comments: 6
Patch Set 11 : address comments #
Messages
Total messages: 51 (32 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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15811) 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...
Description was changed from ========== [promises] Move CreateResolvingFunctions to c++ BUG=v8:5343 ========== to ========== [promises] Move CreateResolvingFunctions to c++ - A new runtime function (%create_resolving_functions) is installed to call the CreateResolvingFunctions builtin from JS. - Three new builtins are created - resolve and reject functions and a third function that creates a new JSFunctions from these resolve/reject builtins. - The promise reject function is installed on the context temporarily as internal_promise_reject. This should go away once we remove PromiseSet. - There is a 7% regression in the bluebird benchmarks. Most of the time is spent in Builtin_CreateResolvingFunction which should reduce once we remove the callers and not create a new JSObject (which is used to return the resolve/reject functions to JS). There is overhead from calling NewFunction as well from creating JSFunctions for the builtins as well as overhead from NewPromiseResolvingFunctionContext. BUG=v8:5343 ==========
Description was changed from ========== [promises] Move CreateResolvingFunctions to c++ - A new runtime function (%create_resolving_functions) is installed to call the CreateResolvingFunctions builtin from JS. - Three new builtins are created - resolve and reject functions and a third function that creates a new JSFunctions from these resolve/reject builtins. - The promise reject function is installed on the context temporarily as internal_promise_reject. This should go away once we remove PromiseSet. - There is a 7% regression in the bluebird benchmarks. Most of the time is spent in Builtin_CreateResolvingFunction which should reduce once we remove the callers and not create a new JSObject (which is used to return the resolve/reject functions to JS). There is overhead from calling NewFunction as well from creating JSFunctions for the builtins as well as overhead from NewPromiseResolvingFunctionContext. BUG=v8:5343 ========== to ========== [promises] Move CreateResolvingFunctions to c++ - A new runtime function (%create_resolving_functions) is installed to call the CreateResolvingFunctions builtin from JS. - Three new builtins are created - resolve and reject functions and a third function that creates a new JSFunctions from these resolve/reject builtins. - The promise reject function is installed on the context temporarily as internal_promise_reject. This should go away once we remove PromiseSet. - There is a 7% regression in the bluebird benchmarks. Most of the time is spent in Builtin_CreateResolvingFunction which should reduce once we remove the callers and not create a new JSObject (which is used to return the resolve/reject functions to JS). There is overhead from calling NewFunction as well from creating JSFunctions for the builtins as well as overhead from NewPromiseResolvingFunctionContext. BUG=v8:5343 ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org, neis@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15822) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
Looking good, adding bmeurer to CC since he suggested this approach. https://codereview.chromium.org/2459283004/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2459283004/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:3602: Handle<String> name = isolate()->factory()->InternalizeUtf8String(""); You can use factory()->empty_string() https://codereview.chromium.org/2459283004/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:3610: native_context()->set_promise_resolve_shared_fun(*info); Why are we storing these per context? Could we instead store them per-Isolate? https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:15: Context* context = isolate->context(); This should be stored into a Handle<Context> immediately. https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:16: Handle<Cell> already_visited(Cell::cast(context->get(4)), isolate); These magic numbers have to be defined as constants somewhere (maybe just in this file, if this is the only place they're accessed?). Same goes for all other such magic numbers in this CL. https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:67: isolate->factory()->NewPromiseResolvingFunctionContext( Couldn't you share this context between the two functions? That'd also let you get rid of the Cell indirection. https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:78: Handle<Object> debug_event = args.atOrUndefined(isolate, 2); atOrUndefined seems wrong here, as this is only called internally so we know how many arguments were passed. I think it'd be better to have DCHECK_EQ(3, args.length()); Handle<Promise> promise = args.at<Promise>(1); ... https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:94: isolate->factory()->NewJSObject(isolate->object_function(), NOT_TENURED); What might be even faster would be to change this calling convention to return an array [resolve, reject]. You'd create a FixedArray of length 2 for the backing store, then call Factory::NewJSArrayWIthElements(). https://codereview.chromium.org/2459283004/diff/60001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/2459283004/diff/60001/src/factory.cc#newcode977 src/factory.cc:977: context->set(6, *debug_event); Looks like if you let the caller do the setting, you could have those enums defined only in builtins-promise.cc (and pass in the length too). https://codereview.chromium.org/2459283004/diff/60001/src/heap-symbols.h File src/heap-symbols.h (right): https://codereview.chromium.org/2459283004/diff/60001/src/heap-symbols.h#newc... src/heap-symbols.h:139: V(resolve_string, "resolve") \ Won't need these if you go with the array-based approach. https://codereview.chromium.org/2459283004/diff/60001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2459283004/diff/60001/src/js/promise.js#newco... src/js/promise.js:54: function CreateResolvingFunctions(promise, debugEvent) { Why do we still need this wrapper function?
caitp@igalia.com changed reviewers: + caitp@igalia.com
this looks pretty nice --- in my version of this, I opted to implement CreateResolvingFunctions as both a code stub and in C++, in order to avoid jumping into C++ from the Promise constructor. Obviously this is more/messier work, so i'd like to find out if there's any serious performance hit from always doing it in C++, could be something to tweak later if needed. Do we have a good way to measure this?
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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15837) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
https://codereview.chromium.org/2459283004/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2459283004/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:3602: Handle<String> name = isolate()->factory()->InternalizeUtf8String(""); On 2016/11/08 18:09:54, adamk wrote: > You can use factory()->empty_string() Done. https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:15: Context* context = isolate->context(); On 2016/11/08 18:09:55, adamk wrote: > This should be stored into a Handle<Context> immediately. Done. https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:16: Handle<Cell> already_visited(Cell::cast(context->get(4)), isolate); On 2016/11/08 18:09:55, adamk wrote: > These magic numbers have to be defined as constants somewhere (maybe just in > this file, if this is the only place they're accessed?). Same goes for all other > such magic numbers in this CL. Done. https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:67: isolate->factory()->NewPromiseResolvingFunctionContext( On 2016/11/08 18:09:55, adamk wrote: > Couldn't you share this context between the two functions? That'd also let you > get rid of the Cell indirection. Done. But now this context is completely busted (lacking closure and previous slots). https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:78: Handle<Object> debug_event = args.atOrUndefined(isolate, 2); On 2016/11/08 18:09:55, adamk wrote: > atOrUndefined seems wrong here, as this is only called internally so we know how > many arguments were passed. I think it'd be better to have > > DCHECK_EQ(3, args.length()); > Handle<Promise> promise = args.at<Promise>(1); > ... Nice! TIL args.at https://codereview.chromium.org/2459283004/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:94: isolate->factory()->NewJSObject(isolate->object_function(), NOT_TENURED); On 2016/11/08 18:09:55, adamk wrote: > What might be even faster would be to change this calling convention to return > an array [resolve, reject]. You'd create a FixedArray of length 2 for the > backing store, then call Factory::NewJSArrayWIthElements(). Done. https://codereview.chromium.org/2459283004/diff/60001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/2459283004/diff/60001/src/factory.cc#newcode977 src/factory.cc:977: context->set(6, *debug_event); On 2016/11/08 18:09:55, adamk wrote: > Looks like if you let the caller do the setting, you could have those enums > defined only in builtins-promise.cc (and pass in the length too). NewDebugEvaluateContext and NewCatchContext have a magic length, so I'm leaving the length here as such. But, I guess they set all the values in the function itself, so it's easier to figure out what the magic number means. Let me know if you'd rather have me pass it in as an argument. https://codereview.chromium.org/2459283004/diff/60001/src/heap-symbols.h File src/heap-symbols.h (right): https://codereview.chromium.org/2459283004/diff/60001/src/heap-symbols.h#newc... src/heap-symbols.h:139: V(resolve_string, "resolve") \ On 2016/11/08 18:09:55, adamk wrote: > Won't need these if you go with the array-based approach. Done. https://codereview.chromium.org/2459283004/diff/60001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2459283004/diff/60001/src/js/promise.js#newco... src/js/promise.js:54: function CreateResolvingFunctions(promise, debugEvent) { On 2016/11/08 18:09:55, adamk wrote: > Why do we still need this wrapper function? Done.
On 2016/11/08 18:30:04, caitp wrote: > this looks pretty nice --- in my version of this, I opted to implement > CreateResolvingFunctions as both a code stub and in C++, in order to avoid > jumping into C++ from the Promise constructor. Obviously this is more/messier > work, so i'd like to find out if there's any serious performance hit from always > doing it in C++, could be something to tweak later if needed. Do we have a good > way to measure this? Yeah, I thought about that too. But I figured that could be a potential step 2 if the jump from code stub to c++ is high enough (by showing up on a profile). I'm just using the bluebird benchmark here, but I guess you could have a microbenchmark that counts the number of new promises resolved.
gsathya@chromium.org changed reviewers: + bmeurer@chromium.org
On 2016/11/08 20:09:16, gsathya wrote: > On 2016/11/08 18:30:04, caitp wrote: > > this looks pretty nice --- in my version of this, I opted to implement > > CreateResolvingFunctions as both a code stub and in C++, in order to avoid > > jumping into C++ from the Promise constructor. Obviously this is more/messier > > work, so i'd like to find out if there's any serious performance hit from > always > > doing it in C++, could be something to tweak later if needed. Do we have a > good > > way to measure this? > > Yeah, I thought about that too. But I figured that could be a potential step 2 > if the jump from code stub to c++ is high enough (by showing up on a profile). > > I'm just using the bluebird benchmark here, but I guess you could have a > microbenchmark that counts the number of new promises resolved. What are the numbers on the BB benchmark looking like? Because, if there isn't a measurable negative impact, it would be great if we could keep it to just one implementation (in C++).
Description was changed from ========== [promises] Move CreateResolvingFunctions to c++ - A new runtime function (%create_resolving_functions) is installed to call the CreateResolvingFunctions builtin from JS. - Three new builtins are created - resolve and reject functions and a third function that creates a new JSFunctions from these resolve/reject builtins. - The promise reject function is installed on the context temporarily as internal_promise_reject. This should go away once we remove PromiseSet. - There is a 7% regression in the bluebird benchmarks. Most of the time is spent in Builtin_CreateResolvingFunction which should reduce once we remove the callers and not create a new JSObject (which is used to return the resolve/reject functions to JS). There is overhead from calling NewFunction as well from creating JSFunctions for the builtins as well as overhead from NewPromiseResolvingFunctionContext. BUG=v8:5343 ========== to ========== [promises] Move CreateResolvingFunctions to c++ - A new runtime function (%create_resolving_functions) is installed to call the CreateResolvingFunctions builtin from JS. - Three new builtins are created - resolve and reject functions and a third function that creates a new JSFunctions from these resolve/reject builtins. - The promise reject function is installed on the context temporarily as internal_promise_reject. This should go away once we remove PromiseSet. BUG=v8:5343 ==========
On 2016/11/08 20:13:27, caitp wrote: > On 2016/11/08 20:09:16, gsathya wrote: > > On 2016/11/08 18:30:04, caitp wrote: > > > this looks pretty nice --- in my version of this, I opted to implement > > > CreateResolvingFunctions as both a code stub and in C++, in order to avoid > > > jumping into C++ from the Promise constructor. Obviously this is > more/messier > > > work, so i'd like to find out if there's any serious performance hit from > > always > > > doing it in C++, could be something to tweak later if needed. Do we have a > > good > > > way to measure this? > > > > Yeah, I thought about that too. But I figured that could be a potential step 2 > > if the jump from code stub to c++ is high enough (by showing up on a profile). > > > > I'm just using the bluebird benchmark here, but I guess you could have a > > microbenchmark that counts the number of new promises resolved. > > What are the numbers on the BB benchmark looking like? Because, if there isn't a > measurable negative impact, it would be great if we could keep it to just one > implementation (in C++). I just updated the issue description. I don't see any regression after changing the resolve/reject builtins to use the same shared context and return a JSArray instead of JSObject. The JSArray will also go away once we move all the callers to c++, so I can see more wins from this for now.
On 2016/11/08 20:15:47, gsathya wrote: > On 2016/11/08 20:13:27, caitp wrote: > > On 2016/11/08 20:09:16, gsathya wrote: > > > On 2016/11/08 18:30:04, caitp wrote: > > > > this looks pretty nice --- in my version of this, I opted to implement > > > > CreateResolvingFunctions as both a code stub and in C++, in order to avoid > > > > jumping into C++ from the Promise constructor. Obviously this is > > more/messier > > > > work, so i'd like to find out if there's any serious performance hit from > > > always > > > > doing it in C++, could be something to tweak later if needed. Do we have a > > > good > > > > way to measure this? > > > > > > Yeah, I thought about that too. But I figured that could be a potential step > 2 > > > if the jump from code stub to c++ is high enough (by showing up on a > profile). > > > > > > I'm just using the bluebird benchmark here, but I guess you could have a > > > microbenchmark that counts the number of new promises resolved. > > > > What are the numbers on the BB benchmark looking like? Because, if there isn't > a > > measurable negative impact, it would be great if we could keep it to just one > > implementation (in C++). > > I just updated the issue description. I don't see any regression after changing > the resolve/reject builtins to use the same shared context and return a JSArray > instead of JSObject. > > The JSArray will also go away once we move all the callers to c++, so I can see > more wins from this for now. Nice :)
https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:11: kAlreadyVisitedSlot = 4, Should this 4 be Context::MIN_CONTEXT_SLOTS or something? https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:16: BUILTIN(PromiseResolveClosure) { Can you add a comment here about what this relates to in the spec? https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:19: Handle<Object> value = args.atOrUndefined(isolate, 1); Please move this down to where it's actually used, no need to pull it out before the bailout. https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:33: maybe_result = Execution::Call(isolate, isolate->promise_resolve(), RETURN_FAILURE_ON_EXCEPTION here, I think. https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:39: BUILTIN(PromiseRejectClosure) { Same here, spec reference? And below. https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:42: Handle<Object> value = args.atOrUndefined(isolate, 1); Same thing, this can move down to where it's used. https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:57: maybe_result = Execution::Call(isolate, isolate->promise_internal_reject(), RETURN_FAILURE_ON_EXCEPTION https://codereview.chromium.org/2459283004/diff/120001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/2459283004/diff/120001/src/factory.cc#newcode966 src/factory.cc:966: Handle<FixedArray> array = NewFixedArray(Context::MIN_CONTEXT_SLOTS + 3); Please pass in this 3; I think we should be able to define it as part of the enum in builtins-promise.cc. https://codereview.chromium.org/2459283004/diff/120001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2459283004/diff/120001/src/js/promise.js#newc... src/js/promise.js:69: executor(callbacks[0], callbacks[1]); Define constants for these 0 and 1? https://codereview.chromium.org/2459283004/diff/120001/src/js/promise.js#newc... src/js/promise.js:616: //TODO(gsathya): Remove this once we update the promise builtin. Super nit: space between "//" and "TODO" please.
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/2459283004/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:11: kAlreadyVisitedSlot = 4, On 2016/11/09 00:06:23, adamk wrote: > Should this 4 be Context::MIN_CONTEXT_SLOTS or something? Done. https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:16: BUILTIN(PromiseResolveClosure) { On 2016/11/09 00:06:23, adamk wrote: > Can you add a comment here about what this relates to in the spec? Done. https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:19: Handle<Object> value = args.atOrUndefined(isolate, 1); On 2016/11/09 00:06:23, adamk wrote: > Please move this down to where it's actually used, no need to pull it out before > the bailout. Done. https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:33: maybe_result = Execution::Call(isolate, isolate->promise_resolve(), On 2016/11/09 00:06:23, adamk wrote: > RETURN_FAILURE_ON_EXCEPTION here, I think. Done. https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:39: BUILTIN(PromiseRejectClosure) { On 2016/11/09 00:06:23, adamk wrote: > Same here, spec reference? And below. Done. https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:42: Handle<Object> value = args.atOrUndefined(isolate, 1); On 2016/11/09 00:06:23, adamk wrote: > Same thing, this can move down to where it's used. Done. https://codereview.chromium.org/2459283004/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:57: maybe_result = Execution::Call(isolate, isolate->promise_internal_reject(), On 2016/11/09 00:06:23, adamk wrote: > RETURN_FAILURE_ON_EXCEPTION Done. https://codereview.chromium.org/2459283004/diff/120001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/2459283004/diff/120001/src/factory.cc#newcode966 src/factory.cc:966: Handle<FixedArray> array = NewFixedArray(Context::MIN_CONTEXT_SLOTS + 3); On 2016/11/09 00:06:23, adamk wrote: > Please pass in this 3; I think we should be able to define it as part of the > enum in builtins-promise.cc. Done. https://codereview.chromium.org/2459283004/diff/120001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2459283004/diff/120001/src/js/promise.js#newc... src/js/promise.js:69: executor(callbacks[0], callbacks[1]); On 2016/11/09 00:06:23, adamk wrote: > Define constants for these 0 and 1? Done. https://codereview.chromium.org/2459283004/diff/120001/src/js/promise.js#newc... src/js/promise.js:616: //TODO(gsathya): Remove this once we update the promise builtin. On 2016/11/09 00:06:23, adamk wrote: > Super nit: space between "//" and "TODO" please. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2459283004/diff/160001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2459283004/diff/160001/src/bootstrapper.cc#ne... src/bootstrapper.cc:3624: factory()->InternalizeUtf8String("create_resolving_functions"), Nit: this guy doesn't need a name IMHO. https://codereview.chromium.org/2459283004/diff/160001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2459283004/diff/160001/src/builtins/builtins-... src/builtins/builtins-promise.cc:37: isolate, Execution::Call(isolate, isolate->promise_resolve(), This is probably a lot slower than the previous JS version as a the PromiseResolve function still lives in JavaScript land. Are you aware of this? https://codereview.chromium.org/2459283004/diff/160001/src/builtins/builtins-... src/builtins/builtins-promise.cc:64: isolate, Execution::Call(isolate, isolate->promise_internal_reject(), Same here. https://codereview.chromium.org/2459283004/diff/160001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2459283004/diff/160001/src/js/promise.js#newc... src/js/promise.js:68: var callbacks = %create_resolving_functions(promise, true); There seems to be no need to have the container object here at all. Can you leave a TODO here, to cleanup this and only allocate the context plus the two JSFunctions (ideally when this becomes a TF builtin)? https://codereview.chromium.org/2459283004/diff/160001/src/js/promise.js#newc... src/js/promise.js:267: var callbacks = %create_resolving_functions(promise, false); Same here. https://codereview.chromium.org/2459283004/diff/160001/src/js/promise.js#newc... src/js/promise.js:301: var callbacks = %create_resolving_functions(promise, debugEvent); And here.
https://codereview.chromium.org/2459283004/diff/160001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2459283004/diff/160001/src/bootstrapper.cc#ne... src/bootstrapper.cc:3624: factory()->InternalizeUtf8String("create_resolving_functions"), On 2016/11/09 05:14:30, Benedikt Meurer wrote: > Nit: this guy doesn't need a name IMHO. Done. https://codereview.chromium.org/2459283004/diff/160001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2459283004/diff/160001/src/builtins/builtins-... src/builtins/builtins-promise.cc:37: isolate, Execution::Call(isolate, isolate->promise_resolve(), On 2016/11/09 05:14:30, Benedikt Meurer wrote: > This is probably a lot slower than the previous JS version as a the > PromiseResolve function still lives in JavaScript land. Are you aware of this? Yes. A follow up patch will move PromiseResolve to C++ https://codereview.chromium.org/2459283004/diff/160001/src/builtins/builtins-... src/builtins/builtins-promise.cc:64: isolate, Execution::Call(isolate, isolate->promise_internal_reject(), On 2016/11/09 05:14:30, Benedikt Meurer wrote: > Same here. Yeah, follow up patch will move PromiseReject to c++ https://codereview.chromium.org/2459283004/diff/160001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2459283004/diff/160001/src/js/promise.js#newc... src/js/promise.js:68: var callbacks = %create_resolving_functions(promise, true); On 2016/11/09 05:14:30, Benedikt Meurer wrote: > There seems to be no need to have the container object here at all. Can you > leave a TODO here, to cleanup this and only allocate the context plus the two > JSFunctions (ideally when this becomes a TF builtin)? Yeah this container exists only because of the piecemeal migration plan. it should go away once we move all the callers to c++. Here's a CL that removes one -- https://codereview.chromium.org/2487053002/ Rest will be moved shortly. Added TODO for now. https://codereview.chromium.org/2459283004/diff/160001/src/js/promise.js#newc... src/js/promise.js:267: var callbacks = %create_resolving_functions(promise, false); On 2016/11/09 05:14:30, Benedikt Meurer wrote: > Same here. Done. https://codereview.chromium.org/2459283004/diff/160001/src/js/promise.js#newc... src/js/promise.js:301: var callbacks = %create_resolving_functions(promise, debugEvent); On 2016/11/09 05:14:30, Benedikt Meurer wrote: > And here. 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/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.
Thanks for the explanations. LGTM from my side then.
And nice work!
lgtm % nits https://codereview.chromium.org/2459283004/diff/180001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2459283004/diff/180001/src/builtins/builtins-... src/builtins/builtins-promise.cc:14: kLength, s/kLength/kPromiseContextLength/ ("kLength" is going to become more ambiguous as this file grows). https://codereview.chromium.org/2459283004/diff/180001/src/contexts.h File src/contexts.h (right): https://codereview.chromium.org/2459283004/diff/180001/src/contexts.h#newcode481 src/contexts.h:481: WHITE_LIST_INDEX = MIN_CONTEXT_SLOTS + 1, Super-nit: Was this added by clang-format? If not, please revert https://codereview.chromium.org/2459283004/diff/180001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/2459283004/diff/180001/src/factory.cc#newcode966 src/factory.cc:966: DCHECK(length >= Context::MIN_CONTEXT_SLOTS); Nit: DCHECK_GE(length, Context::MIN_CONTEXT_SLOTS), which will give a nicer error message if it fails.
https://codereview.chromium.org/2459283004/diff/180001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2459283004/diff/180001/src/builtins/builtins-... src/builtins/builtins-promise.cc:14: kLength, On 2016/11/09 21:36:20, adamk wrote: > s/kLength/kPromiseContextLength/ ("kLength" is going to become more ambiguous as > this file grows). Done. https://codereview.chromium.org/2459283004/diff/180001/src/contexts.h File src/contexts.h (right): https://codereview.chromium.org/2459283004/diff/180001/src/contexts.h#newcode481 src/contexts.h:481: WHITE_LIST_INDEX = MIN_CONTEXT_SLOTS + 1, On 2016/11/09 21:36:20, adamk wrote: > Super-nit: Was this added by clang-format? If not, please revert Done. https://codereview.chromium.org/2459283004/diff/180001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/2459283004/diff/180001/src/factory.cc#newcode966 src/factory.cc:966: DCHECK(length >= Context::MIN_CONTEXT_SLOTS); On 2016/11/09 21:36:21, adamk wrote: > Nit: DCHECK_GE(length, Context::MIN_CONTEXT_SLOTS), which will give a nicer > error message if it fails. Done.
The CQ bit was checked by gsathya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2459283004/#ps200001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [promises] Move CreateResolvingFunctions to c++ - A new runtime function (%create_resolving_functions) is installed to call the CreateResolvingFunctions builtin from JS. - Three new builtins are created - resolve and reject functions and a third function that creates a new JSFunctions from these resolve/reject builtins. - The promise reject function is installed on the context temporarily as internal_promise_reject. This should go away once we remove PromiseSet. BUG=v8:5343 ========== to ========== [promises] Move CreateResolvingFunctions to c++ - A new runtime function (%create_resolving_functions) is installed to call the CreateResolvingFunctions builtin from JS. - Three new builtins are created - resolve and reject functions and a third function that creates a new JSFunctions from these resolve/reject builtins. - The promise reject function is installed on the context temporarily as internal_promise_reject. This should go away once we remove PromiseSet. BUG=v8:5343 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [promises] Move CreateResolvingFunctions to c++ - A new runtime function (%create_resolving_functions) is installed to call the CreateResolvingFunctions builtin from JS. - Three new builtins are created - resolve and reject functions and a third function that creates a new JSFunctions from these resolve/reject builtins. - The promise reject function is installed on the context temporarily as internal_promise_reject. This should go away once we remove PromiseSet. BUG=v8:5343 ========== to ========== [promises] Move CreateResolvingFunctions to c++ - A new runtime function (%create_resolving_functions) is installed to call the CreateResolvingFunctions builtin from JS. - Three new builtins are created - resolve and reject functions and a third function that creates a new JSFunctions from these resolve/reject builtins. - The promise reject function is installed on the context temporarily as internal_promise_reject. This should go away once we remove PromiseSet. BUG=v8:5343 Committed: https://crrev.com/cb6c8e48cc8bd31d44d08592cb6c8a6beee3777a Cr-Commit-Position: refs/heads/master@{#40903} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/cb6c8e48cc8bd31d44d08592cb6c8a6beee3777a Cr-Commit-Position: refs/heads/master@{#40903} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
