Should PromiseResolve(C, x) return x if x is a promise?

[gsathya]

Promise.resolve(x) returns x if x a promise (and same subclass), but PromiseResolve does not have this check. This means, we always create a new promise irrespective of whether x is a promise or not. This behavior is observable because of the microtask queue ordering.

var resolves = [];
var promises = [];
var value = 0;
var start = performance.now();
var count = 100000;

for (let i = 0; i < count; i++) {
  let resolve;
  let p = new Promise(r => { resolve = r });
  promises.push(p);
  resolves.push(resolve);

  Promise.resolve()
    .finally(() => {
      return promises[i];
    })
    .then(() => {
      value++;
    });

  Promise.resolve()
    .then(() => { resolves[i](); })
}

%RunMicrotasks();


var end = performance.now();
print(end - start);
print(value == count);

Running the above microbenchmark in V8 --
Current spec implementation: 570.32 ms
Modified implementation which does not create an extra promise: 443.42 ms

[gsathya]

cc @domenic @littledan

[ljharb]

Based on these numbers, that looks like it's 0.00127ms slower for a single operation - that doesn't seem particularly large, but I'm not used to evaluating benchmarks like this.

Separately, I grant that it's possible to construct a scenario such that the ordering is observable - but is is likely to be observable in the majority case? If not, is that something that could be detected and optimized away?

Having Promises be fast is very valuable, but the "fast path" in Promise.resolve imo is a very unfortunate decision - it would be a useful thing if Promise.resolve(x) !== x was true for every value; as it is, it's very odd to me that const x = Promise.resolve(2); x.foo = {}; Promise.resolve(x).foo === x.foo. I had hoped not to propagate that decision to "finally".

[littledan]

I like this proposal. It seems consistent with the current API, not harmful for programmers, and reduces the implementation burden to get as good performance.

As @gsathya explains, it's rather difficult to optimize these things away completely because of microtask queue ordering. Promise performance remains a source of complaints/fear for developers, even after it has improved in V8.

@ljharb Can you explain more about why you think the decision with Promise.resolve is an unfortunate decision? Even if that decision is unfortunate, it's hard for me to see how developers would encounter and be harmed by the optimization in this more specific case.

[ljharb]

It is indeed consistent, and I don't think it's particularly harmful. I'm not strongly opposed to making this change.

I think it's unfortunate because a) it's codifying a semantic change in the spec solely for optimization reasons; b) it means Promise.resolve(x) !== x won't be true for every value (kind of like how you didn't want Number(someBigInt) to throw for some values and not for others, you preferred a consistent answer to that question)

[littledan]

Yes, both of those "unfortunate" things are true. That is exactly what the design is. Do you have an idea of when users might want to depend on b) ? In particular, I think throwing/not throwing is a bit more drastic and consequential than equal/not equal.

[ljharb]

I agree - in the case of throwing/not throwing, if i make a mistake, i get an exception loudly notifying me; in the case of returning the wrong value, my code silently does the wrong thing.

As to when users might depend on b), I know that i would do it (if i didn't know about this Promise.resolve spec fast path) if i wanted to attach extra properties to a promise, or to use it as a key in a Map, and i wanted Promise.resolve(x) to work like "clone" when given a Promise. I think since that's been in the spec for years, there's not going to be any real examples of people relying on it; that doesn't mean it's not a surprising way for it to work.

[littledan]

OK, I can (abstractly) see that cloning use case.

Here, Promise.prototype.finally will output a new Promise, not the original, right? We're just talking about intermediate objects that get created.

[ljharb]

That's a very fair point; my distaste for what Promise.resolve does definitely doesn't mean that adding it in the PromiseResolve abstract operation necessarily has to leak out anywhere else in the future.

I'll make this change now. Thanks @gsathya and @littledan for talking it through!