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

FieldResolver is too slow #487

Closed
sixmen opened this issue Dec 6, 2019 · 9 comments · Fixed by #488
Closed

FieldResolver is too slow #487

sixmen opened this issue Dec 6, 2019 · 9 comments · Fixed by #488
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request Solved ✔️ The issue has been solved
Milestone

Comments

@sixmen
Copy link
Contributor

sixmen commented Dec 6, 2019

Is your feature request related to a problem? Please describe.
This is an advanced topic of #254

If you use FieldResolver, execution needs more time.

import 'reflect-metadata';
import { buildSchemaSync, Field, FieldResolver, ObjectType, Query, Resolver, Root } from 'type-graphql';
import { graphql } from 'graphql';

@ObjectType({ simpleResolvers: true })
class Item {
  @Field()
  a: number;
  @Field()
  b: number;
  @Field()
  c: number;
  @Field()
  d: number;
  @Field()
  e: number;
}

@Resolver(Item)
class ItemResolver {
  @Query(() => [Item])
  items() {
    return new Array(5000).fill(0).map((v, i) => ({ a: i, b: i, c: i, d: i, e: i }))
  }

  @FieldResolver()
  a(@Root() source) {
    return source.a;
  }

  @FieldResolver()
  b(@Root() source) {
    return source.b;
  }

  @FieldResolver()
  c(@Root() source) {
    return source.c;
  }

  @FieldResolver()
  d(@Root() source) {
    return source.d;
  }

  @FieldResolver()
  e(@Root() source) {
    return source.e;
  }
}

const schema = buildSchemaSync({
  resolvers: [ItemResolver]
});

const start = Date.now();
graphql(schema, '{ items { a b c d e } }')
  .then((r) => {
    console.log(r.data!.items.length);
    console.log(r.data!.items[30].c);
    console.log('time =', Date.now() - start);
  });

Describe the solution you'd like
I didn't think whether some option likes simple is a solution or not.

But in the end, some optimization technique applied in graphql-js may be needed, I think.

For example,

    if (isPromise(result)) {
      return result.then(undefined, error => {
        exeContext.errors.push(error);
        return Promise.resolve(null);
      });
    }
    return result;

(from https://github.com/graphql/graphql-js/blob/master/src/execution/execute.js)

In graphql-js, those two declarations become difference result.

const Item = new GraphQLObjectType({
  name: 'Item',
  fields: {
    a: { type: GraphQLString, resolve: (s) => s.a },
  }
});

const Item = new GraphQLObjectType({
  name: 'Item',
  fields: {
    a: { type: GraphQLString, resolve: async (s) => s.a },
  }
});
@MichalLytek MichalLytek added Need More Info 🤷‍♂️ Further information is requested Question ❔ Not future request, proposal or bug issue labels Dec 7, 2019
@MichalLytek
Copy link
Owner

The problem here is not about promises - it's a really small overhead that's hard to measure.

The real cause is that field resolvers are a normal resolvers - they have injected transformed args and other params, you can have custom decorators, etc. So they are not generated as root[field] but a normal class method call, so we also need resolve the dependencies, get an instance from container, etc.

@FieldResolver are designed to work with "rich resolvers" that e.g. resolve relation between tables in database. If you need something like this:

  @FieldResolver()
  a(@Root() source) {
    return source.a;
  }

It is better to use a getter that is called just like the simple resolver:

  @Field({ name: "a" })
  get getA() {
    return this.a;
  }

@sixmen
Copy link
Contributor Author

sixmen commented Dec 10, 2019

It is a good idea to use getter instead of FieldResolver.
But I reminded the original issue of mine, and it was an async resolver. So it it not helpful.

Anyway, getParams for '@root' may be the cause of the performance issue.
So, I think it is still useful to optimize Promises.

I tried to optimize by myself, and make a PR #488. Please check it.
In my test, the performance is improved dramatically. (maybe no need for simple option?)

@MichalLytek
Copy link
Owner

MichalLytek commented Dec 10, 2019

From code readability and performance perspective, I don't like to mix the sync and async stuff just for some percent of use cases that use field resolver for simple sync access.

I think that all we can think of now is to add a { raw: true } support for full resolvers like @Query or @FieldResolver that will skip all the TypeGraphQL things and executes using a standard (root, args, ctx, info) signature.
This will allow to "fallback" into almost the plain graphql-js execution when someone needs that.

@MichalLytek MichalLytek added Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request and removed Need More Info 🤷‍♂️ Further information is requested labels Jan 2, 2020
@MichalLytek MichalLytek added this to the 1.0.0 release milestone Jan 2, 2020
@benawad
Copy link
Contributor

benawad commented Jan 3, 2020

I would love to do { raw: true } at a global level

@MichalLytek
Copy link
Owner

{ raw: true } at a global level

@benawad
So what do you need TypeGraphQL for? Maybe #339 (https://github.com/BramKaashoek/typescript-typedefs) fits you better?

@benawad
Copy link
Contributor

benawad commented Jan 3, 2020

@MichalLytek I still like writing the resolvers in the TypeGraphQL style, I'd just like to have a raw option if I don't need to use middleware/validation/etc

@MichalLytek
Copy link
Owner

For now we will just merge #488 - further optimization will be applied on vNext that will aim to have as minimal overhead as possible:
https://github.com/MichalLytek/type-graphql/tree/vNext
https://github.com/MichalLytek/type-graphql/projects/2

@benawad
Copy link
Contributor

benawad commented Jan 3, 2020

@MichalLytek Awesome, I'm looking forward to that!

Are you open to { simpleResolvers: true } at a global level?

@MichalLytek
Copy link
Owner

MichalLytek commented Jan 3, 2020

Are you open to { simpleResolvers: true } at a global level?

With #488 if you don't use middlewares nor auth checker, there's no performance penalty - see the PR and the benchmarks results.

@MichalLytek MichalLytek added Solved ✔️ The issue has been solved and removed Question ❔ Not future request, proposal or bug issue labels Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request Solved ✔️ The issue has been solved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants