Execute: simplify + speedup

[IvanGoncharov]

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]

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

[IvanGoncharov]

@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]

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

[IvanGoncharov]

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

[leebyron]

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

[IvanGoncharov]

@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]

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

[leebyron]

Really nice work simplifying this!