Lookup resolve property only once in PerformPromiseAll and PerformPromiseRace

[gsathya]

See tc39/proposal-promise-allSettled#40 for background

This would be a change similar to the one proposed with Promise.allSettled

[bakkot]

This change seems consistent with other parts of the language: for example, the Map constructor (and related constructors elsewhere) look up "set" exactly once, outside of the loop.

[mathiasbynens]

It’s also in the spirit of changes we’ve made after-the-fact to the iterator protocol, and then some: #976, #988, #1021, #1398, #1408.

[ljharb]

I’d love to see it happen; do we have any data about web compatibility?

[bakkot]

For this to break anyone, they would need to be replacing Promise.resolve with a not-semantically-equivalent version as a side effect of an iterator passed to Promise.{all, race}, right? (edit: or making access to {PromiseOrPromiseSubclass}.resolve be side-effecting, as Leo points out below.) That seems like the sort of thing which is unlikely to be happening in practice.

I would love web compat data, but this seems like one of those cases where we could decide to make the change without data and hope engines don't find they need to back it out (assuming of course that engines were on board).

[ljharb]

If we’re all comfortable with that, then i am too :-) i agree it seems highly unlikely.

[erights]

agreed

[leobalter]

var promise = new Promise(function() {});

var resolveCount = 0
var proxy = new Proxy(Promise, {
    get(_, name) {
        if (name === 'resolve') {
            resolveCount++;
        }
        return Promise[name];
    }
});

Promise.all.call(proxy, [promise, promise, promise]);

console.log(resolveCount); // should be 1 after this change, it's currently 3

If you prefer avoiding Proxies:

var promise = new Promise(function() {});
var resolveCount = 0;

function NotPromise(executor) {
    return new Promise(executor);
}

Object.defineProperty(NotPromise, 'resolve', {
    get() {
        resolveCount++;
        return function(v) {
            return Promise.resolve(v);
        };
    }
});

Promise.all.call(NotPromise, [promise, promise, promise]);

console.log(resolveCount);

Here are some tests you can use for Promise.all and Promise.allSettled. Promise.race would need a few tweaks.

I support this change.

---

You can also monkeypatch Promise.resolve directly but make sure it does not conflict with other stuff while running the tests:

var promise = new Promise(function() {});
var resolveCount = 0;
var origResolve = Promise.resolve;

Object.defineProperty(Promise, 'resolve', {
    get() {
        resolveCount++;
        return origResolve;
    }
});

Promise.all([promise, promise, promise])

console.log(resolveCount);

[mathiasbynens]

I’m with the “let’s just do it [in V8]” crowd. Agreed that the change seems obscure enough that I wouldn’t expect any significant breakage.

I plan on merging tc39/proposal-promise-allSettled#40.

@gsathya do you want to kick off a PR against ecma262 as well?

[gsathya]

@gsathya do you want to kick off a PR against ecma262 as well?

Done: #1506

[gsathya]

@leobalter Can you please land your tests in test262? I'd like to have good test262 coverage before shipping this in chrome. Thanks!

[mathiasbynens]

tc39/proposal-promise-allSettled#40 is now merged. Let's land the Test262 tests and get this optimization shipped in Chrome soon, so we can gather data. Thanks, everyone!

[leobalter]

We have tests in tc39/test262#2131.

I'd like to discuss if we should change this behavior to only get the 'resolve' method if the iterable is not empty. Do we have precedent for this?

[ljharb]

It feels like there'd be more precedent for minimize calling user code before checking all invariants; iow, if resolve isn't callable, why bother invoking next at all? (to check if the iterable is empty)

[bakkot]

@leobalter:

I'd like to discuss if we should change this behavior to only get the 'resolve' method if the iterable is not empty. Do we have precedent for this?

I think the closest thing is the behavior of things like the Map or Set constructors, which are precedent in the other direction: they look up their adder function before consuming the iterator at all, even to check if it's empty (i.e., pretty much what #1506 does). I wouldn't think that precedent needs to be binding, if there's a reason to do things differently, but on the other hand I think the currently proposed behavior is more predictable (and it sounds like e.g. @gibson042 agrees).

[leobalter]

sounds reasonable. I just wanted to make sure the tests represent the proposed intent. Sounds like it already does.