Execute: simplify + speedup by IvanGoncharov · Pull Request #1286 · graphql/graphql-js

IvanGoncharov · 2018-03-13T16:38:18Z

For a feature, I'm working on I needed to understand how execute is working so I read through its implementation. I notice that completeValueWithLocatedError is called only from completeValueCatchingError and would be simpler to just merge them together.
So main reason of this PR is to remove one layout of try/catch & isPromise/then and make error handling more obvious.

To check that I didn't introduce any performance regressions I profiled modified version of introspectionFromSchema-benchmark.js
During this profiling I discovered that hottest function is a fat arrow callback inside executeFields function:

Statistical profiling result from base3-v8.log, (9005 ticks, 439 unaccounted, 0 excluded).

 [Shared libraries]:
   ticks  total  nonlib   name
      9    0.1%          /usr/lib/system/libsystem_pthread.dylib
      2    0.0%          /usr/lib/system/libsystem_malloc.dylib

 [JavaScript]:
   ticks  total  nonlib   name
    892    9.9%    9.9%  LoadIC: A load IC from the snapshot
    850    9.4%    9.5%  LazyCompile: *<anonymous> /Users/ivang/dev/graphql-js/perf_test/baseDist/execution/execute.js:361:58
    436    4.8%    4.8%  LazyCompile: *completeValue /Users/ivang/dev/graphql-js/perf_test/baseDist/execution/execute.js:613:23
    345    3.8%    3.8%  Stub: CallApiGetterStub
    259    2.9%    2.9%  LazyCompile: *getArgumentValues /Users/ivang/dev/graphql-js/perf_test/baseDist/execution/values.js:101:27
    248    2.8%    2.8%  LazyCompile: *completeValueWithLocatedError /Users/ivang/dev/graphql-js/perf_test/baseDist/execution/execute.js:578:39
    231    2.6%    2.6%  Builtin: KeyedLoadIC_Megamorphic
    210    2.3%    2.3%  Builtin: ArrayReduce
    202    2.2%    2.2%  Stub: GetPropertyStub
    130    1.4%    1.4%  LazyCompile: *defaultFieldResolver /Users/ivang/dev/graphql-js/perf_test/baseDist/execution/execute.js:844:88

So I replaced Array.reduce with for cycle on Object.keys and together with removal of completeValueWithLocatedError it resulted in ~15% speedup(9005 ticks vs 7612 ticks), see profile:

Statistical profiling result from changed_beautiful-v8.log, (7612 ticks, 402 unaccounted, 0 excluded).

 [Shared libraries]:
   ticks  total  nonlib   name
      9    0.1%          /usr/lib/system/libsystem_pthread.dylib
      1    0.0%          /usr/lib/system/libsystem_platform.dylib
      1    0.0%          /usr/lib/system/libsystem_malloc.dylib

 [JavaScript]:
   ticks  total  nonlib   name
    823   10.8%   10.8%  LoadIC: A load IC from the snapshot
    551    7.2%    7.2%  LazyCompile: *completeObjectValue /Users/ivang/dev/graphql-js/perf_test/changedDist/execution/execute.js:732:29
    477    6.3%    6.3%  LazyCompile: *completeValue /Users/ivang/dev/graphql-js/perf_test/changedDist/execution/execute.js:611:23
    335    4.4%    4.4%  Stub: CallApiGetterStub
    279    3.7%    3.7%  LazyCompile: *getArgumentValues /Users/ivang/dev/graphql-js/perf_test/changedDist/execution/values.js:101:27
    247    3.2%    3.2%  Builtin: KeyedLoadIC_Megamorphic
    145    1.9%    1.9%  Stub: GetPropertyStub
    121    1.6%    1.6%  LazyCompile: *defaultFieldResolver /Users/ivang/dev/graphql-js/perf_test/changedDist/execution/execute.js:835:88
    100    1.3%    1.3%  LazyCompile: *getFieldDef /Users/ivang/dev/graphql-js/perf_test/changedDist/execution/execute.js:855:21
     91    1.2%    1.2%  Builtin: KeyedStoreIC_Megamorphic_Strict
     88    1.2%    1.2%  Builtin: FastNewClosure
     77    1.0%    1.0%  Builtin: ArrayReduce
     75    1.0%    1.0%  Builtin: WeakMapLookupHashIndex

Performance improvement achieved on a synthetic test and probably wouldn't account for any measurable speedup in most servers. Another small bonus is call stack reduction 1 call (2 calls if a value returned as a promise) for every field or item inside an array.

But most importantly this PR make it easier for a developer to trace how values are resolved/complete inside execute.

leebyron · 2018-03-29T19:51:43Z

  path: ResponsePath,
  result: mixed,
 ): MaybePromise<mixed> {
-  // If result is a Promise, apply-lift over completeValue.


Do we have a test that shows completing Array<Promise> works?

@leebyron Yes, it checked here:

graphql-js/src/execution/__tests__/lists-test.js

Lines 167 to 173 in 43ff3e3

describe('Array<Promise<T>>', () => {

it(

'Contains values',

check(type, [resolved(1), resolved(2)], {

data: { nest: { test: [1, 2] } },

}),

);

leebyron · 2018-03-29T19:53:03Z

-    return null;
+  } catch (rawError) {
+    const error = locatedFieldError(rawError, fieldNodes, path);
+    return handleFieldError(error, returnType, exeContext);


locatedFieldError and handleFieldError are always called together - should they become a single function?

@leebyron Yes, you're right. I merged them together 👍

leebyron · 2018-03-29T19:53:48Z

-      result,
-    );
+    let completed;
+    if (isPromise(result)) {


I'm concerned moving this out of completeValue may break some scenarios we might not have sufficient test coverage over.

@leebyron completeValue only called in 3 places:

graphql-js/src/execution/execute.js

Lines 907 to 912 in 43ff3e3

// If result is a Promise, apply-lift over completeValue.

if (isPromise(result)) {

return result.then(resolved =>

completeValue(exeContext, returnType, fieldNodes, info, path, resolved),

);

}

I moved this code to completeValueCatchingError so nothing changed here.

graphql-js/src/execution/execute.js

Lines 919 to 929 in 43ff3e3

// If field type is NonNull, complete for inner type, and throw field error

// if result is null.

if (isNonNullType(returnType)) {

const completed = completeValue(

exeContext,

returnType.ofType,

fieldNodes,

info,

path,

result,

);

This code is called after promise uplift so nothing changed here.

graphql-js/src/execution/execute.js

Line 849 in 43ff3e3

const completed = completeValue(

Also nothing is broken here since this call moved to completeValueCatchingError and completeValueCatchingError now handles promise uplift by itself.

IvanGoncharov · 2018-04-05T21:19:23Z

@leebyron I address review comments. Can you please take a second look?

leebyron · 2018-04-18T20:45:54Z

Really nice work simplifying this!

* Simplify non-null tests * Remove 'check' function and write test directly * containSubset => deep.equal * Change order of errors because of #1286 * remove excessive async/await

facebook-github-bot added the CLA Signed label Mar 13, 2018

This was referenced Mar 13, 2018

Slow response times with large documents #723

Open

Fix execute deoptimisation caused by GraphQLEnumType #1288

Merged

leebyron reviewed Mar 29, 2018

View reviewed changes

IvanGoncharov added 7 commits April 6, 2018 00:14

Remove 'completeValueWithLocatedError'

2daa853

Execute: move promise resolution to the completeValueCatchingError

de51665

Remove fat arrow function

7496e73

Execute: rewrite executeFields to use for-in instead of reduce

b364898

Extract locatedFieldError function

c52afaa

extract handleFieldError function

3802af7

Address @leebyron review comments

c4fe200

IvanGoncharov force-pushed the executeSpeedup branch from 535813c to c4fe200 Compare April 5, 2018 21:18

leebyron merged commit 69d90c6 into graphql:master Apr 18, 2018

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Apr 18, 2018

Change order of errors because of graphql#1286

f75ff2f

IvanGoncharov mentioned this pull request Apr 18, 2018

Make "nonnull-test.js" more explicit #1311

Merged

IvanGoncharov deleted the executeSpeedup branch June 6, 2018 15:50

snyk-bot mentioned this pull request Apr 10, 2021

[Snyk] Upgrade chai from 4.2.0 to 4.3.4 turkdevops/graphql-js#133

Closed

This was referenced Jun 20, 2021

[Snyk] Upgrade cspell from 5.2.4 to 5.5.1 turkdevops/graphql-js#220

Closed

[Snyk] Upgrade cspell from 5.2.4 to 5.5.2 turkdevops/graphql-js#222

Closed

snyk-bot mentioned this pull request Jun 27, 2021

[Snyk] Upgrade cspell from 5.2.4 to 5.6.0 turkdevops/graphql-js#228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Execute: simplify + speedup#1286

Execute: simplify + speedup#1286
leebyron merged 7 commits intographql:masterfrom
IvanGoncharov:executeSpeedup

IvanGoncharov commented Mar 13, 2018 •

edited

Loading

Uh oh!

leebyron Mar 29, 2018

Uh oh!

IvanGoncharov Apr 5, 2018

Uh oh!

leebyron Mar 29, 2018

Uh oh!

IvanGoncharov Apr 5, 2018

Uh oh!

leebyron Mar 29, 2018

Uh oh!

IvanGoncharov Apr 5, 2018 •

edited

Loading

Uh oh!

IvanGoncharov commented Apr 5, 2018

Uh oh!

leebyron commented Apr 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	describe('Array<Promise<T>>', () => {
	it(
	'Contains values',
	check(type, [resolved(1), resolved(2)], {
	data: { nest: { test: [1, 2] } },
	}),
	);

	// If result is a Promise, apply-lift over completeValue.
	if (isPromise(result)) {
	return result.then(resolved =>
	completeValue(exeContext, returnType, fieldNodes, info, path, resolved),
	);
	}

	// If field type is NonNull, complete for inner type, and throw field error
	// if result is null.
	if (isNonNullType(returnType)) {
	const completed = completeValue(
	exeContext,
	returnType.ofType,
	fieldNodes,
	info,
	path,
	result,
	);

Conversation

IvanGoncharov commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leebyron Mar 29, 2018

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

leebyron Mar 29, 2018

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

leebyron Mar 29, 2018

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov commented Apr 5, 2018

Uh oh!

leebyron commented Apr 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

IvanGoncharov commented Mar 13, 2018 •

edited

Loading

IvanGoncharov Apr 5, 2018 •

edited

Loading