Optimize resolvers execution paths

[sixmen]

Fixes #487

[codecov]

Codecov Report

Merging #488 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   95.14%   95.18%   +0.04%     
==========================================
  Files          75       76       +1     
  Lines        1318     1329      +11     
  Branches      252      256       +4     
==========================================
+ Hits         1254     1265      +11     
  Misses         61       61              
  Partials        3        3

Impacted Files	Coverage Δ
src/resolvers/helpers.ts	100% <100%> (ø)	⬆️
src/utils/isPromiseLike.ts	100% <100%> (ø)
src/resolvers/create.ts	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba96c58...1cbfccf. Read the comment docs.

[MichalLytek]

@sixmen
Please add proper entries to the results in the benchmarks folder - without measuring it makes no sense to add any "optimization".

[MichalLytek]

Lesson 1: always build your code before running 😄

Sync resolver:  12118.231ms
Async/await:    28740.372ms

So this makes sense to bloat the code with the fast sync path 🤔

[sixmen]

I also don't like unnecessary optimization that ruins the code.

(It may be unreal cases, but I faced the case as #254 shown.
I cannot be sure for FieldResolver.
I faced a problem with async FieldResolver, and I avoided it with no FieldResolver with cached value)

So I tried to minimize the change.

I just made this PR to show you the possibility of optimization.
{simpleResolvers:true} was too difficult to find resolvers that need to apply.

I added the benchmark commit, and following is the result from my computer (MacBook Pro i7 2.6GHz)
(tg means type-graphql, raw means graphql-js)

Benchmark	Node 10.16.0 master	Node 13.3.0 master	Node 10.16.0 PR	Node 13.3.0 PR
simple-single-raw	472.977ms	408.466ms	464.077ms	416.154ms
simple-nested-raw	1305.241ms	1.270s	1322.084ms	1.233s
simple-single-tg	1271.616ms	745.608ms	504.383ms	428.95ms
simple-nested-tg	4614.922ms	2.222s	1419.520ms	1.262s
large-basic-raw	2535.619ms	2.661s	2550.349ms	2.610s
large-sync-raw	2466.865ms	2.616s	2518.828ms	2.625s
large-async-raw	6623.758ms	6.315s	6569.861ms	6.251s
large-basic-tg	20637.726ms	15.165s	3192.436ms	3.143s
large-sync-tg	39585.782ms	24.437s	4292.903ms	4.097s
large-async-tg	40319.222ms	23.873s	9920.493ms	9.594s

[MichalLytek]

I wonder why field resolvers are still 50% slower than the raw graphql-js - with simpleResolvers it's only 15% 🤔

@sixmen please mark allowing changes to a pull request branch created from a fork as I want to push benchmark results 😉

[sixmen]

@MichalLytek do you mean "Allow edits from maintainers."? I enabled it.

I will also look for more bottlenecks, if I had time.

[sixmen]

I found another possibility.

In my project, convertToType causes a problem.

An ObjectType class of my project has a read-only field (using Object.defineProperty).
If this object is passed to @Root, convertToType will try to Object.assign(new Target(), data) and fail.
I want to skip create an object, but I want type checking working.
So I used type for the class by type MyClassVO = Pick<MyClass, keyof MyClass>.

If I changes @Root() source: SampleObject to @Root() source: any, it runs faster.

[MichalLytek]

do you mean "Allow edits from maintainers."? I enabled it.

@sixmen

remote: Permission to croquiscom/type-graphql.git denied to MichalLytek.
fatal: unable to access 'https://github.com/croquiscom/type-graphql/': The requested URL returned error: 403
Pushing to https://github.com/croquiscom/type-graphql

[sixmen]

I enabled it. I don't know what is the reason.

Instead, I invited you to the forked repository.

[MichalLytek]

Fun story - "then" in value is messing up something in Promises so when I call it in isPromiseLike, the tests are not passing 😕 Reverted to the type assertion

[sixmen]

Fun story - "then" in value is messing up something in Promises so when I call it in isPromiseLike, the tests are not passing 😕 Reverted to the type assertion

Two are different:

> a = 2
2
> a.then
undefined
> 'then' in a
Thrown:
TypeError: Cannot use 'in' operator to search for 'then' in 2

More accurate version may be https://github.com/then/is-promise/blob/master/index.js, I copied it from https://github.com/graphql/graphql-js/blob/master/src/jsutils/isPromise.js.

[MichalLytek]

Right, both me and TypeScript forgot that the 'in' operator can be used only on objects, not primitives.
But I will stick with the simpler version as it's measurably faster 😄

[MichalLytek]

@sixmen
I am really astonished how things speeds up with this changes.
Now the simplest cases takes 15.518s instead of 57.776s, which makes simpleResolvers not needed then. It speeds up even when global middleware are used (80.517s vs 62.664s)!

I've also added a benchmark case when simpleResolvers are usefull, as if you have a global middleware or auth checker, it reduces from 62.664s to 14.980s 😉

The last thing I have to do in this PR is to update the performance docs with this insights.

[MichalLytek]

LGTM!

[sixmen]

When you will release this commit? I need to decide whether waiting next release or using beta release.

[MichalLytek]

@sixmen
I am grouping breaking changes before releasing major 0.18 version.
You can safely install type-graphql@beta - it's not an experimental branch, it's a master branch that is a release candidate 😉
I am collecting feedback about this changes as it sometimes breaks others code: #510