Conversation
|
Have you checked the spec whether that's sound? I remember when promises got introduced in node (and bluebird still was a thing), that there are some corner cases where simply replacing test262 results may not be reliable for this; it doesn't appear to have coverage. |
|
I'll check the spec, I admit I haven't. Thanks for the insight! |
|
I couldn't find an explanation in the spec, but since the Promise prototype is not writable and doesn't use @@species, I don't see how this change could be observed. V8 seems to do the same trick, if I'm reading this correctly: https://github.com/v8/v8/blob/5fe0aa3bc79c0a9d3ad546b79211f07105f09585/src/builtins/promise-resolve.tq#L66 |
|
Looks like V8 also fast-paths .then, if the @@species chain is intact, I'll look into that for a follow-up PR. https://github.com/v8/v8/blob/f9e2258a2c6c7aa50372c496f75db8ddcecff25f/src/builtins/promise-then.tq#L42 |
bnoordhuis
left a comment
There was a problem hiding this comment.
Okay, LGTM then. One final question though: can't you move the this_val == ctx->promise_ctor inside js_new_promise_capability?
|
Nevermind, you're right! It can be made simpler and all call sites for js_new_promise_capability benefit. |
When `this` is the builtin Promise constructor, `js_new_promise_capability` will call `js_promise_new`, which initializes a promise and resolving functions, without the unnecessary executor intermediary.
d8a6ab0 to
dc09bf8
Compare
When
thisis the builtin Promise constructor, calljs_promise_newdirectly instead ofgoing through
js_new_promise_capabilitywhich creates an unnecessary executor intermediary.