Bug 146828 – Integrate automatic microtask draining into JSC framework and re-enable Promise

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

https://lists.webkit.org/pipermail/webkit-dev/2015-July/027531.html

Add default microtask system to JSC framework and draining the tasks when apilock depth becomes 0.

[Yusuke Suzuki]

Created attachment 256694 [details]
Patch

[Saam Barati]

Comment on attachment 256694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256694&action=review

Looks good to me.

> Source/JavaScriptCore/API/tests/testapi.c:1862
> +            JSStringRef undefined = JSStringCreateWithUTF8CString("function");

This should be renamed to "function" or something like that.

[Saam Barati]

Comment on attachment 256694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256694&action=review

> Source/JavaScriptCore/jsc.cpp:1404
> +        globalObject->vm().drainMicrotasks();

Will this handle the nested queuing of microtasks?

[Yusuke Suzuki]

Comment on attachment 256694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256694&action=review

I'll update the patch to pass the build in OSX (adding export macro to the specific method).

>> Source/JavaScriptCore/jsc.cpp:1404
>> +        globalObject->vm().drainMicrotasks();
> 
> Will this handle the nested queuing of microtasks?

Yes. In drainMicrotasks, we consumes the microtasks like the following,

    while (!m_microtaskQueue.isEmpty())
        m_microtaskQueue.takeFirst()->run();

During the microtask execution, the new microtask may be enqueued.
In that case, the queue becomes non empty and we'll drain it in the next loop iteration.
I added the test for this case in js/promise-nested-microtasks :)

[Yusuke Suzuki]

Created attachment 256697 [details]
Patch

[Yusuke Suzuki]

Created attachment 256699 [details]
Patch

[Yusuke Suzuki]

Created attachment 256700 [details]
Patch

[Yusuke Suzuki]

Currently I'm checking GTK port environment.
I'll check it in OSX to pass the build.

[Yusuke Suzuki]

OK, could you take a look?

[Saam Barati]

Comment on attachment 256700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256700&action=review

Looks good to me. I'd r+ if I could.

> Source/JavaScriptCore/API/tests/testapi.c:1862
> +            JSStringRef undefined = JSStringCreateWithUTF8CString("function");

Name this variable "function" or something similar

> LayoutTests/js/script-tests/promise-nested-microtasks.js:26
> +debug('The promise is not fulfilled now.');

Maybe call this:
"The promise is not fulfilled until after this function call executes"

[Yusuke Suzuki]

Comment on attachment 256700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256700&action=review

Thanks!

>> Source/JavaScriptCore/API/tests/testapi.c:1862
>> +            JSStringRef undefined = JSStringCreateWithUTF8CString("function");
> 
> Name this variable "function" or something similar

Oops! Thanks, nice catch.
I'll fix it :D

>> LayoutTests/js/script-tests/promise-nested-microtasks.js:26
>> +debug('The promise is not fulfilled now.');
> 
> Maybe call this:
> "The promise is not fulfilled until after this function call executes"

Looks nice. Fixed.

[Yusuke Suzuki]

Created attachment 256775 [details]
Patch

[Yusuke Suzuki]

Created attachment 256777 [details]
Patch

[Yusuke Suzuki]

Created attachment 256902 [details]
Patch

[Yusuke Suzuki]

Hi, could anyone take a look?

[Sam Weinig]

Comment on attachment 256902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256902&action=review

> Source/JavaScriptCore/runtime/VM.h:615
> +    WTF::Deque<std::unique_ptr<QueuedTask>> m_microtaskQueue;

I don't think the WTF is needed here.

[Yusuke Suzuki]

Comment on attachment 256902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256902&action=review

Thank you for your review!

>> Source/JavaScriptCore/runtime/VM.h:615
>> +    WTF::Deque<std::unique_ptr<QueuedTask>> m_microtaskQueue;
> 
> I don't think the WTF is needed here.

Dropped `WTF::`.

[Yusuke Suzuki]

Committed r186966: <http://trac.webkit.org/changeset/186966>

[Yusuke Suzuki]

I'm now creating the follow up patch.

[Yusuke Suzuki]

Committed r186970: <http://trac.webkit.org/changeset/186970>