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

Improve consistency of field shapes #4360

Merged
merged 9 commits into from
Mar 3, 2023
Merged

Improve consistency of field shapes #4360

merged 9 commits into from
Mar 3, 2023

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Feb 24, 2023

Initialize GraphQL::Schema::Field so that its instance variables are always initialized in the same order. This way Ruby's "Object Shapes" optimization will work for field instances during query execution. (However, this also requires an application's custom field state to use consistent ivar setup, too.)

Part of #4299

This ended up improving the Schema footprint memory benchmark, though I couldn't tell you why:

  $ bundle exec rake bench:profile_schema_memory_footprint

- Total allocated: 4060200 bytes (34279 objects)
+ Total allocated: 4058600 bytes (34279 objects)
- Total retained:  1832888 bytes (6185 objects)
+ Total retained:  1831288 bytes (6185 objects)


  allocated memory by location
  -----------------------------------
  ... 
-     199200  /Users/rmosolgo/code/graphql-ruby/lib/graphql/schema/resolver/has_payload_type.rb:92
+     197600  /Users/rmosolgo/code/graphql-ruby/lib/graphql/schema/resolver/has_payload_type.rb:92

@rmosolgo rmosolgo added this to the 2.0.18 milestone Feb 24, 2023
@@ -355,7 +357,7 @@ def initialize(type: nil, name: nil, owner: nil, null: nil, description: :not_gi
# @return [Boolean, nil]
# @see GraphQL::Subscriptions::BroadcastAnalyzer
def broadcastable?
if defined?(@broadcastable)
if @broadcastable != NOT_CONFIGURED
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not worth it, but when using this kind of "unset" tokens, I prefer to inverse the check so that if I hit a weird user defined object with a custom == it won't be invoked.

You can also use equal? to only check identity:

Suggested change
if @broadcastable != NOT_CONFIGURED
if !NOT_CONFIGURED.equal?(@broadcastable)

But I agreed that it doesn't read as well.

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍 Thanks for the suggestion, I'll update it!

@@ -5,6 +5,8 @@
module GraphQL
class Schema
class Field
NOT_CONFIGURED = ::Object.new
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you'd use this pattern in multiple classes, it might make sense to have a single GraphQL::NOT_CONFIGURED. You can also make it a private_constant if you don't want users to be able to pass it as argument.

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍 I added it here and I've opened #4363 to track rolling it out across the codebase.

…icter checks that won't call user-defined == implementations
@@ -66,6 +66,9 @@ def self.scan(graphql_string)
def self.scan_with_ragel(graphql_string)
GraphQL::Language::Lexer.tokenize(graphql_string)
end

NOT_CONFIGURED = Object.new
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is kind of ROFLscale territory, but I wonder if all these extra references might slow down major GC pause.

One way to avoid this would be to use an immediate object, like a static symbol, e.g. :__GraphQL__NOT_DEFINED__. This allows the GC to deretctly skip that reference.

It's not as hack proof, as it's much easier for users to pass that symbol, but I can't imagine it happening by accident.

That said, I'm no GC expert, it might not make a difference at all.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm going to leave it as-is 😅

@rmosolgo rmosolgo merged commit 77eb8fe into master Mar 3, 2023
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

2 participants