If await operand is already resolved/rejected promise, and the promise's "then" property is not modified, and promise's job queue is empty, we can optimize away Generator/Promise handling, and just return resolved value or throw rejected reason from await. With WIP patch that checks job queue length and promise's status, it becomes ~4 times faster with the following code. ("then" property is not yet checked) code: var start = Date.now(); async function a() { var x = 0; for (let i = 0; i < 123456; i++) { x += await Promise.resolve(7); } return x; }; var V = 0; a().then(x => V = x); drainJobQueue(); var end = Date.now(); print(end - start, "ms"); before: 855 ms after (with incomplete patch): 195 ms Remaining concern is that if the browser's job queue length can also be used as the condition. I'm not sure how other events and event tick works with Promise.

one more thing to check: if the promise's constructor/species are not modified.

The current patch changes the execution order in this test case: --- async function f() { print("p:1"); Promise.resolve(); print("p:2"); await Promise.resolve(); print("p:3"); } f(); Promise.resolve().then(() => print("Hi!")); --- Without patch: Prints "p:1, p:2, Hi!, p:3" With patch: "p:1, p:2, p:3, Hi!" (*) (*) V8 and Chakra produce the same output, so maybe they use a similar optimization?

(In reply to André Bargull from comment #2) > The current patch changes the execution order in this test case: Bleh, that's because if the `value` in step 3 of AsyncFunctionAwait is a thenable, we have to enqueue is PromiseResolveThenableJob first, so in step 8 we can't have a fulfilled Promise, so we have to wait not one, but two microtasks. :( That said, the optimization shouldn't apply at all in this case: we can't omit enqueuing the Promise if any code can run after we'd suspend the Generator: it might enqueue its own microtasks, and it might change things that invalidate our preconditions for the optimization to be valid. In practice that probably means the optimization can only be applied if we're already in a resumed async function, and it's the only frame on the stack. (In reply to Tooru Fujisawa [:arai] from comment #1) > one more thing to check: if the promise's constructor/species are not > modified. Why is that required? It seems like we always use %Promise% and shouldn't need to consult @@species, no?

(In reply to André Bargull from comment #2) > Without patch: Prints "p:1, p:2, Hi!, p:3" > With patch: "p:1, p:2, p:3, Hi!" (*) > > (*) V8 and Chakra produce the same output, so maybe they use a similar > optimization? Thanks! apparently I forgot to count top level code when checking the job queue length. so, in term of js shell, the actual job queue length becomes 0 when it's executing drainJobQueue. not sure how to detect it on about browser.

(In reply to Till Schneidereit [:till] from comment #3) > (In reply to Tooru Fujisawa [:arai] from comment #1) > > one more thing to check: if the promise's constructor/species are not > > modified. > > Why is that required? It seems like we always use %Promise% and shouldn't > need to consult @@species, no? https://tc39.github.io/ecma262/#sec-promise.prototype.then "then" creates new promise with passed promise's @@species.

(In reply to Tooru Fujisawa [:arai] from comment #5) > "then" creates new promise with passed promise's @@species. Oh, right. Ok, so we just need essentially the same preconditions as for all the other Promise optimizations.

So, I think, we should create an API that is called when we're executing last promise job in the queue, and set flag on runtime. and clear the flag by another API or when enqueue-ing new promise job.

or perhaps, we could count the number of queued promises by increment on enqueue and decrement in PromiseReactionJob/PromiseResolveThenableJob ? and set the flag when decrementing from 1 to 0, and reset on increment. if it works, we don't need public API.

Here's updated patch. changes: * check passed promise's "then", "constructor", "constructor[@@species]" * skip when passed value is primitive * check JSRuntime.isExecutingLastPromiseJob flag, that is set/reset by JS::SetIsExecutingLastPromiseJob, and also reset when enqueue-ing new job so, currently the job queue is owned by embedders, and we don't have any control over it, so "guessing" the length might be bad and providing API would be better. here's performance comparison (now it also checks primitive) code: var start = Date.now(); async function a() { var x = 0; for (let i = 0; i < 100000; i++) { x += await 1; x += await Promise.resolve(10); } return x; }; var V = 0; a().then(x => V = x); drainJobQueue(); var end = Date.now(); print(end - start, "ms"); result: before: 858 ms after: 162 ms

Based on the fact that queue is not owned by SpiderMonkey, the embedder should tell us "it's going to execute a promise job, and any job queued by the job will be executed immediately after that", instead of the queue length, since empty queue doesn't mean there won't be any other JS code executed between current job and next job. so the API name should be changed along that.

bug 1193394 may be related, since it touches Promise and MicroTask, and it should affect when and how we can ensure the optimizable condition.

What about JS::SetHasPendingMicroTask()? It's a bit unfortunate that we'd be introducing HTML terminology (task) instead of staying with JS terminology (job), but everything else seems somewhat confusing. JS::{Set,Reset}CanSkipEnqueuingJobs() might be an alternative.

(In reply to Till Schneidereit [:till] from comment #12) > What about JS::SetHasPendingMicroTask()? It's a bit unfortunate that we'd be > introducing HTML terminology (task) instead of staying with JS terminology > (job), but everything else seems somewhat confusing. > > JS::{Set,Reset}CanSkipEnqueuingJobs() might be an alternative. Thanks :) JS::{Set,Reset}CanSkipEnqueuingJobs() sounds better, since the name doesn't imply any underlying conditions partially, and one should check the whole conditions in block comment or somewhere in API document. "HasPendingMicroTask" matches "it's not going to execute the last task" case and "it's executing JavaScript outside micro task" case.

we should wait for bug 1193394 before touching outside SpiderMonkey part.

For now, changed API name and added API call to several placed that touches micro task queue. I'll continue after bug 1193394 gets fixed.

FWIW all the "microtask" related code in Promise.cpp will be going away.

This improves the performance of the testcase in bug 1393712.

the performance gain somehow reduced after rebasing onto current m-c. I'm now bisecting...

(In reply to Tooru Fujisawa [:arai] from comment #18) > the performance gain somehow reduced after rebasing onto current m-c. unfortunately it's from bug 1386534. I'll investigate the reason.

sorry, it was just that my code gets bitrot (species getter is now native) :P

rebased

cleanup for bug 1410695

now bug 1193394 is fixed in firefox 60. wontfix-ing 59.

try run with skipping already-resolved one https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f7e9fe86bb389abb958673d4ff19420d584dc4 skipping already-rejected one seems to need some fix. now disabled.

I think I'm hitting the same issue as what I hit on js shell before. (In reply to Tooru Fujisawa [:arai] from comment #4) > apparently I forgot to count top level code when checking the job queue > length. > > so, in term of js shell, the actual job queue length becomes 0 when it's > executing drainJobQueue. > not sure how to detect it on about browser. So, just counting the length of the queue seems to be insufficient. I'll check how we can detect actually-optimizable situation.

here's minimal testcase that fails with the current impl: let s = ""; async function f() { async function g() { s += "p:1\n"; await Promise.resolve(); s += "p:2\n"; } g(); s += "Hi\n"; } new Promise(r => setTimeout(r, 1000)).then(function() { f(); setTimeout(() => { console.log(s); }, 1000); }); the code after `await Promise.resolve()` in function `g` is executed immediately, before executing the remaining part after `g();` call in function 'f'. This happens if the function `f` is called in onFulfilled, but doesn't happen when it's called on top level code, or directly inside setTimeout callback. So, we should detect the execution of promise resolution/rejection handler.

actually, the issue was also happening in js shell. Promise.resolve().then(async function() { async function g() { print("p:1"); await Promise.resolve(); print("p:2"); } g(); print("Hi!"); }); We cannot optimize `await` inside the async function that is called from another function that doesn't await on the return value. I'll re-think about the optimization algorithm. maybe we can optimize only the top level one, or only if all callers are awaiting on the return value.

I had thought that this optimization would only apply if the function containing the await is the top-most JS frame. That is, after it was resumed at least once. If we can make it work in more cases, such as if all parent frames are resumed async functions, too, then that'd be fantastic. It might make sense to do that as a second step, though.

Thank you for the hint :) here's the patch that applies the optimization only when it's the topmost JS frame (actually, when the 'next' function call for the internal generator is the topmost JS frame) I think we could extend the optimization target later by adding more supported cases into IsTopMostAsyncFunctionCall function in the patch.

This is very exciting to see! \o/ I glanced over the patch and it likes quite straight-forward to me. Unfortunately I won't be able to fully review it immediately, but I'll definitely get to it before the end of the week.

Testing rebased patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa6f521dc01ae3d2fd3b5cb13f624c3d2afa509b&selectedJob=181079080

rebased

rebased

Comment on attachment 8989653 [details] [diff] [review] Optimize away Generator/Promise handling for await in the topmost JS frame with already resolved/rejected Promise. Review of attachment 8989653 [details] [diff] [review]: ----------------------------------------------------------------- xpcom/base/CycleCollectedJSContext.cpp and dom/workers/RuntimeService.cpp should be reviewed by someone else, because I don't know that code base. And don't we need to call "JS::DisableSkipEnqueuingJobs" in <https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/dom/worklet/WorkletThread.cpp#188>, too? Do we have any numbers how often this case occurs in practice? Just asking because I don't have a good feeling how async functions/generators are currently used on the web resp. in browser chrome code. I'm simply curious, but it's okay if you don't have any data. Otherwise looks good to me. Only resetting r?, because I still want to test it a bit and to take another look after the review comments were fixed. Thanks! :-) ::: js/src/builtin/Promise.cpp @@ +3202,5 @@ > +// * promise.then is Promise.prototype.then > +// * promise.constructor is Promise > +// * promise.constructor[@@species] is Promise > +static MOZ_MUST_USE bool > +PromiseOptimizable(JSContext* cx, Handle<PromiseObject*> promise) This function doesn't seem to be able to GC, so the parameter can actually be |PromiseObject*|. @@ +3247,5 @@ > + MOZ_ASSERT(iter.calleeTemplate()->isAsync()); > + > + ++iter; > + > + // The parent frame should be `next` function of the generator that is Nit: Add "the" before `next`. @@ +3259,5 @@ > + return false; > + > + ++iter; > + > + // There should be no more frame. s/frame/frames/ ? @@ +3285,5 @@ > + *canSkip = true; > + return true; > + } > + > + RootedObject obj(cx, &val.toObject()); We don't need to root |obj| here. @@ +3291,5 @@ > + *canSkip = false; > + return true; > + } > + > + Rooted<PromiseObject*> promise(cx, &obj->as<PromiseObject>()); |Handle<PromiseObject*> promise = obj.as<PromiseObject();| in any case. @@ +3299,5 @@ > + *canSkip = false; > + return true; > + } > + > + RootedValue valueOrReason(cx, promise->getFixedSlot(PromiseSlot_ReactionsOrResult)); If we don't plan to call |cx->setPendingException|, we don't need to root |valueOrReason| here. @@ +3309,5 @@ > + } > + > + // FIXME: We could skip, but handling exception is a bit complicated. > + // cx->setPendingException(valueOrReason); > + // return false; Is this something we plan to support in the near future? If there's no such plan, we should probably remove this code for now and update the documentation for JSOP_SKIPAWAIT accordingly. ::: js/src/builtin/Promise.h @@ +149,5 @@ > MOZ_MUST_USE bool > AsyncFunctionAwait(JSContext* cx, Handle<PromiseObject*> resultPromise, HandleValue value); > > +MOZ_MUST_USE bool > +SkipAwait(JSContext* cx, HandleValue val, bool* canSkip, MutableHandleValue resolved); Do you think it makes sense to change the function name to include a "Maybe", "Try", or something to emphasise that we only attempt to skip the `await` instruction, but may fail to do so? ::: js/src/frontend/BytecodeEmitter.cpp @@ +8503,5 @@ > > bool > BytecodeEmitter::emitAwaitInScope(EmitterScope& currentScope) > { > + if (!emit1(JSOP_SKIPAWAIT)) // VALUE RESOLVED CANSKIP Hmm, I don't understand why JSOP_SKIPAWAIT needs to push VALUE and RESOLVED back on the stack, because we only ever need one of the two. Can't we make it so that JSOP_SKIPAWAIT pushes one VALUE_OR_RESOLVE value on the stack and the CANSKIP boolean then describes if VALUE_OR_RESOLVE represents the original value (when CANSKIP is false) or the resolved value (when CANSKIP is true)? That way the resulting byte code doesn't need the additional JSOP_SWAP and JSOP_POP instructions: JSOP_SKIPAWAIT JSOP_IFNE skipped_label JSOP_AWAIT skipped_label: ::: js/src/jit/BaselineCompiler.h @@ +204,5 @@ > _(JSOP_REST) \ > _(JSOP_TOASYNC) \ > _(JSOP_TOASYNCGEN) \ > _(JSOP_TOASYNCITER) \ > + _(JSOP_SKIPAWAIT) \ Can we move this next to JSOP_AWAIT? ::: js/src/jit/VMFunctions.cpp @@ +1505,5 @@ > return GetFunctionThis(cx, frame, res); > } > > +MOZ_MUST_USE bool > +VMSkipAwait(JSContext* cx, HandleValue val, MutableHandleValue resolved) Nit: Move to the end to match with position in VMFunctions.h. ::: js/src/jit/VMFunctions.h @@ +953,5 @@ > > extern const VMFunction SetObjectElementInfo; > > +MOZ_MUST_USE bool > +VMSkipAwait(JSContext* cx, HandleValue val, MutableHandleValue resolved); I don't think we normally prefix VM-Functions explicitly with "VM", do we? ::: js/src/jsapi.h @@ +4107,5 @@ > SetPromiseRejectionTrackerCallback(JSContext* cx, JSPromiseRejectionTrackerCallback callback, > void* data = nullptr); > > /** > + * Tell runtime that next promise job is the last promise job in the job queue. "Tell the runtime that the next promise [...]" The doc-comment should probably be a bit longer and definitely needs to point out that the caller is required to call DisableSkipEnqueuingJobs to re-enable the standard behaviour, otherwise they're going to have a bad time. ;-) @@ +4110,5 @@ > /** > + * Tell runtime that next promise job is the last promise job in the job queue. > + */ > +extern JS_PUBLIC_API(void) > +EnableSkipEnqueuingJobsForNextAsyncFunction(JSContext* cx); I'm not sure this is a good name for a public API. It's too long, it's too specific and just doesn't feel right. Just to be clear, I wouldn't mind this name for SpiderMonkey internal functionality, I'd probably even prefer to have this naming here. :-) Also I think we should switch the prefixes for both functions, because "Enable" always sounds like something you want to have, whereas the connotation of "Disable" often means an unwanted behaviour. @@ +4114,5 @@ > +EnableSkipEnqueuingJobsForNextAsyncFunction(JSContext* cx); > + > +/** > + * Tell runtime that next promise job is not the last promise job in the job > + * queue and stop optimization. The doc-comment should describe that calling this function undoes "EnableSkipEnqueuingJobsForNextAsyncFunction" and re-enables the standard, default job queueing behaviour. Specifically, we need to make sure that JSAPI users know that they are required to call this function before enqueuing new promise jobs when they have previously called "EnableSkipEnqueuingJobsForNextAsyncFunction". ::: js/src/vm/JSContext.cpp @@ +1044,5 @@ > JS::HandleObject allocationSite, > JS::HandleObject incumbentGlobal, void* data) > { > MOZ_ASSERT(job); > + JS::DisableSkipEnqueuingJobs(cx); We also need to call "DisableSkipEnqueuingJobs" in <https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/js/src/vm/JSContext.cpp#1090>, right? @@ +1152,5 @@ > cx->jobQueue->get()[i] = nullptr; > AutoRealm ar(cx, job); > { > + if (i == cx->jobQueue->length() - 1) > + JS::EnableSkipEnqueuingJobsForNextAsyncFunction(cx); Can we move this before the AutoRealm? And maybe also add a short comment given that |js::RunJobs| is currently relatively comment heavy, so an additional comment matches the current style for this function. @@ +1172,5 @@ > } > } > } > > + JS::DisableSkipEnqueuingJobs(cx); Is it strictly required to call "DisableSkipEnqueuingJobs" here? Can you add a comment describing why we need resp. don't need to call "DisableSkipEnqueuingJobs"? ::: js/src/vm/JSContext.h @@ +889,5 @@ > js::ThreadData<JSGetIncumbentGlobalCallback> getIncumbentGlobalCallback; > js::ThreadData<JSEnqueuePromiseJobCallback> enqueuePromiseJobCallback; > js::ThreadData<void*> enqueuePromiseJobCallbackData; > > + js::ThreadData<bool> canSkipEnqueuingJobs; Move next to |stopDrainingJobQueue| for better packing and to avoid unnecessary padding. ::: js/src/vm/Opcodes.h @@ +2269,5 @@ > + /* > + * Pops the top of stack value as 'value', checks if the await for 'value' > + * can be skipped, and pushes 'value' back on the stack. > + * If the await for 'value' can be skipped and is already resolved, pushes > + * resolved value and 'true' on the stack. Maybe: "If the await operation can be skipped and the resolution value for 'value' can be acquired, pushes the resolution value and 'true' onto the stack." That way we don't need to repeat "await for 'value'" from the first sentence and it avoids "is already resolved" which I found hard to read, because I couldn't easily determine the subject. (The subject is either "the await for 'value'" or is missing, and I'm still not sure which of two it is :-) @@ +2271,5 @@ > + * can be skipped, and pushes 'value' back on the stack. > + * If the await for 'value' can be skipped and is already resolved, pushes > + * resolved value and 'true' on the stack. > + * If the await for 'value' can be skipped and is already rejected, throws > + * an exception with rejected reason. Similar here: "If the await operation can be skipped and the rejection value for 'value' can be acquired, throws an exception with the rejection value."

Thank you for reviewing :) I'll rebase my patch onto your patches in bug 1475678. (given that it has alternative to PromiseOptimizable)

(In reply to André Bargull [:anba] from comment #35) > Do we have any numbers how often this case occurs in practice? Just asking > because I don't have a good feeling how async functions/generators are > currently used on the web resp. in browser chrome code. I'm simply curious, > but it's okay if you don't have any data. Since the optimization is currently applied only to resumed async function, the optimizable case is very limited (comment #28 - comment #29). in whole mochitest and webplatform-test, 1.6% of whole `await` is skipped, while other 11.5% of whole `await` are possibly optimizable (applied to fulfilled/rejected Promises or primitive values, and job queue is empty at that point) also, 72.2% of whole `await` is applied to pending Promise.

Addressed review comments. Now JSOP_SKIPAWAIT supports only primitive or fulfilled Promise (the comment about rejected Promise is removed), and it pops the original value when skippable. Also renamed the public APIs to SetNextJobEnquingSkippable/SetNextJobEnquingNonSkippable, which should be more descriptive. also added some more comments when to call them and what they mean. now SetNextJobEnquingNonSkippable (formerly DisableSkipEnqueuingJobs) is called only when adding a new job to the job queue. (removed the call after running the last job)

Comment on attachment 8994731 [details] [diff] [review] Optimize away Generator/Promise handling for await in the topmost JS frame with already resolved/rejected Promise. Review of attachment 8994731 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the review comments applied for the SpiderMonkey parts. The changes for CycleCollectedJSContext.cpp and RuntimeService.cpp should still be reviewed by someone else. ::: js/src/builtin/Promise.cpp @@ +4377,5 @@ > +// We can skip `await` with already resolved value only if the current frame is > +// the topmost JS frame, which guarantees the job enqueued the next time will > +// be executed immediately after this job if this job is the last job. > +// Currently we support the case that the async function is resumed at least > +// once. We first sentence should be split into two. Maybe: --- We can skip `await` with an already resolved value only if the current frame is the topmost JS frame and the current job is the last job in the job queue. This guarantees that any new job enqueued in the current turn will be executed immediately after the current job. Currently we only support skipping jobs when the async function is resumed at least once. --- @@ +4424,5 @@ > + *canSkip = false; > + return true; > + } > + > + if (!val.isObject()) { Maybe add a comment? --- Primitive values cannot be 'thenables', so we can trivially skip the await operation. --- @@ +4438,5 @@ > + } > + > + PromiseObject* promise = &obj->as<PromiseObject>(); > + > + if (!PromiseHasAnyFlag(*promise, PROMISE_FLAG_RESOLVED)) { Better: `if (promise->state() == JS::PromiseState::Pending)` @@ +4449,5 @@ > + *canSkip = false; > + return true; > + } > + > + if (!PromiseHasAnyFlag(*promise, PROMISE_FLAG_FULFILLED)) { Better: `if (promise->state() == JS::PromiseState::Rejected)` @@ +4450,5 @@ > + return true; > + } > + > + if (!PromiseHasAnyFlag(*promise, PROMISE_FLAG_FULFILLED)) { > + // We don't optimize rejected Promise for now. Nit: s/Promise/Promises/ @@ +4455,5 @@ > + *canSkip = false; > + return true; > + } > + > + resolved.set(promise->getFixedSlot(PromiseSlot_ReactionsOrResult)); Better: `resolved.set(promise->value());` ::: js/src/frontend/BytecodeEmitter.cpp @@ +6079,5 @@ > + if (!emit1(JSOP_TRYSKIPAWAIT)) // VALUE_OR_RESOLVED CANSKIP > + return false; > + > + if (!emit1(JSOP_NOT)) // VALUE_OR_RESOLVED !CANSKIP > + return false; Can you file a follow-up bug to enhance (Internal)IfEmitter to have an option to directly emit JSOP_IFNE? ::: js/src/jsapi.h @@ +4058,5 @@ > + * queuing behavior for `await` in async functions and taking optimized path. > + * > + * This function should be called before executing the last job which is > + * removed from the job queue. > + * SetNextJobEnquingNonSkippable should be called whenever the condition breaks. How about this alternative version? --- Inform the runtime that the job queue is empty and the embedding is going to execute its last promise job. The runtime may now choose to skip creating promise jobs for asynchronous execution and instead continue execution synchronously. More specifically, this optimization is used to skip the standard job queuing behavior for `await` operations in async functions. This function may be called before executing the last job in the job queue. When it was called, SetNextJobEnqueuingNonSkippable must be called in order to restore the default job queuing behavior before the embedding enqueues its next job into the job queue. --- @@ +4061,5 @@ > + * removed from the job queue. > + * SetNextJobEnquingNonSkippable should be called whenever the condition breaks. > + */ > +extern JS_PUBLIC_API(void) > +SetNextJobEnquingSkippable(JSContext* cx); Nit: Missing "eu" after "qu" -> SetNextJobEnqueuingSkippable @@ +4069,5 @@ > + * jobs are supposed to be executed before the job enqueued the next time. > + * This undoes SetNextJobEnquingSkippable and re-enables the standard default > + * job queuing behavior. > + * > + * This function should be called whenever enqueuing a job to the job queue. And here: --- Inform the runtime that job queue is no longer empty. The runtime can now no longer skip creating promise jobs for asynchronous execution, because pending jobs in the job queue must be executed first to preserve the FIFO (first in - first out) property of the queue. This effectively undoes SetNextJobEnqueuingSkippable and re-enables the standard job queuing behavior. This function must be called whenever enqueuing a job to the job queue when SetNextJobEnquingSkippable was called previously. --- @@ +4072,5 @@ > + * > + * This function should be called whenever enqueuing a job to the job queue. > + */ > +extern JS_PUBLIC_API(void) > +SetNextJobEnquingNonSkippable(JSContext* cx); Nit: Missing "eu" after "qu".

Thank you for reviewing! > ::: js/src/frontend/BytecodeEmitter.cpp > @@ +6079,5 @@ > > + if (!emit1(JSOP_TRYSKIPAWAIT)) // VALUE_OR_RESOLVED CANSKIP > > + return false; > > + > > + if (!emit1(JSOP_NOT)) // VALUE_OR_RESOLVED !CANSKIP > > + return false; > > Can you file a follow-up bug to enhance (Internal)IfEmitter to have an > option to directly emit JSOP_IFNE? iiuc, JSOP_IFNE is used only for backward jump in while/do-while loops, and Ion supports those cases only. so, we should first support it in Ion, in order to use it here. https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/js/src/jit/IonControlFlow.cpp#313-316 > ControlFlowGenerator::ControlStatus > ControlFlowGenerator::snoopControlFlow(JSOp op) > { > switch (op) { > ... > case JSOP_IFNE: > // We should never reach an IFNE, it's a stopAt point, which will > // trigger closing the loop. > MOZ_CRASH("we should never reach an ifne!"); > > case JSOP_IFEQ: > return processIfStart(JSOP_IFEQ);

Addressed review comments. smaug, can you review Worker/Worklet/CycleCollectedJSContext part of this patch? this patch adds API calls for informing the JS runtime the optimization opportunity.

(In reply to Tooru Fujisawa [:arai] from comment #40) > iiuc, JSOP_IFNE is used only for backward jump in while/do-while loops, and > Ion supports those cases only. so, we should first support it in Ion, in > order to use it here. Ah okay, never mind then. Probably not worth the trouble to implement the Ion support around it for just avoiding one JSOP_POP.

Comment on attachment 8996573 [details] [diff] [review] Optimize away Generator/Promise handling for await in the topmost JS frame with already resolved/rejected Promise. r=anba ok, the idea is to disable the optimization whenever microtask queue changes, so SetNextJobEnqueuingNonSkippable is called. And SetNextJobEnqueuingSkippable enables the optimization. I would probably have named those functions differently, something like JobQueueMayNotBeEmpty and JobQueueIsEmpty, but not my call.

Would be nice to have some tests where promises and MutationObserver callback calls are interleaved.

https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/support/promise-rejection-events.js has a number of tests that involve interleaving promises and mutationobserver callbacks, if I understand correctly.

Thank you for reviewing! (In reply to Olli Pettay [:smaug] from comment #43) > I would probably have named those functions differently, something like > JobQueueMayNotBeEmpty and JobQueueIsEmpty, but not my call. sounds reasonable. I'll rename them. (In reply to Josh Matthews [:jdm] from comment #45) > https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/ > scripting/processing-model-2/unhandled-promise-rejections/support/promise- > rejection-events.js has a number of tests that involve interleaving promises > and mutationobserver callbacks, if I understand correctly. okay, I'll verify again they pass before landing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c59bddb1d688cdb22523317833df797c1a20ccd Bug 1317481 - Optimize away Generator/Promise handling for await in the topmost JS frame with already resolved/rejected Promise. r=anba,smaug