proposal-faster-promise-adoption icon indicating copy to clipboard operation
proposal-faster-promise-adoption copied to clipboard

a potential unobservable check for fast-path

Open bakkot opened this issue 2 years ago • 9 comments

Copying from the matrix:

If your fast path is guarded on IsPromise(p) && GetOwnProperty(p, 'then') == undefined && GetPrototypeOf(p) == %Promise.prototype%, that never runs user code (because the IsPromise(p) check rules out proxies, which means the other two checks are unobservable).

This is slightly different from the existing fast-path in PromiseResolve, but that could probably changed to match this; it's getting at the same concept. There should be very few things for which these two tests differ.


EDIT: Per below I think we actually want IsPromise(p) && GetPrototypeOf(p) === C.prototype, where C is the built-in %Promise% in every case except in the actual static .resolve() method (in which case it's the class on which .resolve was invoked). This has the advantage of preserving current behavior (except for the .constructor lookup) when calling PromiseSubclass.resolve(). And it's still unobservable for any actual await, and for the built-in promise resolution functions created by the Promise constructor, because in those cases C will be the native Promise, and the .prototype property of the native Promise is nonwritable/nonconfigurable.

This is very close to the current check in PromiseResolve, which is IsPromise(p) && p.constructor === C, except that it's unobservable (except in the DerivedPromise.resolve() case). So it's unlikely to break anything, even subclasses of Promise.

bakkot avatar Jun 06 '22 23:06 bakkot

I think if we update the PromiseResolve code to perform this instead of the constructor check, then add a similar fast path to Promise Resolve Functions to do PerformPromiseThen, we remove all sync access issues for native promises while still making native promise adoption faster.

jridgewell avatar Jun 07 '22 00:06 jridgewell

This would also simplify other parts of the spec, because await and PromiseResolve(%Promise%, _foo_) would never synchronously throw; e.g. https://github.com/tc39/ecma262/pull/2683 would be unnecessary because AsyncGeneratorAwaitReturn would become infallible.

bakkot avatar Jun 07 '22 01:06 bakkot

Yes, making await foo and return foo safe inside async functions is very much on our wish list. I also believe that it'd simplify a bunch of spec step that currently have to assume something can throw.

mhofman avatar Jun 07 '22 04:06 mhofman

If your fast path is guarded on IsPromise(p) && GetOwnProperty(p, 'then') == undefined && GetPrototypeOf(p) == %Promise.prototype%, that never runs user code (because the IsPromise(p) check rules out proxies, which means the other two checks are unobservable).

The main complication I see and to which I haven't found a solution in the case of Promise.resolve is that we're currently comparing the constructor against the resolve receiver, so that DerivedPromise.resolve will compare the constructor against DerivedPromise. The problem is that we can't magically go from Promise to %Promise.prototype%.

mhofman avatar Jun 07 '22 04:06 mhofman

Oh, true. The fast-path I proposed above would indeed mean changing the behavior for subclasses of the built-in Promise type in a literal call to Promise.resolve or Promise.prototype.finally (which are the only places which pass something other than %Promise% to PromiseResolve). And it's not just a number of ticks thing, because it affects identity: class D extends Promise {}; let x = new D(() => {}); console.log(D.resolve(x) === x) should print true, whereas with my change it would print false. Seems bad.

One option would be to refactor Promise.resolve and Promise.prototype.finally to use slightly different machinery than literal await and other spec machinery which works with native promises - say have a NativePromiseResolve which uses the unobservable IsPromise(p) && GetOwnProperty(p, 'then') == undefined && GetPrototypeOf(p) == %Promise.prototype% check, and a separate PromiseResolve which does the current x.constructor == C check.

bakkot avatar Jun 07 '22 05:06 bakkot

Another option would be to change the IsPromise(x) && x.constructor == C check to instead be IsPromise(x) && C.prototype == x.[[Prototype]] (and maybe also GetOwnProperty(x, 'then') == undefined. This is unobservable when C is %Promise%, since the .prototype property of the native Promise constructor is a non-configurable data property and since looking up the internal [[Prototype]] slot of a non-promise is not observable. This should also preserve the same behavior for subclasses, unless the subclass is doing something weird.

This is basically the same check I originally proposed except using C.prototype instead of %Promise.prototype%. These are equivalent when C is %Promise%.


@mhofman

The problem is that we can't magically go from Promise to %Promise.prototype%.

Can't you just access the .prototype property, as I've suggested above? That's observable for subclasses but not for the native %Promise%.

bakkot avatar Jun 07 '22 06:06 bakkot

Can't you just access the .prototype property, as I've suggested above? That's observable for subclasses but not for the native %Promise%.

I probably shouldn't have use the word magical. It'd just be an unusual species check. I suppose there is precedent for this with instanceof. Indeed since the Promise.prototype property is non-configurable and for the cases that matter C would be Promise, so it should be fine. In the case of .resolve() I assume that you already trust the receiver. And finally has implementation divergences which need fixing, so we may have an opportunity to change its behavior without compatibility issues.

mhofman avatar Jun 07 '22 07:06 mhofman

looking up the internal [[Prototype]] slot of a non-promise is not observable.

I'm not following the explicit lookup of [[Prototype]]. Is the goal making explicit what GetPrototype would result in since we know the object is a native promise and not an exotic object?

mhofman avatar Jun 07 '22 07:06 mhofman

I'm not following the explicit lookup of [[Prototype]].

Sorry, there's no difference. It's just a different way of writing the same thing.

bakkot avatar Jun 07 '22 14:06 bakkot