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

Too slow for simple large list #254

Open
sixmen opened this issue Feb 14, 2019 · 4 comments

Comments

Projects
2 participants
@sixmen
Copy link

commented Feb 14, 2019

Is your feature request related to a problem? Please describe.
type-graphql is slower than normal graphql for simple large list.

type-graphql: time = 920

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

@ObjectType()
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 }))
  }
}

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);
  });

normal graphql: time = 101

import {
  graphql,
  GraphQLSchema,
  GraphQLObjectType,
  GraphQLString,
  GraphQLList
} from 'graphql';

const Item = new GraphQLObjectType({
  name: 'Item',
  fields: {
    a: { type: GraphQLString },
    b: { type: GraphQLString },
    c: { type: GraphQLString },
    d: { type: GraphQLString },
    e: { type: GraphQLString },
  }
});

var schema = new GraphQLSchema({
  query: new GraphQLObjectType({
    name: 'RootQueryType',
    fields: {
      items: {
        type: GraphQLList(Item),
        resolve() {
          return new Array(5000).fill(0).map((v, i) => ({ a: i, b: i, c: i, d: i, e: i }))
        }
      }
    }
  })
});

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 think some optimizations are needed for simple cases.

If I change createSimpleFieldResolver, it runs faster.

function createSimpleFieldResolver(fieldMetadata) {
    return (root, args, context, info) => root[fieldMetadata.name];
}
@19majkel94

This comment has been minimized.

Copy link
Owner

commented Feb 14, 2019

Note: partially related to #71.

@sixmen
It's a well known fact that abstraction makes things slower.
But your comparison only show super-simple hello world case when you just read properties of generated objects.

If you are interested in making TypeGraphQL faster, you should consider creating more real-life benchmarks, where you call http endpoint, read from databse, write from database, have inputs validation, some auhtorization guard mechanism, middlewares for catching and transforming results, etc. In this more complicated scenario, the difference will be much smaller cause the most of time it will execute arbitrary user code, not the framework internals.

I can make @Field({ simple: true }) decorator option to skip all the middlewares and other framework internals and generates simple property access resolvers if that fixes your real app problem. But I would rather invite you to create some benchmark suites, measure time spend in different TypeGraphQL internals an maybe we found some easy optimizations like skipping transforming object instances or validation of empty args objects, etc.

@sixmen

This comment has been minimized.

Copy link
Author

commented Feb 15, 2019

Actually this is a REAL problem for me.

My program reads data of length 500 from database and returns as list. And no middleware are applied to the fields.

If the DB time is 200ms, type-graphql is still very slow (300ms vs 100ms).

This is a graph when I remove type-graphql runtime (decorators remain, and build schema from decorators at compile time)

2019-02-139 36 04

@sixmen

This comment has been minimized.

Copy link
Author

commented Feb 15, 2019

For my problem, following shortcuts are enough because I don't have global middlewares.

export function createSimpleFieldResolver(
  fieldMetadata: FieldMetadata,
): GraphQLFieldResolver<any, any, any> {
  const { authChecker, authMode, globalMiddlewares } = BuildContext;
  const middlewares = globalMiddlewares.concat(fieldMetadata.middlewares!);
  applyAuthChecker(middlewares, authMode, authChecker, fieldMetadata.roles);

  return async (root, args, context, info) => {
    const resolverData: ResolverData<any> = { root, args, context, info };
    if (middlewares.length === 0) {
      return root[fieldMetadata.name];
    }
    return await applyMiddlewares(resolverData, middlewares, () => root[fieldMetadata.name]);
  };
}

OR

export function createSimpleFieldResolver(
  fieldMetadata: FieldMetadata,
): GraphQLFieldResolver<any, any, any> {
  const { authChecker, authMode, globalMiddlewares } = BuildContext;
  const middlewares = globalMiddlewares.concat(fieldMetadata.middlewares!);
  applyAuthChecker(middlewares, authMode, authChecker, fieldMetadata.roles);

  if (middlewares.length === 0) {
    return (root, args, context, info) => {
      return root[fieldMetadata.name];
    };
  } else {
    return async (root, args, context, info) => {
      const resolverData: ResolverData<any> = { root, args, context, info };
      return await applyMiddlewares(resolverData, middlewares, () => root[fieldMetadata.name]);
    };
  }
}

If someone has global middlewares, some methods to skip middlewares(like {simple:true}) may be needed.

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Mar 3, 2019

When #183 arrives, I can definitely detect if the resolver has some middlewares or other things attached and then can decide if use an advanced resolver or just the simple one (or even no resolver to let the graphql-js handle it by itself without transforming the parent object and other overheads.

I will take care of that problem after some benchmarks #71.

@19majkel94 19majkel94 added this to Backlog in Board via automation Mar 3, 2019

@19majkel94 19majkel94 added this to the 1.0.0 release milestone Mar 3, 2019

@19majkel94 19majkel94 moved this from Backlog to To Do in Board Mar 3, 2019

@19majkel94 19majkel94 moved this from To Do to Backlog in Board Mar 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.