Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2604273003:
[csa] Refactor promises API (Closed)
Description[csa] Refactor promises API
This removes all the promise allocation related methods from the CSA
and moves them PromiseBuiltinsAssembler with some edits.
BUG=v8:5343
Review-Url: https://codereview.chromium.org/2604273003
Cr-Commit-Position: refs/heads/master@{#42070}
Committed: https://chromium.googlesource.com/v8/v8/+/81dc09fb98661a59c51d7736a281ef251ffea033
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : remove CSA::Promises API #
Total comments: 4
Patch Set 4 : comments #
Messages
Total messages: 35 (26 generated)
The CQ bit was checked by gsathya@chromium.org to run a 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: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/20152)
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 ========== [CSA] Refactor promises API -- Refactors CSA::AllocatePromise to init the promise as well -- Adds CSA::AllocateAndSetJSPromise which allocates and sets fields in the promise -- Removes CSA::PromiseSet -- Add wrappers around these along with promise hooks and use them in PromiseBuiltinsAssembler BUG=v8:5343 ========== to ========== [CSA] Refactor promises API Previously we relied on the caller to initialize the promise after allocating, instead we always initialize promises after allocation to be safer. -- Refactors CSA::AllocatePromise to init the promise as well -- Adds CSA::AllocateAndSetJSPromise which allocates and sets fields in the promise -- Removes CSA::PromiseSet -- Add wrappers around these along with promise hooks and use them in PromiseBuiltinsAssembler BUG=v8:5343 ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org, caitp@igalia.com, ishell@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [CSA] Refactor promises API Previously we relied on the caller to initialize the promise after allocating, instead we always initialize promises after allocation to be safer. -- Refactors CSA::AllocatePromise to init the promise as well -- Adds CSA::AllocateAndSetJSPromise which allocates and sets fields in the promise -- Removes CSA::PromiseSet -- Add wrappers around these along with promise hooks and use them in PromiseBuiltinsAssembler BUG=v8:5343 ========== to ========== [csa] Refactor promises API Previously we relied on the caller to initialize the promise after allocating, instead we always initialize promises after allocation to be safer. -- Refactors CSA::AllocatePromise to init the promise as well -- Adds CSA::AllocateAndSetJSPromise which allocates and sets fields in the promise -- Removes CSA::PromiseSet -- Add wrappers around these along with promise hooks and use them in PromiseBuiltinsAssembler BUG=v8:5343 ==========
https://codereview.chromium.org/2604273003/diff/20001/src/builtins/builtins-p... File src/builtins/builtins-promise.h (right): https://codereview.chromium.org/2604273003/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.h:20: // call out to the init promise hook. This is really a question for caitp, I think, but do we ever want to allow a builtin to allocate a promise without calling out to the promise hooks? It seems like all promises need to flow through that code anyway.
https://codereview.chromium.org/2604273003/diff/20001/src/builtins/builtins-p... File src/builtins/builtins-promise.h (right): https://codereview.chromium.org/2604273003/diff/20001/src/builtins/builtins-p... src/builtins/builtins-promise.h:20: // call out to the init promise hook. On 2017/01/03 18:23:06, adamk wrote: > This is really a question for caitp, I think, but do we ever want to allow a > builtin to allocate a promise without calling out to the promise hooks? It seems > like all promises need to flow through that code anyway. I'm not sure if there are any situations where we're allowed to avoid the hook call, so presumably not. It's weird that CSA's method with the same name doesn't call the init hook, that seems wrong to me.
Description was changed from ========== [csa] Refactor promises API Previously we relied on the caller to initialize the promise after allocating, instead we always initialize promises after allocation to be safer. -- Refactors CSA::AllocatePromise to init the promise as well -- Adds CSA::AllocateAndSetJSPromise which allocates and sets fields in the promise -- Removes CSA::PromiseSet -- Add wrappers around these along with promise hooks and use them in PromiseBuiltinsAssembler BUG=v8:5343 ========== to ========== [csa] Refactor promises API This removes all the promise allocation related methods from the CSA and moves them PromiseBuiltinsAssembler with some edits. BUG=v8:5343 ==========
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/03 18:32:05, caitp wrote: > https://codereview.chromium.org/2604273003/diff/20001/src/builtins/builtins-p... > File src/builtins/builtins-promise.h (right): > > https://codereview.chromium.org/2604273003/diff/20001/src/builtins/builtins-p... > src/builtins/builtins-promise.h:20: // call out to the init promise hook. > On 2017/01/03 18:23:06, adamk wrote: > > This is really a question for caitp, I think, but do we ever want to allow a > > builtin to allocate a promise without calling out to the promise hooks? It > seems > > like all promises need to flow through that code anyway. > > I'm not sure if there are any situations where we're allowed to avoid the hook > call, so presumably not. > > It's weird that CSA's method with the same name doesn't call the init hook, that > seems wrong to me. I removed the promises API from CSA. You can use the PromiseBuiltinsAssembler to create promises.
https://codereview.chromium.org/2604273003/diff/40001/src/builtins/builtins-p... File src/builtins/builtins-promise.h (right): https://codereview.chromium.org/2604273003/diff/40001/src/builtins/builtins-p... src/builtins/builtins-promise.h:19: Node* AllocateAndInitJSPromise(Node* context); Please add some comments explaining the difference between these; in particular it's not obvious what "Init" means vs "Set" https://codereview.chromium.org/2604273003/diff/40001/src/builtins/builtins-p... src/builtins/builtins-promise.h:63: protected: Can these be private?
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
https://codereview.chromium.org/2604273003/diff/40001/src/builtins/builtins-p... File src/builtins/builtins-promise.h (right): https://codereview.chromium.org/2604273003/diff/40001/src/builtins/builtins-p... src/builtins/builtins-promise.h:19: Node* AllocateAndInitJSPromise(Node* context); On 2017/01/03 23:26:10, adamk wrote: > Please add some comments explaining the difference between these; in particular > it's not obvious what "Init" means vs "Set" Done. https://codereview.chromium.org/2604273003/diff/40001/src/builtins/builtins-p... src/builtins/builtins-promise.h:63: protected: On 2017/01/03 23:26:10, adamk wrote: > Can these be private? Moved allocatejspromise to be private. PromiseInit is used in a builtin (which is a subclass of PromiseBuiltinsAssembler) so can't be private
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gsathya@chromium.org
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": 60001, "attempt_start_ts": 1483549064667680,
"parent_rev": "ba66892332af940fd5b809f30025c3d6e27bdb01", "commit_rev":
"81dc09fb98661a59c51d7736a281ef251ffea033"}
Message was sent while issue was closed.
Description was changed from ========== [csa] Refactor promises API This removes all the promise allocation related methods from the CSA and moves them PromiseBuiltinsAssembler with some edits. BUG=v8:5343 ========== to ========== [csa] Refactor promises API This removes all the promise allocation related methods from the CSA and moves them PromiseBuiltinsAssembler with some edits. BUG=v8:5343 Review-Url: https://codereview.chromium.org/2604273003 Cr-Commit-Position: refs/heads/master@{#42070} Committed: https://chromium.googlesource.com/v8/v8/+/81dc09fb98661a59c51d7736a281ef251ff... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/81dc09fb98661a59c51d7736a281ef251ff... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
