Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 10 months ago by dgozman Modified:
4 years, 9 months ago Reviewers:
jochen (gone - plz use gerrit), adamk CC:
Paweł Hajdan Jr., v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionIntroduce v8::MicrotasksScope.
This scope is used to control microtasks execution when MicrotasksPolicy::kScoped is engaged.
Attempt #2. First one was reverted due to chromium breakage: SetAutorunMicrotasks(false) was broken.
BUG=chromium:585949
LOG=Y
TEST=ScopedMicrotasks
Committed: https://crrev.com/9a1387f6a965edb295931012b4f76c689c02a5c2
Cr-Commit-Position: refs/heads/master@{#34504}
Patch Set 1 : Separate scopes #Patch Set 2 : Merged scopes #
Total comments: 10
Patch Set 3 : tests, simplified #Patch Set 4 : nit #Patch Set 5 : bool -> int typo, allow scopes to have no effect when different policy is in place #Patch Set 6 : relaxed DCHECK, fixed new test #Patch Set 7 : SetAutorunMicrotasks fix #
Created: 4 years, 9 months ago
Messages
Total messages: 30 (15 generated)
Description was changed from ========== v8::MicrotasksScope ========== to ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when kScopedMicrotasksInvocation policy is engaged. BUG=585949 ==========
dgozman@chromium.org changed reviewers: + adamk@chromium.org, jochen@chromium.org
Could you please take a look? Here I follow the plan outlined in crbug.com/585949, basically moving essential functionality of V8RecursionScope into v8::MicrotasksScope. On implementation side, we could merge this scope with internal CallDepthScope. First patchset does not merge them, while second does. I'm not sure which one is better - merged one is heavy, but it's one instead of two. Chromium-side patch which uses this: https://codereview.chromium.org/1743763004 Thanks, Dmitry
Description was changed from ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when kScopedMicrotasksInvocation policy is engaged. BUG=585949 ========== to ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when kScopedMicrotasksInvocation policy is engaged. BUG=585949 TEST=no tests yet, but will be before commit ==========
deferring to adamk for the meat of the CL https://codereview.chromium.org/1741893003/diff/20001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1741893003/diff/20001/include/v8.h#newcode5461 include/v8.h:5461: enum MicrotasksPolicy { i'd prefer an enum class https://codereview.chromium.org/1741893003/diff/20001/src/api.h File src/api.h (right): https://codereview.chromium.org/1741893003/diff/20001/src/api.h#newcode438 src/api.h:438: enum MicrotasksPolicy { why not just use the public version?
Found the new code in api.cc pretty hard to read without understanding all the new counters added, will review again once the comments are added. https://codereview.chromium.org/1741893003/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1741893003/diff/20001/src/api.cc#newcode7766 src/api.cc:7766: internal_ = reinterpret_cast<void*>(new CallDepthScope( I'm a little bit worried about the added heap allocation here. I'm wondering if it'd be worth moving CallDepthScope's declaration into the "internal" block in v8.h? https://codereview.chromium.org/1741893003/diff/20001/src/api.h File src/api.h (right): https://codereview.chromium.org/1741893003/diff/20001/src/api.h#newcode489 src/api.h:489: inline void IncrementCallDepth() {call_depth_++;} Given the below new types of "call depth", I think this now deserves a comment as well. https://codereview.chromium.org/1741893003/diff/20001/src/api.h#newcode493 src/api.h:493: inline void IncrementMicrotasksSuppression() {microtasks_suppression_++;} This could use a comment, what is this counter for? https://codereview.chromium.org/1741893003/diff/20001/src/api.h#newcode498 src/api.h:498: inline void IncrementDebugCallDepth() {debug_call_depth_++;} Please add comments, what's a "debug call depth"? https://codereview.chromium.org/1741893003/diff/20001/src/api.h#newcode503 src/api.h:503: inline void IncrementInternalCallDepth() {internal_call_depth_++;} Same here, what's an "internal call depth"?
Description was changed from ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when kScopedMicrotasksInvocation policy is engaged. BUG=585949 TEST=no tests yet, but will be before commit ========== to ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when MicrotasksPolicy::kScoped is engaged. BUG=chromium:585949 LOG=Y TEST=ScopedMicrotasks ==========
PTAL I've added more comments and simplified by not merging scope classes. This makes it easier to review and judge about api. We can work on merging scopes later if that turns out to be a net win. https://codereview.chromium.org/1741893003/diff/20001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1741893003/diff/20001/include/v8.h#newcode5461 include/v8.h:5461: enum MicrotasksPolicy { On 2016/03/01 14:26:08, jochen wrote: > i'd prefer an enum class Done. https://codereview.chromium.org/1741893003/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1741893003/diff/20001/src/api.cc#newcode7766 src/api.cc:7766: internal_ = reinterpret_cast<void*>(new CallDepthScope( On 2016/03/01 19:10:34, adamk wrote: > I'm a little bit worried about the added heap allocation here. I'm wondering if > it'd be worth moving CallDepthScope's declaration into the "internal" block in > v8.h? I think this patch would be easier to understand with separate scopes, so I decided to not merge them. This means heap allocation is gone. https://codereview.chromium.org/1741893003/diff/20001/src/api.h File src/api.h (right): https://codereview.chromium.org/1741893003/diff/20001/src/api.h#newcode438 src/api.h:438: enum MicrotasksPolicy { On 2016/03/01 14:26:08, jochen wrote: > why not just use the public version? Done.
Thanks for the the comments and simplifications, this now lgtm. You might want to wait for jochen given the changes, though.
lgtm
The CQ bit was checked by dgozman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/1741893003/#ps80001 (title: "bool -> int typo, allow scopes to have no effect when different policy is in place")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1741893003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1741893003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_win_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/3732) v8_win_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
The CQ bit was checked by dgozman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/1741893003/#ps100001 (title: "relaxed DCHECK, fixed new test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1741893003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1741893003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when MicrotasksPolicy::kScoped is engaged. BUG=chromium:585949 LOG=Y TEST=ScopedMicrotasks ========== to ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when MicrotasksPolicy::kScoped is engaged. BUG=chromium:585949 LOG=Y TEST=ScopedMicrotasks Committed: https://crrev.com/db77cec242dbdf8ee26da8232fa930270429f253 Cr-Commit-Position: refs/heads/master@{#34472} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/db77cec242dbdf8ee26da8232fa930270429f253 Cr-Commit-Position: refs/heads/master@{#34472}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1762943002/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Speculative. Seems to break a bunch of webkit tests and causes timeouts: https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui... Please rebase upstream if intended..
Message was sent while issue was closed.
Description was changed from ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when MicrotasksPolicy::kScoped is engaged. BUG=chromium:585949 LOG=Y TEST=ScopedMicrotasks Committed: https://crrev.com/db77cec242dbdf8ee26da8232fa930270429f253 Cr-Commit-Position: refs/heads/master@{#34472} ========== to ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when MicrotasksPolicy::kScoped is engaged. Attempt #2. First one was reverted due to chromium breakage: SetAutorunMicrotasks(false) was broken. BUG=chromium:585949 LOG=Y TEST=ScopedMicrotasks ==========
The CQ bit was checked by dgozman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/1741893003/#ps120001 (title: "SetAutorunMicrotasks fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1741893003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1741893003/120001
Message was sent while issue was closed.
Description was changed from ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when MicrotasksPolicy::kScoped is engaged. Attempt #2. First one was reverted due to chromium breakage: SetAutorunMicrotasks(false) was broken. BUG=chromium:585949 LOG=Y TEST=ScopedMicrotasks ========== to ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when MicrotasksPolicy::kScoped is engaged. Attempt #2. First one was reverted due to chromium breakage: SetAutorunMicrotasks(false) was broken. BUG=chromium:585949 LOG=Y TEST=ScopedMicrotasks ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when MicrotasksPolicy::kScoped is engaged. Attempt #2. First one was reverted due to chromium breakage: SetAutorunMicrotasks(false) was broken. BUG=chromium:585949 LOG=Y TEST=ScopedMicrotasks ========== to ========== Introduce v8::MicrotasksScope. This scope is used to control microtasks execution when MicrotasksPolicy::kScoped is engaged. Attempt #2. First one was reverted due to chromium breakage: SetAutorunMicrotasks(false) was broken. BUG=chromium:585949 LOG=Y TEST=ScopedMicrotasks Committed: https://crrev.com/9a1387f6a965edb295931012b4f76c689c02a5c2 Cr-Commit-Position: refs/heads/master@{#34504} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9a1387f6a965edb295931012b4f76c689c02a5c2 Cr-Commit-Position: refs/heads/master@{#34504} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
