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

Add cached, named Visibility profiles #5074

Merged
merged 21 commits into from
Oct 10, 2024
Merged

Add cached, named Visibility profiles #5074

merged 21 commits into from
Oct 10, 2024

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Aug 26, 2024

This aims to add to improvements to GraphQL-Ruby's (forthcoming) schema visibility system.

  • Ahead-of-time caching of named profiles, so that they can be reused verbatim at runtime (no calls to .visible? during execution)
  • Along with dynamic: true, which is how the current visibility system always works, calling .visible? for every schema member on every query.

Part of #5014

TODO

  • Preload named profiles when preload: true (defaults to true when Rails.env.production?)
  • Accept configured visible_in: ... or similar, for named visibility profiles
    • This is still possible, but I don't think it's pressing
  • Rename Visibility::Subset => Visibility::Profile
  • Document Visibility::Profile

@rmosolgo
Copy link
Owner Author

rmosolgo commented Oct 4, 2024

To benchmark this, I modified profile_large_result:

    schema = Class.new(ProfileLargeResult::Schema) do
      # def self.visible?(member, context)
      #   sleep 0.001
      #   super
      # end
    end

    schema2 = Class.new(schema) {
      use GraphQL::Schema::Visibility
    }
    document = ProfileLargeResult::ALL_FIELDS
    Benchmark.ips do |x|
      x.config(time: 10)
      x.report("[WARDEN] Querying for #{ProfileLargeResult::DATA.size} objects") {
        schema.execute(document: document)
      }
      x.report("[PROFILE] Querying for #{ProfileLargeResult::DATA.size} objects") {
        schema2.execute(document: document)
      }

The performance was about the same between the two (Warden a bit faster? within the margin of error.), except when a cached visibility profile was used and sleep 0.001 was present -- then Visibility::Profile was much faster.

Visibility::Profile uses a hair less memory (less than 1% difference).

@rmosolgo rmosolgo modified the milestone: 2.3.18 Oct 4, 2024
@rmosolgo rmosolgo merged commit 3ddf36b into master Oct 10, 2024
15 checks passed
@rmosolgo rmosolgo deleted the named-visibility branch October 10, 2024 19:34
- `Visibility` supports predefined, reusable visibility profiles which speeds up queries using complicated `visible?` checks.
- `Visibility` hides types differently in a few edge cases:
- Previously, `Warden` hide interface and union types which had no possible types. `Visibility` doesn't check possible types (in order to support performance improvements), so those types must return `false` for `visible?` in the same cases where all possible types were hidden. Otherwise, that interface or union type will be visible but have no possible types.
- Some other thing, see TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @rmosolgo – could you provide any more detail here? This memory would really help finding the horcruxes :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Derp... sorry! I think it must have to do with this: f591d98

I'm going to take a stroll down memory lane and if I can remember or rediscover the issue, I'll update this doc and follow up here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok, I updated the note here: 4d325ef

I think it's possible that this compatibility issue could be resolved now, because Schema::Visibility does have a top-level map of types, created by this code here:

def load_all(types: nil)

If it looks like your schema is affected by this issue, let me know and I'll give it a shot.

Copy link
Contributor

@gmac gmac Mar 6, 2025

Choose a reason for hiding this comment

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

Invaluable context, thanks for the quick reply. I'm going to read that new sentence on repeat this afternoon until it clicks. Also, my gosh – abstracts are hard.

Copy link
Owner Author

@rmosolgo rmosolgo Mar 6, 2025

Choose a reason for hiding this comment

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

Ha! If I've understood it properly, a minimal situation is like this:

class Thing < GraphQL::Schema::Object 
  implements Thingable 
end 

module Thingable 
  include GraphQL::Schema::Interface 
  field :f, String 
end 

class SomeUnion < GraphQL::Schema::Union
  possible_types(Thing)
  def self.visible?(...); false; end 
end 

class Query < GraphQL::Schema::Object
  field :thingable, Thingable 
  field :some_union, SomeUnion
end 

Going from old to new, I think the difference in visible schema would be this:

  type Query { 
    thingable: Thingable 
   }
 
  interface Thingable { 
    f: String 
  }

- type Thing implements Thingable { 
-   f: String 
- }

Visibility wouldn't check SomeUnion and discover Thing -- but Warden would, because it used the top-level map from Schema.types. To acheive compatibility, you'd have to add:

module Thingable 
  # ...
  orphan_types(Thing)
end 

Or add field :thing, Thing to Query -- just some way to discover Thing while traversing visible members of the schema.

But this might be fixable now...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ie: anything that's not the interface membership?

Yes, that's what I meant by that ... sorry for the frenzied terseness :P

orphan_types is specifically for the case when an object type is only used in the schema as an implementer of an interface. It's a practical consideration because, in Rails development, we need Rails to load the Object type class so that GraphQL-Ruby can know about the interface implementation. Otherwise, the file might never be loaded.

For example, in a schema like this:

type Query {
  node(id: ID): Node 
}

interface Node {
  id: ID 
}

type Thing implements Node {
  id: ID 
 }

GraphQL-Ruby would need Node.orphan_types(Thing).

Copy link
Contributor

@gmac gmac Mar 6, 2025

Choose a reason for hiding this comment

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

One last question then... how does Schema.orphan_types differ from Interface.orphan_types? I haven't always used the later but haven't noticed unexpected side effects. Is schema orphan types all-encompassing? Thanks again, this is all incredibly helpful (both for visibility upgrade and gem projects).

Copy link
Owner Author

Choose a reason for hiding this comment

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

The two orphan_types methods are basically the same: just ways to attach things to the schema that might not be attached. I don't know of any differences between them 😅 IIRC the gem doesn't even require than an Interface's orphan types implement that interface ... Yes, always using Schema.orphan_types is a fine approach -- Schema.from_definition just passes all object types to that method!

orphan_types(object_types)

Happy to help 🍻

Copy link
Contributor

@gmac gmac Mar 7, 2025

Choose a reason for hiding this comment

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

Schema.from_definition proceeds to do the same work again with the interfaces themselves directly below. I think that could be removed entirely (I omitted it from the stitching composer and it seems to work?).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah, good catch -- I'll try removing it in #5269

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.

2 participants