Open Bug 1342044 Opened 8 years ago Updated 2 years ago

Change JSAPI functions for Promise job queuing and triggering to deal with PromiseJob JSObjects instead of JSFunction instances

Categories

(Core :: JavaScript: Standard Library, defect, P3)

defect

Tracking

()

Tracking Status
firefox54 --- affected

People

(Reporter: till, Unassigned)

References

(Blocks 2 open bugs)

Details

When promises get resolved/rejected, the registered callbacks get triggered by creating a JSFunction instance per reaction and enqueuing that by calling cx->runtime()->enqueuePromiseJobCallback[1]. We could do better in two ways: 1. Instead of creating the JSFunction, the enqueuePromiseJobCallback should just take a JSObject for the reaction. We already store the reactions as such objects internally, so we wouldn't need any extra allocations. That would require changing the reaction triggering mechanism to not just invoke a JSFunction, which bz tells me is doable but not trivial. 2. Instead of enqueuing a job per promise reaction, we could probably enqueue a single job for all reactions registered for a promise and then iterate over the reactions inside the engine. AFAICT that wouldn't cause any ordering concerns. It would however potentially be problematic because of the encumbent global handling. I'm not sure we could set that up inside the engine. Perhaps it'd be possible to invoke a JSAPI callback in the, hopefully rare, case where the incumbent global isn't the same as the global the promise reaction was created in? [1] http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/js/src/vm/Runtime.cpp#660
bz, can you comment on the two proposed optimizations? We talked about the first one and you agreed that we should do something like it, but I don't think I asked you about the second one yet.
Flags: needinfo?(bzbarsky)
> Perhaps it'd be possible to invoke a JSAPI callback in the, hopefully rare, > case where the incumbent global isn't the same as the global the promise reaction was created in? That's not good enough. If you don't set up the incumbent global, then the incumbent global can come back null if Gecko asks. What you _could_ do is only set it up in the rare-ish case when the callback to be invoked is not a scripted function. When it's a scripted function, it will set up an incumbent of its own as soon as it's called. More importantly, why would this not cause ordering concerns? In this testcase: var p1 = Promise.resolve(); var p2 = Promise.resolve(); p1.then(() => console.log(1)); p2.then(() => console.log(2)); p1.then(() => console.log(3)); p2.then(() => console.log(4)); what would get logged?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2) > > Perhaps it'd be possible to invoke a JSAPI callback in the, hopefully rare, > > case where the incumbent global isn't the same as the global the promise reaction was created in? > > That's not good enough. If you don't set up the incumbent global, then the > incumbent global can come back null if Gecko asks. > > What you _could_ do is only set it up in the rare-ish case when the callback > to be invoked is not a scripted function. When it's a scripted function, it > will set up an incumbent of its own as soon as it's called. Ok, that makes sense, and seems just as good. > More importantly, why would this not cause ordering concerns? In this > testcase: > > var p1 = Promise.resolve(); > var p2 = Promise.resolve(); > p1.then(() => console.log(1)); > p2.then(() => console.log(2)); > p1.then(() => console.log(3)); > p2.then(() => console.log(4)); > > what would get logged? This'd only apply for promises that actually have queues of pending reactions, i.e. ones where reactions where added before they were resolved. In your scenario, all the reactions would be enqueued immediately, with no real opportunity to common things up. I didn't make that clear at all, sorry.
Ah, ok. So conceptually replacing a list of consecutive reactions for one promise with a single thing that runs all the reactions. That should be fine, yes. Is the idea to minimize setup costs and the number of calls across the jsapi boundary?
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #4) > Is the idea to minimize setup costs and the number of calls across the jsapi boundary? Exactly. It might at that point even make sense to move processing the queue into JS. Not sure how often people add enough callbacks to pending promises for this to matter, though.
Priority: -- → P3
Blocks: jsapi
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.