Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567033003:
[promises] Port CreateResolvingFunctions to TF (Closed)
Description[promises] Port CreateResolvingFunctions to TF
2% improvement on benchmarks over 5 runs.
BUG=v8:5343
Review-Url: https://codereview.chromium.org/2567033003
Cr-Commit-Position: refs/heads/master@{#41827}
Committed: https://chromium.googlesource.com/v8/v8/+/cc7e0b0eff7b26f784b834247fc89e9fe3b13213
Patch Set 1 #Patch Set 2 : add tests #
Total comments: 23
Patch Set 3 : cleanup #
Total comments: 12
Patch Set 4 : rebase #Patch Set 5 : Remove unused header #
Total comments: 6
Patch Set 6 : fix nits #
Messages
Total messages: 37 (24 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...
Description was changed from ========== [promises] Port CreateResolvingFunctions to TF BUG=v8:5343 ========== to ========== [promises] Port CreateResolvingFunctions to TF 2% improvement on benchmarks over 5 runs. BUG=v8:5343 ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gsathya@chromium.org changed reviewers: + caitp@igalia.com, ishell@google.com, jgruber@chromium.org
lgtm % comments https://codereview.chromium.org/2567033003/diff/20001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2567033003/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:59: a.StoreFixedArrayElement(arr, 0, resolve); We can skip write barriers when arr is guaranteed to be in new space (also below). https://codereview.chromium.org/2567033003/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:62: const ElementsKind kind = FAST_ELEMENTS; If you pull this up you could use it in AllocateFixedArray. https://codereview.chromium.org/2567033003/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:146: var_result.value(), a.BooleanConstant(true), native_context); a.TrueConstant() https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8226: Node* CodeStubAssembler::AllocateFunctionWithMapAndContext(Node* map, Wouldn't 'AllocateFunction' be simpler (since this seems to be a straight port of Factory::NewFunction to CSA). https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8231: Node* code_entry = Why is this one not const? Same below - if you're going with marking nodes const by default (which I personally like), then it probably makes sense to only create non-const Nodes when they will actually be changed. https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8265: StoreContextElement(context, Context::CLOSURE_INDEX, empty_fn); Perhaps this could be refactored to a more generic AllocateFunctionContext here and the additional handling for Promise-specific slots could take place in builtins-promise.h https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8276: std::pair<Node*, Node*> CodeStubAssembler::CreatePromiseResolvingFunctions( This would probably be better as a member of the PromiseBuiltinsAssembler once you get around to using the new CSA style (and as an anonymous-namespace helper before). It's unlikely to be used outside of builtins-promises.h, right? That'd mean you could also remove the promise-utils.h include above. https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.h:1093: void PromiseInit(Node* promise); These could also be moved to PromiseBuiltinsAssembler (see comment on next file).
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/2567033003/diff/20001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2567033003/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:59: a.StoreFixedArrayElement(arr, 0, resolve); On 2016/12/13 10:44:04, jgruber wrote: > We can skip write barriers when arr is guaranteed to be in new space (also > below). Done. https://codereview.chromium.org/2567033003/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:62: const ElementsKind kind = FAST_ELEMENTS; On 2016/12/13 10:44:04, jgruber wrote: > If you pull this up you could use it in AllocateFixedArray. Done. https://codereview.chromium.org/2567033003/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:146: var_result.value(), a.BooleanConstant(true), native_context); On 2016/12/13 10:44:04, jgruber wrote: > a.TrueConstant() Done. https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8226: Node* CodeStubAssembler::AllocateFunctionWithMapAndContext(Node* map, On 2016/12/13 10:44:04, jgruber wrote: > Wouldn't 'AllocateFunction' be simpler (since this seems to be a straight port > of Factory::NewFunction to CSA). Seemed like there's precedent with AllocateHeapNumberWithValue and AllocateHeapNumber. Happy to change if you think it's worthwhile https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8231: Node* code_entry = On 2016/12/13 10:44:04, jgruber wrote: > Why is this one not const? Same below - if you're going with marking nodes const > by default (which I personally like), then it probably makes sense to only > create non-const Nodes when they will actually be changed. I guess. Changed https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8265: StoreContextElement(context, Context::CLOSURE_INDEX, empty_fn); On 2016/12/13 10:44:04, jgruber wrote: > Perhaps this could be refactored to a more generic AllocateFunctionContext here > and the additional handling for Promise-specific slots could take place in > builtins-promise.h All of these are pretty specific to promise https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8276: std::pair<Node*, Node*> CodeStubAssembler::CreatePromiseResolvingFunctions( On 2016/12/13 10:44:05, jgruber wrote: > This would probably be better as a member of the PromiseBuiltinsAssembler once > you get around to using the new CSA style (and as an anonymous-namespace helper > before). It's unlikely to be used outside of builtins-promises.h, right? That'd > mean you could also remove the promise-utils.h include above. These will be used in other builtins I think. caitp? https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.h:1093: void PromiseInit(Node* promise); On 2016/12/13 10:44:05, jgruber wrote: > These could also be moved to PromiseBuiltinsAssembler (see comment on next > file). These will be used in other builtins
https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8226: Node* CodeStubAssembler::AllocateFunctionWithMapAndContext(Node* map, On 2016/12/13 13:01:46, gsathya wrote: > On 2016/12/13 10:44:04, jgruber wrote: > > Wouldn't 'AllocateFunction' be simpler (since this seems to be a straight port > > of Factory::NewFunction to CSA). > > Seemed like there's precedent with AllocateHeapNumberWithValue and > AllocateHeapNumber. Happy to change if you think it's worthwhile Ok, let's wait and see what Igor says, CSA is his baby :) https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8265: StoreContextElement(context, Context::CLOSURE_INDEX, empty_fn); On 2016/12/13 13:01:47, gsathya wrote: > On 2016/12/13 10:44:04, jgruber wrote: > > Perhaps this could be refactored to a more generic AllocateFunctionContext > here > > and the additional handling for Promise-specific slots could take place in > > builtins-promise.h > > All of these are pretty specific to promise Really? Isn't this almost identical to Factory::NewFunctionContext(empty_function)? https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.h:1093: void PromiseInit(Node* promise); On 2016/12/13 13:01:47, gsathya wrote: > On 2016/12/13 10:44:05, jgruber wrote: > > These could also be moved to PromiseBuiltinsAssembler (see comment on next > > file). > > These will be used in other builtins Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8226: Node* CodeStubAssembler::AllocateFunctionWithMapAndContext(Node* map, On 2016/12/13 13:20:52, jgruber wrote: > On 2016/12/13 13:01:46, gsathya wrote: > > On 2016/12/13 10:44:04, jgruber wrote: > > > Wouldn't 'AllocateFunction' be simpler (since this seems to be a straight > port > > > of Factory::NewFunction to CSA). > > > > Seemed like there's precedent with AllocateHeapNumberWithValue and > > AllocateHeapNumber. Happy to change if you think it's worthwhile > > Ok, let's wait and see what Igor says, CSA is his baby :) Acknowledged. https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8265: StoreContextElement(context, Context::CLOSURE_INDEX, empty_fn); On 2016/12/13 13:20:52, jgruber wrote: > On 2016/12/13 13:01:47, gsathya wrote: > > On 2016/12/13 10:44:04, jgruber wrote: > > > Perhaps this could be refactored to a more generic AllocateFunctionContext > > here > > > and the additional handling for Promise-specific slots could take place in > > > builtins-promise.h > > > > All of these are pretty specific to promise > > Really? Isn't this almost identical to > Factory::NewFunctionContext(empty_function)? Can't have the FUNCTION_SCOPE check, can't set previous..
ishell@chromium.org changed reviewers: + ishell@chromium.org
lgtm with nits: https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8226: Node* CodeStubAssembler::AllocateFunctionWithMapAndContext(Node* map, On 2016/12/13 13:29:33, gsathya wrote: > On 2016/12/13 13:20:52, jgruber wrote: > > On 2016/12/13 13:01:46, gsathya wrote: > > > On 2016/12/13 10:44:04, jgruber wrote: > > > > Wouldn't 'AllocateFunction' be simpler (since this seems to be a straight > > port > > > > of Factory::NewFunction to CSA). > > > > > > Seemed like there's precedent with AllocateHeapNumberWithValue and > > > AllocateHeapNumber. Happy to change if you think it's worthwhile > > > > Ok, let's wait and see what Igor says, CSA is his baby :) > > Acknowledged. Let this function be :) https://codereview.chromium.org/2567033003/diff/20001/src/code-stub-assembler... src/code-stub-assembler.cc:8276: std::pair<Node*, Node*> CodeStubAssembler::CreatePromiseResolvingFunctions( On 2016/12/13 13:01:46, gsathya wrote: > On 2016/12/13 10:44:05, jgruber wrote: > > This would probably be better as a member of the PromiseBuiltinsAssembler once > > you get around to using the new CSA style (and as an anonymous-namespace > helper > > before). It's unlikely to be used outside of builtins-promises.h, right? > That'd > > mean you could also remove the promise-utils.h include above. > > These will be used in other builtins I think. caitp? +1. Maybe there should be an assembler class with stuff common to promises and async/await. BTW, I think it's already the time to start using TF_BUILTIN macro in builtins-promise.cc. https://codereview.chromium.org/2567033003/diff/40001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2567033003/diff/40001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:57: Node* const kSize = a.Int32Constant(2); Please make it IntPtrConstant... https://codereview.chromium.org/2567033003/diff/40001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:60: Node* const arr = a.AllocateFixedArray(kind, kSize); and pass here INTPTR_PARAMETERS. 1) INTPTR parameters do not require sign-extension instruction and 2) we will soon remove INTEGER_PARAMETERS completely and make INTPTR_PARAMETERS default instead. https://codereview.chromium.org/2567033003/diff/40001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2567033003/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:8259: StoreMapNoWriteBarrier(context, map); StoreMapNoWriteBarrier(context, Heap::kFunctionContextMapRootIndex); https://codereview.chromium.org/2567033003/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:8266: StoreContextElement(context, Context::CLOSURE_INDEX, empty_fn); We can skip write barriers here and below since the new context is guaranteed to be small enough to be allocated in new space. Maybe add StoreContextElementNoWriteBarrier(). https://codereview.chromium.org/2567033003/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:8282: LoadContextElement(native_context, Context::PROMISE_RESOLVE_SHARED_FUN); Please move the load before the call to respective allocate to make one live value less. https://codereview.chromium.org/2567033003/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:8284: LoadContextElement(native_context, Context::PROMISE_REJECT_SHARED_FUN); Same here.
lgtm
https://codereview.chromium.org/2567033003/diff/40001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2567033003/diff/40001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:57: Node* const kSize = a.Int32Constant(2); On 2016/12/13 23:42:34, Igor Sheludko wrote: > Please make it IntPtrConstant... Done. https://codereview.chromium.org/2567033003/diff/40001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:60: Node* const arr = a.AllocateFixedArray(kind, kSize); On 2016/12/13 23:42:34, Igor Sheludko wrote: > and pass here INTPTR_PARAMETERS. > 1) INTPTR parameters do not require sign-extension instruction and > 2) we will soon remove INTEGER_PARAMETERS completely and make INTPTR_PARAMETERS > default instead. Done. https://codereview.chromium.org/2567033003/diff/40001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2567033003/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:8259: StoreMapNoWriteBarrier(context, map); On 2016/12/13 23:42:34, Igor Sheludko wrote: > StoreMapNoWriteBarrier(context, Heap::kFunctionContextMapRootIndex); Done. https://codereview.chromium.org/2567033003/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:8266: StoreContextElement(context, Context::CLOSURE_INDEX, empty_fn); On 2016/12/13 23:42:34, Igor Sheludko wrote: > We can skip write barriers here and below since the new context is guaranteed to > be small enough to be allocated in new space. Maybe add > StoreContextElementNoWriteBarrier(). Done. https://codereview.chromium.org/2567033003/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:8282: LoadContextElement(native_context, Context::PROMISE_RESOLVE_SHARED_FUN); On 2016/12/13 23:42:34, Igor Sheludko wrote: > Please move the load before the call to respective allocate to make one live > value less. Done. https://codereview.chromium.org/2567033003/diff/40001/src/code-stub-assembler... src/code-stub-assembler.cc:8284: LoadContextElement(native_context, Context::PROMISE_REJECT_SHARED_FUN); On 2016/12/13 23:42:35, Igor Sheludko wrote: > Same here. Done.
gsathya@chromium.org changed reviewers: + hpayer@chromium.org - ishell@google.com
+hannes Hannes, PTAL @ CodeStubAssembler::AllocateFunctionWithMapAndContext -- https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler.cc Do I need to mark this code entry (similar to JSFunction::set_code)?
lgtm with nits: As long as the object is allocated in new space we don't need to trigger any write barriers. https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:8304: Node* const code_entry = BitcastWordToTagged( Please remove this BitcastWordToTagged and... https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:8320: StoreObjectFieldNoWriteBarrier(fun, JSFunction::kCodeEntryOffset, code_entry); ... store code_entry as a MachineType::PointerRepresentation(). https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler... src/code-stub-assembler.h:407: MachineRepresentation rep = MachineRepresentation::kTagged); It will always be tagged. Please remove rep parameter.
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 reviews! https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:8304: Node* const code_entry = BitcastWordToTagged( On 2016/12/19 20:33:32, Igor Sheludko wrote: > Please remove this BitcastWordToTagged and... Done. https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:8320: StoreObjectFieldNoWriteBarrier(fun, JSFunction::kCodeEntryOffset, code_entry); On 2016/12/19 20:33:32, Igor Sheludko wrote: > ... store code_entry as a MachineType::PointerRepresentation(). Done. https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler.h File src/code-stub-assembler.h (right): https://codereview.chromium.org/2567033003/diff/80001/src/code-stub-assembler... src/code-stub-assembler.h:407: MachineRepresentation rep = MachineRepresentation::kTagged); On 2016/12/19 20:33:33, Igor Sheludko wrote: > It will always be tagged. Please remove rep parameter. Done.
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, caitp@igalia.com, ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2567033003/#ps100001 (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": 100001, "attempt_start_ts": 1482185967658880,
"parent_rev": "48a36c7df71c767d2635d0b2d08ed5c93c862a86", "commit_rev":
"cc7e0b0eff7b26f784b834247fc89e9fe3b13213"}
Message was sent while issue was closed.
Description was changed from ========== [promises] Port CreateResolvingFunctions to TF 2% improvement on benchmarks over 5 runs. BUG=v8:5343 ========== to ========== [promises] Port CreateResolvingFunctions to TF 2% improvement on benchmarks over 5 runs. BUG=v8:5343 Review-Url: https://codereview.chromium.org/2567033003 Cr-Commit-Position: refs/heads/master@{#41827} Committed: https://chromium.googlesource.com/v8/v8/+/cc7e0b0eff7b26f784b834247fc89e9fe3b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/v8/v8/+/cc7e0b0eff7b26f784b834247fc89e9fe3b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
