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)
Core
JavaScript: Standard Library
Tracking
()
NEW
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
Reporter | ||
Comment 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
> 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)
Reporter | ||
Comment 3•8 years ago
|
||
(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.
Comment 4•8 years ago
|
||
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?
Reporter | ||
Comment 5•8 years ago
|
||
(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.
Updated•7 years ago
|
Priority: -- → P3
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•