Skip to content

PoC: Cache complexity#5638

Open
sobrinho wants to merge 1 commit into
rmosolgo:masterfrom
sobrinho:master
Open

PoC: Cache complexity#5638
sobrinho wants to merge 1 commit into
rmosolgo:masterfrom
sobrinho:master

Conversation

@sobrinho
Copy link
Copy Markdown
Contributor

In the first round (#5631) we dropped from ~806ms to ~176ms, but that's still too much processing time. Caching the complexity calculation can bring that 176ms down to ~1ms on a cache hit. This PR is a PoC for that approach.

The slowest piece is calling max_possible_complexity, not the visitor itself. Further improvements could be made with that in mind, but I didn't go down that path since caching largely solves the problem without requiring a refactor of the complexity calculation. We also have field_complexity, but it can't be used here because it's called during max_possible_complexity — and what we actually want is to avoid calling max_possible_complexity altogether.

The digesting principle

The approach is simple:

  1. Capture the fields and their complexity values (if the complexity changes, the digest changes).
  2. Skip caching entirely if any field uses dynamic complexity (a lambda and/or complexity_for).
  3. When validating the complexity, compute the digest and look it up.

Open questions

  1. Multiplexes: def result doesn't have access to queries, so I haven't figured out how to make this analyzer work with multiplexed queries. (We don't multiplex very often on our end, so this hasn't been a blocker for the PoC.)
  2. Error handling: visitor.arguments_for might raise. What should we do in that case — skip the cache attempt and fall through to the normal calculation?
  3. Does this belong in core? I think it should, at least the digest calculation, since it's tightly coupled to the gem's internals.
  4. Query#complexity_fingerprint: We initially discussed in Cache complexity analyzer in-memory/redis #5632 the idea of adding a Query#complexity_fingerprint method, but that would require firing a dedicated analyzer, which feels out of place compared to how the other fingerprints are designed:
   def complexity_fingerprint
     @complexity_fingerprint ||= GraphQL::Analysis.analyze_query(self, [TheCacheDigestComplexityAnalyzer])[0]
   end

The upside would be reusability for other purposes (though honestly, I can't think of any concrete ones right now). analyze_query should be fast, but this still feels a bit redundant given that we already have the complexity analyzer.

Feedback

How do you feel about this PoC? There are no specs yet — I'm looking for initial feedback before moving forward and handling all the edge cases.

@sobrinho
Copy link
Copy Markdown
Contributor Author

A few caveats: the code isn't pretty, relying on the caller to sort is awkward, and even delegating the SHA256 hashing might be questionable. All of this could potentially happen inside the gem itself, exposing only the final fingerprint.

But again, I want to validate the idea/principle of the problem and solution before moving forward with these technical decisions.


if max_possible_page_size.nil?
raise GraphQL::Error, "Can't calculate complexity for #{field_definition.path}, no `first:`, `last:`, `default_page_size`, `max_page_size` or `default_max_page_size`"
end
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire algorithm was copied over from GraphQL::Schema::Field#calculate_complexity. It is very likely that we want to extract this max_possible_page_size to a method instead of duplicating it.

# The page size affects the complexity.
if field_definition.connection?
arguments = visitor.arguments_for(node, field_definition)
return unless arguments.is_a?(GraphQL::Execution::Interpreter::Arguments) # FIXME: Are errors by definition not cacheable?
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible that what we are looking for here is to flag as not cacheable since we can't access the actual arguments:

unless arguments.is_a?(GraphQL::Execution::Interpreter::Arguments)
  @uncacheable << visitor.query
  @complexity_digest.delete(visitor.query)

  return
end

raise GraphQL::Error, "Can't calculate complexity for #{field_definition.path}, no `first:`, `last:`, `default_page_size`, `max_page_size` or `default_max_page_size`"
end

complexity_digest << max_possible_page_size
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a field (assuming complexity 1):

["Person.name", 1]

For a connection (assuming complexity 1 and first/last as 10):

["People", 1, 10]

@sobrinho
Copy link
Copy Markdown
Contributor Author

We're also trying to cache in a way where the query string itself isn't relevant — what matters are the actual fields being selected. Some queries have the exact same complexity even though their query strings differ.

For example:

query MyQuery($search: String, $limit: Int!) {
  people(search: $search, first: $limit) {
    id
    firstName
    lastName
  }
}

has the exact same complexity as the following, even though the query string is completely different:

query AnotherQuery($s: String!, $x: Int!) {
  myAlias: people(first: $x, search: $s) {
    lastName
    id
    firstName
  }
}

In the end, we're selecting the same fields with the same arguments. Furthermore, the first argument impacts complexity while search has no impact at all (assuming complexity isn't dynamic).

So whether I send { s: "Sobrinho", x: 10 } or { x: 10, s: "Gabriel" }, we know the complexity is the same. And we also know the complexity changes if x changes, even if s stays the same: { s: "Sobrinho", x: 10 } has a different complexity from { s: "Sobrinho", x: 100 }.

The PoC takes this into account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant