Resolve fields asyncronously

[ssttevee]

Modified executeFields to resolve each field in their own go routine.

I suspect that this may be the intended behaviour because it is just a copy of executeFieldsSerially right now.

[ssttevee]

Having a hard time trying to figure out why tests are failing. It seems to work fine in my own project.

[narqo]

Is resolveField goroutine-safe? From the first sign, it mutates the passed context.

[ssttevee]

This is my bad for assuming p.ExecutionContext was just a regular context.Context.

[narqo]

A half year ego I wanted to make similar changes, but then decided that spawning a goroutine on every resolve might be much more expensive than doing it in series. So now I wonder, have you checked the impact on the execution time and resource allocation? Does it really make sense?

[ssttevee]

This undoubtedly uses more resources due to the fact that is trying to do more at the same time. However, performance is immediately increased from parallelism when GOMAXPROCS > 1.

It especially makes sense when calling many long running procedures where the majority of the work is just waiting (i.e. RPCs).

[ssttevee]

Also, not sure if you know this, but goroutines are incredibly cheap and it's not uncommon to have thousands to goroutines at a time.

see https://talks.golang.org/2012/concurrency.slide#17

[narqo]

Although goroutines are cheap, they aren't free. To my knowledge, a goroutine allocates about 4KB of memory for its stack. So, I believe, in a project with 100RPS, 1000th of goroutines (spawned for every request) might lead to unexpected over-consumption of memory. And worth to mention, that such load profile would impact on a way how the data-source behaves (e.g. db or some external http service(-s)).

Concurrency doesn't just make things faster. A silly benchmark that fills a slice of 1000 items one by one (serially vs concurrently) gives almost two orders of magnitude difference in execution time. So in the case of the resolving, everything might depend on how many such "long running procedures" the graph has, comparing to total number of resolvers.

Don't get me wrong, I'm not judging the changes or PR. As I said, some time ago I decided that doing resolving in a concurrent way (at leat by simply spawn a goroutine per resolve) might bring some extra issues in a high-loaded project, but I was too lazy to actually do benchmarks. And now I'm interested if I was wrong.

[ssttevee]

I see what you're getting at now, but I feel like it's reasonable to assume that someone that handles 100s of requests per second would allocate the necessary resources to do so. Also that someone with such a large schema would have their own optimizations for hitting external resources.

On the other hand, perhaps just spawning a goroutine per resolve is a little lazy. Another solution could be to only spawn a goroutine when a func() (interface{}, error) is returned by the resolve function. Similar to a how it works in graphqljs with promises.

[coveralls]

Coverage increased (+0.03%) to 82.161% when pulling 15f000b on ssttevee:patch-1 into 3e619b6 on graphql-go:master.

[ssttevee]

This commit should address the thread safety of ExecutionContext.

[bbuck]

I'd recommend, even though in this case it's benefits are null, a defer eCtx.errorsMutex.Unlock() immediately after the lock. The benefit here is potential reduction in future errors if this gets modified. This is just a style comment though.

[coveralls]

Coverage increased (+0.06%) to 82.126% when pulling 4fad7c8 on ssttevee:patch-1 into 105a6c2 on graphql-go:master.

[coveralls]

Coverage increased (+0.09%) to 82.15% when pulling 1435aeb on ssttevee:patch-1 into 105a6c2 on graphql-go:master.

[coveralls]

Coverage increased (+0.06%) to 82.12% when pulling fda4b32 on ssttevee:patch-1 into 105a6c2 on graphql-go:master.

[coveralls]

Coverage increased (+0.09%) to 82.15% when pulling fda4b32 on ssttevee:patch-1 into 105a6c2 on graphql-go:master.

[ssttevee]

I've modified to to be an opt-in concurrency, only running concurrently if a func() (interface{}, error) is returned as the result in a resolve function.

[Kliton]

Any update?

[coveralls]

Coverage increased (+0.08%) to 82.141% when pulling 46a59cc on ssttevee:patch-1 into 105a6c2 on graphql-go:master.

[deoxxa]

This is working very well for me.

I fixed a couple of bugs, shoved github.com/nicksrandall/dataloader in my data access layer, and saw a threefold increase in requests/sec. I've got a moderately complex schema with lots of relations, and a fairly naiive strategy for fetching them (usually one query per relation), so this worked great.

I've had a look at the other concurrent loading PR (#132) and it's significantly more complex than this one. Granted, this implementation may not be as comprehensive, but it's much easier to understand its method of operation.

[coveralls]

Coverage increased (+0.06%) to 82.282% when pulling d6366ac on ssttevee:patch-1 into 9b5a661 on graphql-go:master.

[dvic]

Does anybody know what the status is of this PR?

[deoxxa]

@dvic works fine for me - been in production for a month now with no problems. Not sure what the plan is regarding merging though.

[dvic]

@deoxxa Works fine indeed with dataloader 😃

@chris-ramon Do you know if this will be merged in the near future? Thanks 👍

[dvic]

I took the changes from this branch and added the concurrent resolving of list values #132 in https://github.com/dvic/graphql/tree/resolve-asynchronously, that was an important missing part for me. This is a perfect match with https://github.com/nicksrandall/dataloader. There were some data races but I managed to fix these as well, all tests are passing with the -race flag included. If anybody is interested in this I can open a PR.

[dvic]

Some benchmarks comparing this branch (+ parallel list evaluation):

# master
BenchmarkListQuery_1-8        	   10000	    155662 ns/op	   51342 B/op	    1067 allocs/op
BenchmarkListQuery_100-8      	    3000	    518540 ns/op	  222106 B/op	    3606 allocs/op
BenchmarkListQuery_1K-8       	     500	   3666769 ns/op	 1780063 B/op	   28291 allocs/op
BenchmarkListQuery_10K-8      	      50	  34972951 ns/op	17892675 B/op	  278041 allocs/op
BenchmarkListQuery_100K-8     	       5	 231047041 ns/op	117428734 B/op	 1819260 allocs/op
BenchmarkWideQuery_1_1-8      	   10000	    105671 ns/op	   36253 B/op	     684 allocs/op
BenchmarkWideQuery_10_1-8     	    5000	    269208 ns/op	   84774 B/op	    1845 allocs/op
BenchmarkWideQuery_100_1-8    	    1000	   1792880 ns/op	  573930 B/op	   13161 allocs/op
BenchmarkWideQuery_1K_1-8     	     100	  17094916 ns/op	 5628088 B/op	  125968 allocs/op
BenchmarkWideQuery_1_10-8     	   10000	    121407 ns/op	   46185 B/op	     805 allocs/op
BenchmarkWideQuery_10_10-8    	    5000	    357362 ns/op	  123137 B/op	    2434 allocs/op
BenchmarkWideQuery_100_10-8   	     500	   2622739 ns/op	  967160 B/op	   18141 allocs/op
BenchmarkWideQuery_1K_10-8    	      50	  26363954 ns/op	10789724 B/op	  173798 allocs/op

# https://github.com/dvic/graphql/tree/resolve-asynchronously
BenchmarkListQuery_1-8        	   10000	    158149 ns/op	   52866 B/op	    1093 allocs/op
BenchmarkListQuery_100-8      	    3000	    490188 ns/op	  303528 B/op	    5113 allocs/op
BenchmarkListQuery_1K-8       	     500	   2969223 ns/op	 2595591 B/op	   43372 allocs/op
BenchmarkListQuery_10K-8      	      50	  31944810 ns/op	26000820 B/op	  433798 allocs/op
BenchmarkListQuery_100K-8     	      10	 236689768 ns/op	170768831 B/op	 2843417 allocs/op
BenchmarkWideQuery_1_1-8      	   10000	    108122 ns/op	   37236 B/op	     701 allocs/op
BenchmarkWideQuery_10_1-8     	    5000	    266334 ns/op	   87330 B/op	    1889 allocs/op
BenchmarkWideQuery_100_1-8    	    1000	   1796816 ns/op	  592376 B/op	   13475 allocs/op
BenchmarkWideQuery_1K_1-8     	     100	  17283137 ns/op	 5803984 B/op	  128978 allocs/op
BenchmarkWideQuery_1_10-8     	   10000	    155514 ns/op	   49555 B/op	     872 allocs/op
BenchmarkWideQuery_10_10-8    	    5000	    359734 ns/op	  142299 B/op	    2771 allocs/op
BenchmarkWideQuery_100_10-8   	    1000	   2204073 ns/op	 1146777 B/op	   21179 allocs/op
BenchmarkWideQuery_1K_10-8    	     100	  22325519 ns/op	12554450 B/op	  203839 allocs/op

[marpaia]

This PR looks really great, is there any reason in particular why it hasn't been merged?

[gburt]

@dvic can you help me understand when/why resolving list items in goroutines is useful? If they're scalars/enums they'll resolve very quickly, and if they're objects they'll go down the same completeObjectValue -> executeFields path (which go-routines off if the resolved value is a func), no?

[dvic]

@gburt For me it's basically for libraries such as https://github.com/nicksrandall/dataloader, where one can do some smart batching/caching of queries.

[gburt]

@dvic oh I totally get that, I'm just struggling to see how goroutining within completeListValue helps things -- since the result of the list will already have been fetched async if requested, and once you have the result, doing the val := resultVal.Index(i).Interface() for each item in the list shouldn't need to be done async, should it?

[scniro]

This PR looks really great, is there any reason in particular why it hasn't been merged?

Has there been any forward progress on this? Seems like there's a real want here.

[ssttevee]

Unfortunately, it doesn't seem like the owner of this repo has any interest in accepting this PR.

I've left this PR open for when they change their mind, as well as for this solution to be more easily found by someone who may be looking for one.

I'll also try to keep this patch fresh as updates come.

[edpark11]

I pulled down @dvic 's branch with this change (https://github.com/dvic/graphql/tree/resolve-asynchronously) and implemented a resolver function with Nick Randall's dataloader. It seems to work very well. My use case is to reduce the n+1 query problem on belongsto relations, i.e. replace n Get(id) calls with a single List(idList) call. To do that, in the resolver, I just return a func() (interface{}, error). To be specific, the key line in the resolve function is returnVal := (func() (interface{}, error))(dataloaders["MyEntity"].Load(p.Context, key)). (I had to type the Thunk to what this PR specifically expects, but it otherwise works seamlessly.)

Anyways, I mostly wanted to say thanks to the folks on this pull thread and ask the question of whether this can be merged into the core. I would be open to the other async solutions as well, but the code in this one is very easy to understand and it's very easy to integrate with Nick Randall's dataloader. Or if this one can't be merged, I'd love to understand the criteria for merging one of the other ones. Thanks!!

[chris-ramon]

Thanks a lot guys! 👍 — Closing this one in favor of #388, you might want to take a look to a real-use case working example that shows the support for concurrent resolvers.