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
Patch (11.62 KB, patch)
2019-10-26 01:20 PDT, Yusuke Suzuki
no flags
Patch (14.33 KB, patch)
2019-10-26 01:42 PDT, Yusuke Suzuki
keith_miller: review+
ews-watchlist: commit-queue-
Yusuke Suzuki
Comment 1 2019-10-26 00:26:36 PDT
Yusuke Suzuki
Comment 2 2019-10-26 01:20:09 PDT
Yusuke Suzuki
Comment 3 2019-10-26 01:42:15 PDT
Radar WebKit Bug Importer
Comment 4 2019-10-26 01:48:14 PDT
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
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.