Memoize selection gathering

[gmac]

Been wondering this for a while, but is there a reason why selection gathering isn’t memoized per subtree so that the work isn’t repeated across a list, like what GraphQL JS does? https://github.com/graphql/graphql-js/blob/d26810bd7f1aab0c67736ad958f0c892cbe7343e/src/execution/execute.ts#L66

This is something we’ve seen in our traces for years that lists of objects burn time gathering the same selections over and over. The Cardinal shim cut this step out of its runtime sequencing (using the selections Cardinal builds once up front at the start of each static subtree branch), and I suspect is a big reason why the shim benchmarks slightly faster than native gem execution.

[rmosolgo]

IIRC the challenge in this code is from how directives are implemented:

• Directives can change the subtree getting executed (as in @skip), and the current implementation of this is in gather_selections:

  graphql-ruby/lib/graphql/execution/interpreter/runtime.rb

  Lines 214 to 217 in fbe350f

  	# Skip gathering this if the directive says so
  	if !directives_include?(node, owner_object, owner_type)
  	next
  	end

• And, those directives receive owner_object (the thing that the application returned for the previous field) as an input (also seen in the code above).

So as it stands, you need some amount of per-object processing (since the object is an input to those directive checks). I tried applying some optimizations a while back, in cases where I thought they could be applied: #4623. But it ended up in causing infinite loops (😖 ) and was reverted (#4684).

I'm definitely interested in and open to improvements here -- agreed that this would be a big win. Breaking changes to how Directive.include? are on the table as far as I am concerned -- to my knowledge it's not used in the wild.

[rmosolgo]

Oh, I found another failed attempt: #4990

[hwrdprkns]

From what I can tell, the earlier caching attempts ran into trouble because #4623 cached inside gather_selections keyed on AST nodes - which caused issues when fragments got spread at multiple locations since they share the same fragment_def.selections object.

One thought — what if caching happened at each_gathered_selections instead keyed on [owner_type, response_hash.graphql_selections.object_id]?

Somewhere around here: https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/execution/interpreter/runtime.rb#L199-L210

[rmosolgo]

I'm definitely open to a fresh approach to caching that result, but I think that implementation would still need to somehow address the fact that owner_object is currently an input to gather_selections.

If one of you wants to take a crack at implementing a new approach, I'm willing to do the compatibility work to ship it!

[rmosolgo]

This will be accomplished by #5507