Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Big performance drop due to switching TS output from ES5 to ES6 #3648

Closed
IvanGoncharov opened this issue Jun 15, 2022 · 9 comments
Closed

Big performance drop due to switching TS output from ES5 to ES6 #3648

IvanGoncharov opened this issue Jun 15, 2022 · 9 comments

Comments

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 15, 2022

I investigated the performance drop between v15 and v16 and it was caused by switching transpilation target from es5 to es6.
It's easy to reproduce with the following diff:

diff --git a/tsconfig.json b/tsconfig.json
index edb522f1..299b9309 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -7,7 +7,7 @@
   ],
   "compilerOptions": {
     "lib": ["es2020"],
-    "target": "es2020",
+    "target": "es5",
     "module": "commonjs",
     "moduleResolution": "node",
     "strict": true,

Note: some of the functionality stops working due to an error in this file (we can't extend Map in es5):
https://github.com/graphql/graphql-js/blob/main/src/jsutils/AccumulatorMap.ts

But even benchmarks that are working showing pretty big performance difference:
local is ES5 and HEAD is ES2020

image

I don't think we need to switch back to ES5 since it has its own drawbacks but instead, we just need to figure out what is slowing us and implement that code changes manually in our code base.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Aug 5, 2022

As a reference there are a handful of features that are ES6 that are a lot faster when transpiled into ES5, might be worth scanning the code base for some of these and maybe set up some transformations for the published files or adjust the code.

An example here would be for..of what is your preference on how this is tackled, a selective transpilation or hand-coded?

@marvinhagemeister
Copy link

marvinhagemeister commented Aug 5, 2022

Like @JoviDeCroock said my hunch is that ES2015+ features aren't as well optimized as ES5 ones. for..of is a good candidate as that always has to invoke the iterator protocol because the array prototype could've been messed with somewhere.

Another area where the iterator protocol leads to overhead is destructuring:

const arr = [1,2,3];

// invokes iterator protocol
const [a, b] = arr;

// usually faster than destructuring
const a = arr[0];
const b = arr[1];

FYI: The table linked was last run in 2020 and might not represent current engine optimizations well.

@JoviDeCroock
Copy link
Member

Quickly did a few manual passes to confirm the assumptions and we seem to be on the right track here

The build schema from AST benchmark going from 96.61 ops/sec to 160ops/sec

@StefanFeederle
Copy link

I saw a noticable performance drop going from graphql-js v15 to v16.8.0. About ~40% depending on the query.
@IvanGoncharov is your fix from IvanGoncharov@d1dea90 not yet included in v16.8.0? Any chance you can release this? Thank you :)

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Aug 26, 2023

I don't think it's published, where did you see the perf drop? Parsing, execute,...?

@StefanFeederle
Copy link

How do i benchmark parsing vs execute?

My query is quite simple (3 objects, about 25 fields), but has a lot of results (~700kb json).
I would guess it's probably the field execution because parsing should be quick in comparison.

query parts {
  parts: recentParts {
    ...partFragment
    __typename
  }
}

fragment partFragment on part {
  id
  pos
  width
  length
  height
  quantity
  quantityNested
  quantityClosed
  partName
  comment
  weight
  costTargetNumber
  drawing
  state
  identifier
  nestingIds
  createdAt
  updatedAt
  closedAt
  image
  firstDateIfUnfinished
  ORDERITEM {
    ORI_ID
    mainDrawing {
      clientPath
      __typename
    }
    mainStep {
      clientPath
      __typename
    }
    __typename
  }
  __typename
}

@diogotorres97
Copy link

@IvanGoncharov @JoviDeCroock @StefanFeederle is this fixed? We are considering upgrading from v15 to v16 😄

@JoviDeCroock
Copy link
Member

This never made it in, I'm open to reviving my PR but would love to know what the steps are to getting it in then before committing more time to this.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Oct 13, 2024

This is currently fixed on main - when comparing ES5 to the current main output the performance is equal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants