enhancement: remove extra ticks by yaacovCR · Pull Request #3754 · graphql/graphql-js

yaacovCR · 2022-09-30T08:38:51Z

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 · 2022-09-30T08:38:56Z

✅ 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 · 2022-10-05T19:09:29Z

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

yaacovCR · 2022-10-06T14:06:32Z

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 · 2022-10-06T14:31:57Z

I added 89521b3 to "optimize" completeValueCatchingErrors.

yaacovCR · 2022-10-06T19:08:37Z

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 · 2022-10-21T17:28:33Z

@github-actions run-benchmark

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

...integrating review feedback

github-actions · 2022-12-27T10:19:22Z

@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 · 2022-12-27T11:35:01Z

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.

see graphql#3754 (comment)

yaacovCR · 2024-04-01T13:32:49Z

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 · 2024-04-01T13:37:32Z

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

yaacovCR · 2024-04-03T18:35:52Z

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.

see graphql#3754 (comment)

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 Depends on graphql#3793

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 Depends on #3793

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 Depends on graphql#3793

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 Depends on #3793

yaacovCR force-pushed the more branch 6 times, most recently from 7575d65 to e1074d8 Compare October 4, 2022 13:28

yaacovCR commented Oct 4, 2022

View reviewed changes

Comment thread src/execution/execute.ts

Comment thread src/execution/execute.ts Outdated

yaacovCR changed the title ~~fix(incrementalDelivery): remove unnecessary thens~~ fix(incrementalDelivery): remove extra ticks Oct 4, 2022

yaacovCR force-pushed the more branch 2 times, most recently from 70d4963 to 1fb60f2 Compare October 5, 2022 19:04

yaacovCR requested review from a team and robrichard October 5, 2022 19:06

yaacovCR changed the title ~~fix(incrementalDelivery): remove extra ticks~~ enhancement: remove extra ticks Oct 5, 2022

yaacovCR added the PR: feature 🚀 requires increase of "minor" version number label Oct 5, 2022

yaacovCR mentioned this pull request Oct 5, 2022

polish: a few smaller refactors #3756

Closed

robrichard approved these changes Oct 6, 2022

View reviewed changes

yaacovCR force-pushed the more branch 2 times, most recently from 1fb60f2 to 68b229e Compare October 12, 2022 09:30

yaacovCR mentioned this pull request Oct 18, 2022

fix(incrementalDelivery): fix null bubbling with async iterables #3760

Merged

yaacovCR force-pushed the more branch from 68b229e to 01b0f70 Compare October 18, 2022 09:49

This comment has been minimized.

Sign in to view

yaacovCR force-pushed the more branch from 01b0f70 to 8650f39 Compare December 5, 2022 20:52

yaacovCR mentioned this pull request Dec 6, 2022

polish: do not repeat isPromise check #3793

Merged

yaacovCR force-pushed the more branch from 8650f39 to 145acee Compare December 6, 2022 13:29

rename completePromise to completePromisedValue

b19a684

...integrating review feedback

yaacovCR force-pushed the more branch from 265e687 to b19a684 Compare December 14, 2022 20:59

yaacovCR merged commit 1564174 into graphql:main Dec 14, 2022

yaacovCR deleted the more branch December 14, 2022 21:05

This comment has been minimized.

Sign in to view

yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Dec 27, 2022

add additional benchmark

4c25a0e

see graphql#3754 (comment)

yaacovCR mentioned this pull request Dec 27, 2022

add benchmark for async list with nested async field #3805

Closed

yaacovCR mentioned this pull request Jan 5, 2023

introduce new Publisher for incremental delivery #3786

Closed

yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Oct 1, 2024

add additional benchmark

af60b5f

see graphql#3754 (comment)

jasonkuhrt mentioned this pull request Oct 29, 2024

refactor: treeshakable kind enum #4270

Merged

Conversation

yaacovCR commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

UPDATE:

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

Uh oh!

netlify Bot commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

yaacovCR commented Oct 5, 2022

Uh oh!

yaacovCR commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaacovCR commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaacovCR commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

github-actions Bot commented Oct 21, 2022

Uh oh!

This comment has been minimized.

github-actions Bot commented Dec 27, 2022

Uh oh!

yaacovCR commented Dec 27, 2022

Uh oh!

yaacovCR commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaacovCR commented Apr 1, 2024

Uh oh!

yaacovCR commented Apr 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yaacovCR commented Sep 30, 2022 •

edited

Loading

netlify Bot commented Sep 30, 2022 •

edited

Loading

yaacovCR commented Oct 6, 2022 •

edited

Loading

yaacovCR commented Oct 6, 2022 •

edited

Loading

yaacovCR commented Oct 6, 2022 •

edited

Loading

yaacovCR commented Apr 1, 2024 •

edited

Loading