Introduce v8::MicrotasksScope.

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}

[dgozman, 2016-02-27 03:00:21]

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, 2016-02-27 03:05:41]

dgozman@chromium.org changed reviewers:
+ adamk@chromium.org, jochen@chromium.org

[dgozman, 2016-02-27 03:05:42]

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

[dgozman, 2016-02-27 03:06:52]

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
==========

[jochen (gone - plz use gerrit), 2016-03-01 14:26:08]

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?

[adamk, 2016-03-01 19:10:34]

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"?

[dgozman, 2016-03-03 01:05:16]

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
==========

[dgozman, 2016-03-03 01:22:06]

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.

[adamk, 2016-03-03 01:57:20]

Thanks for the the comments and simplifications, this now lgtm. You might want
to wait for jochen given the changes, though.

[jochen (gone - plz use gerrit), 2016-03-03 05:13:47]

lgtm

[dgozman, 2016-03-03 20:14:06]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-03-03 20:14:06]

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")

[commit-bot: I haz the power, 2016-03-03 20:14:14]

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

[commit-bot: I haz the power, 2016-03-03 20:30:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-03 20:30:26]

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...)

[dgozman, 2016-03-04 00:56:29]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-03-04 00:56:29]

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")

[commit-bot: I haz the power, 2016-03-04 00:56:36]

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

[commit-bot: I haz the power, 2016-03-04 04:01:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-04 04:02:43]

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}
==========

[commit-bot: I haz the power, 2016-03-04 04:02:44]

Patchset 6 (id:??) landed as
https://crrev.com/db77cec242dbdf8ee26da8232fa930270429f253
Cr-Commit-Position: refs/heads/master@{#34472}

[Michael Achenbach, 2016-03-04 07:30:37]

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..

[dgozman, 2016-03-04 17:07:17]

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
==========

[dgozman, 2016-03-04 17:09:37]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-03-04 17:09:37]

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")

[commit-bot: I haz the power, 2016-03-04 17:09:47]

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

[commit-bot: I haz the power, 2016-03-04 17:41:43]

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
==========

[commit-bot: I haz the power, 2016-03-04 17:41:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-04 17:42:23]

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}
==========

[commit-bot: I haz the power, 2016-03-04 17:42:26]

Patchset 7 (id:??) landed as
https://crrev.com/9a1387f6a965edb295931012b4f76c689c02a5c2
Cr-Commit-Position: refs/heads/master@{#34504}