Bug 203454 – [JSC] Optimize Promise runtime functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Optimize Promise runtime functions

[Yusuke Suzuki]

Created attachment 382005 [details]
Patch

[Yusuke Suzuki]

Created attachment 382006 [details]
Patch

[Yusuke Suzuki]

Created attachment 382007 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56644056>

[Yusuke Suzuki]

Comment on attachment 382007 [details]
Patch

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

> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:53
>      @resolveWithoutPromise(value,
> -        function(value) { @asyncFunctionResume(generator, promise, value, @GeneratorResumeModeNormal); },
> -        function(error) { @asyncFunctionResume(generator, promise, error, @GeneratorResumeModeThrow); });
> +        function(value) { @asyncFunctionResume(capturedGenerator, capturedPromise, value, @GeneratorResumeModeNormal); },
> +        function(error) { @asyncFunctionResume(capturedGenerator, capturedPromise, error, @GeneratorResumeModeThrow); });

I'm also planning to introduce some special promise-reaction here to avoid 2 function allocations (that is never phantom-ed since it is posted to the next microtask, so we should avoid allocation manually as much as possible).

[EWS Watchlist]

Comment on attachment 382007 [details]
Patch

Attachment 382007 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13180699

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases

[Keith Miller]

Comment on attachment 382007 [details]
Patch

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

>> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:53
>> +        function(error) { @asyncFunctionResume(capturedGenerator, capturedPromise, error, @GeneratorResumeModeThrow); });
> 
> I'm also planning to introduce some special promise-reaction here to avoid 2 function allocations (that is never phantom-ed since it is posted to the next microtask, so we should avoid allocation manually as much as possible).

Yeah, seems like this could be global private method. I guess we'd need to handle them specially though to make sure we pass the state.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:269
> +    var @reject = function @reject(reason) {
>          return @rejectPromiseWithFirstResolvingFunctionCallCheck(capturedPromise, reason);
> -    }
> +    };

Why make this change?

> Source/JavaScriptCore/builtins/PromiseOperations.js:96
> +        return {
> +            @resolve: @resolve,
> +            @reject: @reject,
> +            @promise: promise,
> +        };

Can you just do `return { @resolve, @reject, @promise: promise }` or does that not work for private identifiers?

> JSTests/microbenchmarks/promise-reject.js:3
> +drainMicrotasks();

I don't think you need this? Don't we drain microtasks before exiting anyway?

[Yusuke Suzuki]

Comment on attachment 382007 [details]
Patch

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

Thanks, soon, I'll upload the patch again.

>>> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:53
>>> +        function(error) { @asyncFunctionResume(capturedGenerator, capturedPromise, error, @GeneratorResumeModeThrow); });
>> 
>> I'm also planning to introduce some special promise-reaction here to avoid 2 function allocations (that is never phantom-ed since it is posted to the next microtask, so we should avoid allocation manually as much as possible).
> 
> Yeah, seems like this could be global private method. I guess we'd need to handle them specially though to make sure we pass the state.

Yeah. I think we can create a reaction which holds generator and promise. Then chain this reaction to promise's queue, and it will be resolved appropriately.
Then, we can avoid three allocations here,

1. One for fullfill handler,
2. One for reject handler,
3. One for lexical environment holding capturedGenerator and capturedPromise.

>> Source/JavaScriptCore/builtins/PromiseConstructor.js:269
>> +    };
> 
> Why make this change?

It is hacky but this reduces bytecode size to make bytecodes of this function tight to make inlining available. I'm planning to make global private function resolution as constant loading and make this bytecode further smaller.

>> Source/JavaScriptCore/builtins/PromiseOperations.js:96
>> +        };
> 
> Can you just do `return { @resolve, @reject, @promise: promise }` or does that not work for private identifiers?

I'll fix this.

[Keith Miller]

Comment on attachment 382007 [details]
Patch

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

>>> Source/JavaScriptCore/builtins/PromiseConstructor.js:269
>>> +    };
>> 
>> Why make this change?
> 
> It is hacky but this reduces bytecode size to make bytecodes of this function tight to make inlining available. I'm planning to make global private function resolution as constant loading and make this bytecode further smaller.

Oh, from offline you said this is because we add extra moves when emitting the function var syntax. Can you file a bug to fix those extra moves. It seems like this problem would extend to websites too.

[Keith Miller]

Comment on attachment 382007 [details]
Patch

r=me.

[Yusuke Suzuki]

Comment on attachment 382007 [details]
Patch

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

Thanks!

>>>> Source/JavaScriptCore/builtins/PromiseConstructor.js:269
>>>> +    };
>>> 
>>> Why make this change?
>> 
>> It is hacky but this reduces bytecode size to make bytecodes of this function tight to make inlining available. I'm planning to make global private function resolution as constant loading and make this bytecode further smaller.
> 
> Oh, from offline you said this is because we add extra moves when emitting the function var syntax. Can you file a bug to fix those extra moves. It seems like this problem would extend to websites too.

Filed this in https://bugs.webkit.org/show_bug.cgi?id=203502

[Yusuke Suzuki]

Committed r251671: <https://trac.webkit.org/changeset/251671>

[Yusuke Suzuki]

*** Bug 201452 has been marked as a duplicate of this bug. ***