Skip to content

Schema::Visibility is much slower than Schema::Warden #5324

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

Closed
myronmarston opened this issue Apr 9, 2025 · 4 comments · Fixed by #5325
Closed

Schema::Visibility is much slower than Schema::Warden #5324

myronmarston opened this issue Apr 9, 2025 · 4 comments · Fixed by #5325

Comments

@myronmarston
Copy link

myronmarston commented Apr 9, 2025

Describe the bug

I've been working on deploying an upgrade to one of our internal systems, and discovered that using GraphQL::Schema::Visibility is much slower than the GraphQL::Schema::Warden plugin that we've been using. According to my benchmark, it's more than 20x slower for a very simple query!

Versions

graphql version: 2.5.2
rails (or other framework): N/A
other applicable versions (graphql-batch, etc): N/A

GraphQL schema

https://gist.github.com/myronmarston/ee6fa2d92d2941b7071459708f7d46d1

(Apologies for the large anonymized schema, but I didn't think I should share our proprietary internal schema and I wasn't able to come up with a smaller schema that reproduced this issue--so I just replaced all the type names and field names to anonymize it.).

GraphQL query

  query {
    field264(
      field265: [ENUM_VALUE1000]
    ) {
      __typename
    }
  }

Steps to reproduce

Create a ruby script with this code:

require "bundler/inline"

gemfile do
  source "https://rubygems.org"

  gem "graphql", "2.5.2"
  gem "benchmark-ips"
end

require "benchmark/ips"
require "graphql"
require "open-uri"

SCHEMA_STRING = URI("https://gist.githubusercontent.com/myronmarston/ee6fa2d92d2941b7071459708f7d46d1/raw/8b99a4d9723dc99a7a921cf787c8e6b31a8fc6b7/anonymized_schema.graphql").read

class Application
  def initialize(using: {})
    @schema = ::GraphQL::Schema.from_definition(
      SCHEMA_STRING,
      default_resolve: self,
      using: using
    )
  end

  def call(parent_type, field, object, args, context)
    nil
  end

  def execute_query(query_string)
    response = ::GraphQL::Query.new(@schema, query_string, variables: {}).result

    unless response.fetch("errors", []).empty?
      raise "Query failed: #{response.to_h.inspect}"
    end

    response
  end
end

warden_app = Application.new(using: {
  GraphQL::Schema::Warden => {}
})

visibility_app = Application.new(using: {
  GraphQL::Schema::Visibility => {preload: true}
})

query = <<~EOS
  query {
    field264(
      field265: [ENUM_VALUE1000]
    ) {
      __typename
    }
  }
EOS

Benchmark.ips do |x|
  x.report("Using Visibility") do
    visibility_app.execute_query(query)
  end

  x.report("Using Warden") do
    warden_app.execute_query(query)
  end

  x.compare!
end

Run it.

Expected behavior

I expect the new GraphQL::Schema::Visibility plugin to be as performant as GraphQL::Schema::Warden. (At least, I need it to be as performant before I can switch to the new plugin, and I'm expecting Warden to be removed in a future version.)

Actual behavior

The Visibility plugin is ~20x slower:

ruby 3.2.7 (2025-02-04 revision 02ec315244) [arm64-darwin24]
Warming up --------------------------------------
    Using Visibility    13.000 i/100ms
        Using Warden   253.000 i/100ms
Calculating -------------------------------------
    Using Visibility    127.969 (± 0.8%) i/s    (7.81 ms/i) -    650.000 in   5.079653s
        Using Warden      2.742k (± 2.8%) i/s  (364.70 μs/i) -     13.915k in   5.079058s

Comparison:
        Using Warden:     2742.0 i/s
    Using Visibility:      128.0 i/s - 21.43x  slower
myronmarston added a commit to block/elasticgraph that referenced this issue Apr 9, 2025
We've observed that the new `GraphQL::Schema::Visibility` plugin causes a
large performance degradation. `GraphQL::Schema::Warden` does not have this
issue, so until it's fixed we'll stick with it.

```
ruby 3.3.4 (2024-07-09 revision be1089c8ec) [arm64-darwin23]
Warming up --------------------------------------
    Using Visibility    10.000 i/100ms
        Using Warden   248.000 i/100ms
Calculating -------------------------------------
    Using Visibility    102.340 (± 2.0%) i/s    (9.77 ms/i) -    520.000 in   5.082553s
        Using Warden      2.471k (± 4.9%) i/s  (404.71 μs/i) -     12.400k in   5.030776s

Comparison:
        Using Warden:     2470.9 i/s
    Using Visibility:      102.3 i/s - 24.14x  slower
```

See rmosolgo/graphql-ruby#5324 for details on this issue.
@rmosolgo
Copy link
Owner

rmosolgo commented Apr 9, 2025

Hey, thanks so much for reporting this. Performance is definitely one of the goals for Schema::Visibility!

I took your script and slapped some Stackprof and MemoryProfiler on it and quickly found the problem:

if (dup_defn = non_dups.find { |d| d.graphql_name == defn.graphql_name })

It looks like I didn't benchmark schemas with very large sets of fields/arguments/enum values, so the current implementation has a bad trade-off there. In #5325 I was able to bring Visibility up to parity with Warden. And for sport, I added a new example application:

visibility_profile_app = Application.new(using: {
  GraphQL::Schema::Visibility => {preload: true, profiles: { all: {} }}
})

# ... 

x.report("Using Visibility with Profile") do
  visibility_profile_app.execute_query(query, context: { visibility_profile: :all })
end

It's 2.5x faster 😎

Comparison:
Using Visibility with Profile:     3576.7 i/s
    Using Visibility:     1438.0 i/s - 2.49x  slower
        Using Warden:     1306.4 i/s - 2.74x  slower

Could you try the patch from #5325 on your app to see if it looks good in real life?

gem "graphql", github: "rmosolgo/graphql-ruby", ref: "fix-vis-for-large-lists"

@rmosolgo
Copy link
Owner

rmosolgo commented Apr 9, 2025

Apologies for the large anonymized schema

This was brilliant, by the way. If you don't mind sharing the script that transformed it, I'd love to recommend it to people in the same situation. Getting the exact same schema structure is a huge deal in replication -- and names don't matter!

@myronmarston
Copy link
Author

Thanks for the incredibly quick fix!

It's 2.5x faster 😎

Woah. I hadn't really dug into visibility profiles yet but clearly I need to get that setup :). Thanks for the tip.

I found something interesting when updating my benchmark script to include both the profile thing you pointed out and also using GraphQL::Schema::AlwaysVisible:

require "bundler/inline"

gemfile do
  source "https://rubygems.org"

  if ENV["TRY_BRANCH"]
    gem "graphql", git: "https://github.com/rmosolgo/graphql-ruby.git", ref: "e2dfdf5954429d8302ef912c89a55ddac0ae6390"
  else
    gem "graphql", "2.5.2"
  end
  gem "benchmark-ips"
end

require "benchmark/ips"
require "graphql"
require "open-uri"

SCHEMA_STRING = URI("https://gist.githubusercontent.com/myronmarston/ee6fa2d92d2941b7071459708f7d46d1/raw/8b99a4d9723dc99a7a921cf787c8e6b31a8fc6b7/anonymized_schema.graphql").read

class Application
  def initialize(using: {})
    @schema = ::GraphQL::Schema.from_definition(
      SCHEMA_STRING,
      default_resolve: self,
      using: using
    )
  end

  def call(parent_type, field, object, args, context)
    nil
  end

  def execute_query(query_string, context: {})
    response = ::GraphQL::Query.new(@schema, query_string, variables: {}, context: context).result

    unless response.fetch("errors", []).empty?
      raise "Query failed: #{response.to_h.inspect}"
    end

    response
  end
end

warden_app = Application.new(using: {
  GraphQL::Schema::Warden => {}
})

visibility_app = Application.new(using: {
  GraphQL::Schema::Visibility => {preload: true}
})

visibility_profile_app = Application.new(using: {
  GraphQL::Schema::Visibility => {preload: true, profiles: { all: {} }, dynamic: true}
})

always_visible_app = Application.new(using: {
  GraphQL::Schema::AlwaysVisible => {}
})

query = <<~EOS
  query {
    field264(
      field265: [ENUM_VALUE1000]
    ) {
      __typename
    }
  }
EOS

Benchmark.ips do |x|
  x.report("Using Visibility with dynamic") do
    visibility_profile_app.execute_query(query)
  end

  x.report("Using Visibility with profile") do
    visibility_profile_app.execute_query(query, context: { visibility_profile: :all })
  end

  x.report("Using Visibility") do
    visibility_app.execute_query(query)
  end

  x.report("Using Warden") do
    warden_app.execute_query(query)
  end

  x.report("Using AlwaysVisible") do
    always_visible_app.execute_query(query)
  end

  x.compare!
end

Results w/o your fix (not passing TRY_BRANCH=1):

ruby 3.2.7 (2025-02-04 revision 02ec315244) [arm64-darwin24]
Warming up --------------------------------------
Using Visibility with dynamic
                        12.000 i/100ms
Using Visibility with profile
                       731.000 i/100ms
    Using Visibility    12.000 i/100ms
        Using Warden   280.000 i/100ms
 Using AlwaysVisible   286.000 i/100ms
Calculating -------------------------------------
Using Visibility with dynamic
                        128.391 (± 0.8%) i/s    (7.79 ms/i) -    648.000 in   5.047754s
Using Visibility with profile
                          6.806k (± 7.7%) i/s  (146.93 μs/i) -     34.357k in   5.077871s
    Using Visibility    123.572 (± 2.4%) i/s    (8.09 ms/i) -    624.000 in   5.052369s
        Using Warden      2.721k (± 2.9%) i/s  (367.54 μs/i) -     13.720k in   5.047243s
 Using AlwaysVisible      2.789k (± 2.3%) i/s  (358.61 μs/i) -     14.014k in   5.028334s

Comparison:
Using Visibility with profile:     6806.0 i/s
 Using AlwaysVisible:     2788.5 i/s - 2.44x  slower
        Using Warden:     2720.8 i/s - 2.50x  slower
Using Visibility with dynamic:      128.4 i/s - 53.01x  slower
    Using Visibility:      123.6 i/s - 55.08x  slower

Results w/ your fix (passing TRY_BRANCH=1):

ruby 3.2.7 (2025-02-04 revision 02ec315244) [arm64-darwin24]
Warming up --------------------------------------
Using Visibility with dynamic
                       289.000 i/100ms
Using Visibility with profile
                       685.000 i/100ms
    Using Visibility   288.000 i/100ms
        Using Warden   270.000 i/100ms
 Using AlwaysVisible   277.000 i/100ms
Calculating -------------------------------------
Using Visibility with dynamic
                          2.892k (± 3.2%) i/s  (345.75 μs/i) -     14.450k in   5.001495s
Using Visibility with profile
                          6.916k (± 2.2%) i/s  (144.60 μs/i) -     34.935k in   5.053943s
    Using Visibility      2.857k (± 3.6%) i/s  (350.07 μs/i) -     14.400k in   5.047974s
        Using Warden      2.705k (± 3.1%) i/s  (369.72 μs/i) -     13.770k in   5.096533s
 Using AlwaysVisible      2.770k (± 2.7%) i/s  (361.05 μs/i) -     13.850k in   5.004324s

Comparison:
Using Visibility with profile:     6915.6 i/s
Using Visibility with dynamic:     2892.3 i/s - 2.39x  slower
    Using Visibility:     2856.6 i/s - 2.42x  slower
 Using AlwaysVisible:     2769.7 i/s - 2.50x  slower
        Using Warden:     2704.7 i/s - 2.56x  slower

AlwaysVisible is slower than using Visibility with a profile (by ~2.5x!). That surprises me because https://graphql-ruby.org/authorization/visibility.html#opting-out suggests it should be faster, as it opts out of visibility checks that would otherwise happen. If it's faster to just use Visibility with an all profile then there's not much reason for AlwaysVisible to exist. Seems like an optimization opportunity?

Could you try the patch from #5325 on your app to see if it looks good in real life?

I tried some local benchmarks (on my laptop) of a real query (not the simple __typename one...) and confirmed it was a lot faster. I've also kicked things off so I have the option of deploying my app with your fix but that'll take time--we have a CI/CD pipeline that "puts it through its paces", so to speak. I'm completely satisfied with your fix, if you'd like to go ahead and merge. If you want the extra confidence from me deploying, let me know, but I'd rather spend my time adopting a visibility profile for even greater speed in my app :).

If you don't mind sharing the script that transformed it, I'd love to recommend it to people in the same situation.

Here you go:

https://gist.github.com/myronmarston/4feeeaa5472096d3c1a09eea8d983046

FWIW, goose wrote this for me :).

@rmosolgo
Copy link
Owner

rmosolgo commented Apr 9, 2025

Ah... I haven't updated AlwaysVisible yet. My "long" term goal is to make that the GraphQL-Ruby default, and only turn on visibility checks when they're explicitly enabled, just to keep the performance overhead down. But in the meantime, I could update it to basically the code from this example! (I tried it: #5326)

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 a pull request may close this issue.

2 participants