V8 Microtask Queue & API

V8 Microtask Queue & API

This patch generalizes Object.observe callbacks and promise resolution into a FIFO queue called a "microtask queue".

It also exposes new V8 API which exposes the microtask queue to the embedder. In particular, it allows the embedder to

-schedule a microtask (EnqueueExternalMicrotask)
-run the microtask queue (RunMicrotasks)
-control whether the microtask queue is run automatically within V8 when the last script exits (SetAutorunMicrotasks).

R=dcarney@chromium.org, rossberg@chromium.org, dcarney, rossberg, svenpanne
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=19344

[rossberg, 2014-02-10 14:02:06]

Adding Dan and Sven as reviewers, who are more familiar with V8 API pitfalls.

[rossberg, 2014-02-10 14:09:00]

Seems GTM. This just exposes the current per-context queue, right? Do you still
plan to land it as is, or will you move to per-isolate first?

https://codereview.chromium.org/154283002/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/154283002/diff/20001/include/v8.h#newcode4585
include/v8.h:4585: * Runs the Microtask Work Queue until empty
The new functions could use comments pointing out that they're experimental for
the time being.

[Sven Panne, 2014-02-10 14:22:02]

A few API-related DBCs...

https://codereview.chromium.org/154283002/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/154283002/diff/20001/include/v8.h#newcode3862
include/v8.h:3862: typedef void (*MicrotaskCallback)();
We should probably add an Isolate* parameter. Every time we didn't add one for
callbacks, we regretted it later...

https://codereview.chromium.org/154283002/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/154283002/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:20553: void ExternalMicrotaskOne() {
Nit: static. Reduces the probability of linker clashes for cctest.

https://codereview.chromium.org/154283002/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:20554: v8::HandleScope scope(CcTest::isolate());
Calling CcTest::isolate() is a strong indicator that the callbacks should
probably get an Isolate* argument. CcTest::isolate() is an ugly hack (basically
Isolate::Current() in disguise) and already scheduled for removal. :-)

[dcarney, 2014-02-10 16:45:33]

We are trying to remove api callbacks that are not v8::Functions when the
callbacks is executed from javascript.  please use a v8::Function for the
microtask callback.  This drastically simplifies the cl and removes the explicit
path through runtime.

[rafaelw, 2014-02-11 00:28:51]

Ok. All done and ready for review again.

Note that this patch now has https://codereview.chromium.org/131413008/ rolled
into it along with per-isolate storage (isolate->heap()->microtask_state()) and
a per-isolate cctest.

https://codereview.chromium.org/154283002/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/154283002/diff/20001/include/v8.h#newcode3862
include/v8.h:3862: typedef void (*MicrotaskCallback)();
On 2014/02/10 14:22:02, Sven Panne wrote:
> We should probably add an Isolate* parameter. Every time we didn't add one for
> callbacks, we regretted it later...

Done.

https://codereview.chromium.org/154283002/diff/20001/include/v8.h#newcode4585
include/v8.h:4585: * Runs the Microtask Work Queue until empty
On 2014/02/10 14:09:01, rossberg wrote:
> The new functions could use comments pointing out that they're experimental
for
> the time being.

Done.

https://codereview.chromium.org/154283002/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/154283002/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:20553: void ExternalMicrotaskOne() {
On 2014/02/10 14:22:02, Sven Panne wrote:
> Nit: static. Reduces the probability of linker clashes for cctest.

Done.

https://codereview.chromium.org/154283002/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:20554: v8::HandleScope scope(CcTest::isolate());
On 2014/02/10 14:22:03, Sven Panne wrote:
> Calling CcTest::isolate() is a strong indicator that the callbacks should
> probably get an Isolate* argument. CcTest::isolate() is an ugly hack
(basically
> Isolate::Current() in disguise) and already scheduled for removal. :-)

Good to know. Done.

[dcarney, 2014-02-11 07:47:12]

https://codereview.chromium.org/154283002/diff/180001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/154283002/diff/180001/include/v8.h#newcode5421
include/v8.h:5421: static const int kEmptyStringRootIndex = 148;
did you run make grokdump and commit the changes?

https://codereview.chromium.org/154283002/diff/180001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/154283002/diff/180001/src/api.cc#newcode6315
src/api.cc:6315: ExternalMicrotaskCallback callback) {
No need for a new ExternalMicrotaskCallback type: just use Handle<Function> here
as the second argument.
Also, I don't see a reason to call all these things ExternalMicrotask
everywhere, just Microtask is maybe better.

[Yang, 2014-02-11 08:06:03]

On 2014/02/11 07:47:12, dcarney wrote:
> https://codereview.chromium.org/154283002/diff/180001/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/154283002/diff/180001/include/v8.h#newcode5421
> include/v8.h:5421: static const int kEmptyStringRootIndex = 148;
> did you run make grokdump and commit the changes?

Good call. It seems that the grokdump files are already out of sync.


> 
> https://codereview.chromium.org/154283002/diff/180001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/154283002/diff/180001/src/api.cc#newcode6315
> src/api.cc:6315: ExternalMicrotaskCallback callback) {
> No need for a new ExternalMicrotaskCallback type: just use Handle<Function>
here
> as the second argument.
> Also, I don't see a reason to call all these things ExternalMicrotask
> everywhere, just Microtask is maybe better.

[rafaelw, 2014-02-11 20:37:43]

https://codereview.chromium.org/154283002/diff/180001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/154283002/diff/180001/include/v8.h#newcode5421
include/v8.h:5421: static const int kEmptyStringRootIndex = 148;
I didn't, but I have now, although I have no idea what grokdump is and when I
need to run it. The changes now included in this patch now clearly exceed the
scope of this patch. I'm assuming that's ok?

On 2014/02/11 07:47:13, dcarney wrote:
> did you run make grokdump and commit the changes?

https://codereview.chromium.org/154283002/diff/180001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/154283002/diff/180001/src/api.cc#newcode6315
src/api.cc:6315: ExternalMicrotaskCallback callback) {
On 2014/02/11 07:47:13, dcarney wrote:
> No need for a new ExternalMicrotaskCallback type: just use Handle<Function>
here
> as the second argument.
> Also, I don't see a reason to call all these things ExternalMicrotask
> everywhere, just Microtask is maybe better.

Done.

[dcarney, 2014-02-12 07:17:14]

lgtm, small nit

https://codereview.chromium.org/154283002/diff/180001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/154283002/diff/180001/include/v8.h#newcode5421
include/v8.h:5421: static const int kEmptyStringRootIndex = 148;
On 2014/02/11 20:37:43, rafaelw wrote:
> I didn't, but I have now, although I have no idea what grokdump is and when I
> need to run it. The changes now included in this patch now clearly exceed the
> scope of this patch. I'm assuming that's ok?

any time you add or remove an instancetype or add or remove heap roots, it must
be run, the changes that it applies are intended

https://codereview.chromium.org/154283002/diff/260001/src/v8.cc
File src/v8.cc (right):

https://codereview.chromium.org/154283002/diff/260001/src/v8.cc#newcode185
src/v8.cc:185: void V8::EnqueueMicrotask(Isolate* isolate, Handle<Object>
microtask) {
this function seems kinds of pointless now, maybe delete

[rossberg, 2014-02-12 10:51:39]

lgtm

[rafaelw, 2014-02-12 21:25:03]

Thanks for your help, everyone.

https://codereview.chromium.org/154283002/diff/180001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/154283002/diff/180001/include/v8.h#newcode5421
include/v8.h:5421: static const int kEmptyStringRootIndex = 148;
Got it. Thanks. What is grokdump used for?

On 2014/02/12 07:17:15, dcarney wrote:
> On 2014/02/11 20:37:43, rafaelw wrote:
> > I didn't, but I have now, although I have no idea what grokdump is and when
I
> > need to run it. The changes now included in this patch now clearly exceed
the
> > scope of this patch. I'm assuming that's ok?
> 
> any time you add or remove an instancetype or add or remove heap roots, it
must
> be run, the changes that it applies are intended

https://codereview.chromium.org/154283002/diff/260001/src/v8.cc
File src/v8.cc (right):

https://codereview.chromium.org/154283002/diff/260001/src/v8.cc#newcode185
src/v8.cc:185: void V8::EnqueueMicrotask(Isolate* isolate, Handle<Object>
microtask) {
Excellent point =-). Removed.

On 2014/02/12 07:17:15, dcarney wrote:
> this function seems kinds of pointless now, maybe delete

[rafaelw, 2014-02-12 22:04:31]

Committed patchset #9 manually as r19344 (presubmit successful).