enhancement: remove extra ticks

[yaacovCR]

UPDATE:

After much testing related to #3796, I am unable to generalize the speed savings from this change. Although it is true that there is a single extra PromiseJob created when returning a promise from an onFulfilled handler with .then(...) (as opposed to using an async function) -- this extra tick in the general case does not seem to incur a real-world significant performance cost. In particular, when directly using our benchmark suite on the async version without an extra tick versus the .then(...) version with an extra tick, there is no performance benefit.

So why did this PR demonstrate a significant performance benefit? Probably because in the case where item completion requires completing a promise, we used to call .then(...) twice, first to call completeValue, and then to install an error handler. It seems that the "extra" tick might not matter, but double-calling .then(...) does. In fact, we only get the true savings when the completeValue call within completePromisedValue does not return a promise. We used to have a second .then(...) anyway for the error handler, but now we can return directly with a single await. If completion does return a promise, i.e. if there is a nested field that itself returns a promise, the entire performance benefit disappears.

So the performance benefit with this PR does not have to do with the proposal below for faster promise adoption.

Promise resolution order changes in some instances, resulting in different orders for some errors within the errors array, as well as in different values of hasNext within incremental delivery payloads.

This PR introduces an async completePromisedValue helper function rather than using a promise chain (see below links).

https://github.com/tc39/proposal-faster-promise-adoption
tc39/ecma262#2770
tc39/ecma262#2772
tc39/ecma262#1250
https://v8.dev/blog/fast-async

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	b19a684
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/639a3949da530a0008adf841
😎 Deploy Preview	https://deploy-preview-3754--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[yaacovCR]

This PR has been pruned to only include the changes the result in tick decreases -- and is now ready for review!

[yaacovCR]

In terms of comparing the number of ticks, see the following, adapted from tc39/ecma262#2770

const result = Promise.resolve(1);

function dummyAsync(n) {
  return Promise.resolve(n+1);
}

async function asyncHelperWith2Awaits(p) {
  try {
    const r = await p;
    return await dummyAsync(r);
  } catch (err) {
    return 0;
  }
}

function returnAsyncWith2Awaits() {
  return asyncHelperWith2Awaits(result);
}

function returnPromiseWithThen() {
  return result.then(r => dummyAsync(r));
}

function returnPromiseWithThenAndCatch() {
  return result.then(r => dummyAsync(r)).then(undefined, () => 0);
}

const resolved = Promise.resolve();

async function test(fn) {
  let done = false;
  let count = 0;
  fn().then(() => { done = true; });

  function counter() {
    if (done) {
      console.log(`${fn.name} took ${count} ticks`);
    } else {
      resolved.then(() => {
        count++;
        counter();
      });
    }
  }
  counter();
}

async function tests() {
  await resolved;
  await test(returnAsyncWith2Awaits);
  await test(returnPromiseWithThen);
  await test(returnPromiseWithThenAndCatch);
}

tests();

which emits on my instance of chrome:

returnAsyncWith2Awaits took 3 ticks
returnPromiseWithThen took 4 ticks
returnPromiseWithThenAndCatch took 5 ticks

Additional async helpers may be of use elsewhere; I have focused here so far on value completion.

[yaacovCR]

I added 89521b3 to "optimize" completeValueCatchingErrors.

[yaacovCR]

The "optimization" does not seem to be necessary for completeValueCatchingErrors:

const result = Promise.resolve(1);

async function direct() {
  try {
    return await result;
  } catch (err) {
    return 0;
  }
}

async function asyncHelper(p) {
  try {
    return await p;
  } catch (err) {
    return 0;
  }
}

function returnAsync() {
  return asyncHelper(result);
}

function returnPromiseWithCatch() {
  return result.then(undefined, () => 0);
}

const resolved = Promise.resolve();

async function test(fn) {
  let done = false;
  let count = 0;
  fn().then(() => { done = true; });

  function counter() {
    if (done) {
      console.log(`${fn.name} took ${count} ticks`);
    } else {
      resolved.then(() => {
        count++;
        counter();
      });
    }
  }
  counter();
}

async function tests() {
  await resolved;
  await test(direct);
  await test(returnAsync);
  await test(returnPromiseWithCatch);
}
tests();

yields:

direct took 2 ticks
returnAsync took 2 ticks
returnPromiseWithCatch took 2 ticks

89521b3 removed!

[github-actions]

@github-actions run-benchmark

@yaacovCR Please, see benchmark results here: https://github.com/graphql/graphql-js/actions/runs/3297051171/jobs/5437417123#step:6:1

[yaacovCR]

I removed the sync helper as it doesn't really help anything.

[yaacovCR]

If I'm running this right, tests are a bit noisy (running on WSL on my laptop), but looks like a speed bump of 58%....

Which is good! Note that this comes entire from this PR rather than #3793 which is HEAD~1

[yaacovCR]

#3796 formalizes the approach in this PR across the code base using eslint rules to prefer async functions over then/catch. It removes another extra tick.

[IvanGoncharov]

left, a name suggestion.
As far as every else, it is ready to merge.

Integrated feedback.

[github-actions]

@github-actions publish-pr-on-npm

@yaacovCR The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.2.canary.pr.3754.1564174b0dc26e0adf7ff2833716d06606b06a20
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-3754

[yaacovCR]

See edited comment above, the performance benefit with this PR does not have to in the end with faster promise adoption; it seems to have to do with subtracting a .then(...) call. We can probably close #3796 as the general case is not helpful.

[yaacovCR]

This PR no longer results in a performance savings on main. With the help of git bisect, it appears that this change in behavior occurred with #3886.

Reverting this PR from the commit right before #3886 yields the following performance drop:

But with #3886, reverting this PR, actually leads to a performance improvement!

Why? .....

[yaacovCR]

clearly this does not have to do with ticks and presumably has something to do with JIT changes ???

[yaacovCR]

I have played around with deopt-explorer trying to explore what optimization changes completePromisedValue might introduce and in what context.

There are definite changes in what is optimized/deoptimized with this functions. In terms of profiling, however, I could not see how this was tied to performance, because using deopt-explorer, it was never faster with this optimization, for reasons unknown.

These benchmarks are very synthetic => we use the same operation repeatedly, have a single field, and a single rootValue. My expectation is that v8's ability to optimize would be challenged with a mixture of complex operations, and root values. All of this is internal behavior, and reworking graphql-js in ways that make the code more complex to trick v8 into producing a set of more optimized functions for a synthetic test case that may not influence real world performance does not seem like the best idea.

So I am much more bearish on this PR from the get-go. My inclination even at this point is to revert it prior to the v17 release, especially as it no longer seems to produce the desired synthetic performance benefit.