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!