Fix memory leak in buildSchema/extendSchema

[IvanGoncharov]

This code will quickly use all memory:

const {
  parse,
  buildSchema,
  extendSchema,
} = require('./dist')

let schema = buildSchema(`
  type Query
`);

for (let i = 0; i < 100000000; ++i) {
  console.log(i);
  schema = extendSchema(schema, parse(`
    extend type Query {
      foo${i}: String
    }
  `));
}

If limit node memory to 100mb it will crash on around 1000 cycle.

[IvanGoncharov]

The memory leak happens because _typeConfig contains field and interfaces closures that depend on some internal arrays + in case of extendSchema it also depends on the previous schema.

[mjmahone]

nice debugging!

[IvanGoncharov]

Required because eslint and prettier disagree about this rule.

[mjmahone]

This is an impressive find, thank you for digging into and resolving it!

[mjmahone]

nice debugging!

[mjmahone]

Why Object.create(null) instead of {}?

[IvanGoncharov]

@mjmahone I just move code from _defineFieldMap here no changes were made.
But here is the gist on why graphql-js always uses Object.create(null):

const a = {};
Boolean(a.toString)
// true
const b = Object.create(null);
Boolean(b.toString)
// false

[mjmahone]

That makes sense! Thanks for the info, seems good to merge.

[nodkz]

@IvanGoncharov maybe i'm something missing and it does not make unnecessary checks at runtime.

BUT the following code may be a point for degradation:

  getFields(): GraphQLFieldMap<*, *> {
-    return (
-      this._fields ||
-      (this._fields = defineFieldMap(this, this._typeConfig.fields))
-    );
+    if (typeof this._fields === 'function') {
+      this._fields = this._fields();
+    }
+    return this._fields;
  }

Before for second and all further calls works the following code:

return this._fields;

But now became with one more additional expensive typeof check:

if (typeof this._fields) // false
return this._fields;

Is this method Type.getFields() used for every query execution or not?

[IvanGoncharov]

Before for second and all further calls works the following code:

Actually, it was something like:

return this._fields === undefined ? this._fields : //...

We just used && shortcut.

But both Node and modern browsers optimize this code anyway so the difference is 2-3% in this case:
https://jsperf.com/undefined-vs-typeof2

On the other hand, assigning new properties to the objects is a real performance killer since it deoptimizes not only getFields but all other functions it was inlined in.
For the detailed analysis see: #1288

We can do this._fields = undefined inside constructor but I don't think we should
focus on micro-optimization on this stage of the project.

Anyway, it's hard to predict the performance of the modern JS engines so I think we should focus on writing realistic benchmarks and measure performance difference.

You can run the benchmark using npm run benchmark -- --revs local 4124b61 to see performance impact of this commit or npm run benchmark -- --revs local v0.13.2 to see all performance difference between 0.13.2 vs 14.0.0.

Note: in the current state benchmark has 3-5% precision but it's enough for measuring realistic performance.

P.S. Would be great if someone would contribute more realistic benchmarks for executing sync and async queries.

[nodkz]

@IvanGoncharov thanks for such detailed explanation! I have no questions any more. 👍

PS. I somewhere missed that to graphql-js was added benchmarks. Thanks for point me on them.