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

[Performance & Security] Dramatic Performance Regression from 0.11.7 to 14.5.8 #2428

Closed
CreatCodeBuild opened this issue Feb 6, 2020 · 3 comments
Labels

Comments

@CreatCodeBuild
Copy link

CreatCodeBuild commented Feb 6, 2020

Reporting issues with GraphQL.js

I was using 0.11.7 and recently updated to 14.5.8. The performance of GraphQL layer decreased by about 60%.

Here is an example

const { graphql, buildSchema } = require('graphql');

const s = buildSchema(`

type Query {
    getX: [X]
    getY: [Y]
}

type X {
    name: String
    y: [Y]
}

type Y {
    name: String
    age: Int
    y: [Y]
}

`);
const stat = [];
for (let i = 0; i < 100 / 200 / 300 / 400; i++) {
    const t = new Date();
    const p = graphql(
        s,
        `{
        getX {
            name
            y {
                name, age
            }
        }
        getY {
            name
            age
            y { 
                name, age 
                y {
                    y {
                        name, age
                        y {
                            y {
                                y {
                                    name, y {
                                        y {
                                            name, age
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
      }`,
        {
            getX: () => {
                return [
                    X(),
                    X(),
                    X(),
                    X(),
                    X(),
                ]
            },
            getY: () => {
                return [Y(), Y(), Y()]
            }
        },                 // root value
    )
    const t2 = new Date();
    stat.push(t2 - t);
}

console.log( stat.reduce((sum, cur) => { return sum + cur}) / stat.length );

function X() {
    return {
        name: "X",
        y: function () {
            return [Y(), Y(), Y()]
        }
    }
}

function Y() {
    return {
        name: "Y",
        age: 200,
        y: function () {
            return [Y(), Y(), Y()];
        }
    }
}

Notice that I am not measuring the time when the promise is resolved. I am only measuring the time spent in the synchronous code that creates the promise.

I ran it N * 100 times in a loop. Here is the average in ms.

100 200 300 400
0.11.7 32.6 39.54 63.58 JavaScript heap out of memory
14.5.8 58.9 58.475 58.403 58.19

2 surprising find out:

  1. 0.11.7 has a memory leak
  2. 14.5.8's performance is more consistent but it is about 60% slower when running 100 times in a loop.

Of course, in a real scenario, the amount of traffic would be as fast as a for loop so that I never get this heap dump error in production.

My question are:

  1. Does anybody have any advice on how to improve the synchronous code performance? Should I upgrade to a newer version?
  2. Could the performance be related to the memory leak? For example, 0.11.7 leaks memory so that GC doesn't kick in so often such that it performs better in low traffic.
@CreatCodeBuild CreatCodeBuild changed the title Dramatic Performance Regression from 0.11.7 to 14.5.8 [Performance & Security] Dramatic Performance Regression from 0.11.7 to 14.5.8 Feb 6, 2020
@IvanGoncharov
Copy link
Member

@CreatCodeBuild Thanks for reporting 👍
To be honest, this issue took me by surprise so I did some investigation.
It is caused by #1137 and fixed by #1174.
Just add NODE_ENV=production before running your server and you should do it anyway since a lot of other node.js libraries disable additional checks in production mode.

@CreatCodeBuild
Copy link
Author

Can we make this an argument of graph function because we have other NODE ENV that needs it as well.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Feb 7, 2020

@CreatCodeBuild It will require HUGE code changes almost every function will have an additional argument so I can't do that.

production is a pretty standard value it is used by express, React and others:
https://expressjs.com/en/advanced/best-practice-performance.html#set-node_env-to-production
https://create-react-app.dev/docs/adding-custom-environment-variables/

As the last resort, you can try to fork this lib and change this file:
https://github.com/graphql/graphql-js/blob/master/src/jsutils/instanceOf.js

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

No branches or pull requests

2 participants