WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
203454
[JSC] Optimize Promise runtime functions
https://bugs.webkit.org/show_bug.cgi?id=203454
Summary
[JSC] Optimize Promise runtime functions
Yusuke Suzuki
Reported
2019-10-26 00:24:36 PDT
[JSC] Optimize Promise runtime functions
Attachments
Patch
(10.31 KB, patch)
2019-10-26 00:26 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(11.62 KB, patch)
2019-10-26 01:20 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(14.33 KB, patch)
2019-10-26 01:42 PDT
,
Yusuke Suzuki
keith_miller
: review+
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2019-10-26 00:26:36 PDT
Created
attachment 382005
[details]
Patch
Yusuke Suzuki
Comment 2
2019-10-26 01:20:09 PDT
Created
attachment 382006
[details]
Patch
Yusuke Suzuki
Comment 3
2019-10-26 01:42:15 PDT
Created
attachment 382007
[details]
Patch
Radar WebKit Bug Importer
Comment 4
2019-10-26 01:48:14 PDT
<
rdar://problem/56644056
>
Yusuke Suzuki
Comment 5
2019-10-26 02:39:34 PDT
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 6
2019-10-26 04:06:44 PDT
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 7
2019-10-28 11:43:59 PDT
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 8
2019-10-28 12:31:48 PDT
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 9
2019-10-28 13:10:05 PDT
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 10
2019-10-28 13:10:47 PDT
Comment on
attachment 382007
[details]
Patch r=me.
Yusuke Suzuki
Comment 11
2019-10-28 13:57:49 PDT
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
Comment 12
2019-10-28 14:14:02 PDT
Committed
r251671
: <
https://trac.webkit.org/changeset/251671
>
Yusuke Suzuki
Comment 13
2019-10-29 20:12:48 PDT
***
Bug 201452
has been marked as a duplicate of this bug. ***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug