tc39 / proposal-faster-promise-adoption

Reduce the number of microtask ticks required to adopt the state of a promise
https://tc39.es/proposal-faster-promise-adoption
MIT License
48 stars 2 forks source link

a potential unobservable check for fast-path #1

Open bakkot opened 2 years ago

bakkot commented 2 years ago

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.

jridgewell commented 2 years ago

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.

bakkot commented 2 years ago

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.

mhofman commented 2 years ago

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 commented 2 years ago

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%.

bakkot commented 2 years ago

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 commented 2 years ago

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%.

mhofman commented 2 years ago

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 commented 2 years ago

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?

bakkot commented 2 years ago

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.