Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid Hash#merge on large subclass schemas #4664

Merged
merged 2 commits into from
Oct 16, 2023
Merged

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Oct 13, 2023

It was already mentioned in the code how a large schema can incur a costly Hash#merge when using .references_to:

# `@own_references_to` can be quite large for big schemas,
# and generally speaking, we won't inherit any values.
# So optimize the most common case -- don't create a duplicate Hash.

But the implementation there didn't address when a schema did have some inherited types. In that case, the cost was still paid of merging the hashes, even for a very small query.

We can re-work this, improving an existing implementation of .references_to(type_name), so that the big, merged hash isn't used by GraphQL-Ruby execution, so it's never made at all.

For a small schema with inherited types, it can make a big difference. The benchmark I added here:

-      Run small query      4.586k (± 8.1%) i/s -     23.256k in   5.108886s
+     Run small query      4.879k (± 5.8%) i/s -     24.735k in   5.090457s
... 
- Total allocated: 30656 bytes (278 objects)
+ Total allocated: 27248 bytes (277 objects)

That was one object taking up 10% of memory!

Another thing I noticed was that, because the @references_to Hash was assigning [] to keys by default, GraphQL::Schema could actually have some entries in its Hash. But they might not point at anything -- they might just be misses. So it would cause .merge for a bunch of empty arrays 😩

TODO:

  • check large query benchmarks -- did this slow them down? Perhaps cache Warden#referenced?
    • The other benchmarks look the same. And they didn't have any reduction in memory, presumably because they didn't have any inherited references_to so they weren't performing the merge to start with.

@rmosolgo rmosolgo added this to the 2.1.4 milestone Oct 13, 2023
@rmosolgo rmosolgo merged commit d5968d9 into master Oct 16, 2023
12 checks passed
@rmosolgo rmosolgo deleted the avoid-hash-merge branch October 16, 2023 14:37
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.

None yet

1 participant