Speed up one_of?

[jbourassa]

Speed up one_of? by defining it as false, and when one_of is used, redefining the method as true.

In profiles, I noticed that oneOf? was taking some time. Looping through the directive list each time feels wasteful.

This approach borrows from Rails' Class#class_attribute and redefines the one_of? when calling the one_of class method. This respects the fact that directives are inherited from parent classes; so is the one_of? class method. That said, this is not 100% backward compatible because users could be adding the OneOf directive without using the one_of DSL, but... it still feels pretty safe?

I didn't see a benchmark that had a lot of input, and in rake bench:query, the results are well within the margin of error. In a benchmark in our app though, where we're only validating a Hash against a GraphQL type, this made a difference.

[jbourassa]

Thanks for running the tests, I thought they passed but I must’ve missed something.

I’ll take a look. Marking this as draft until then.

[jbourassa]

The tests failures were cases where the OneOf directive was added explicitly without calling one_of. That's the incompatibility risk I called out in the description 😅. I fixed those.

I also built a benchmark to show the gains:

Before:

  Calculating -------------------------------------
  input list - 10 elem      3.145k (± 1.4%) i/s -     15.750k in   5.008840s
  input list - 100 elem
                          726.600  (± 1.5%) i/s -      3.672k in   5.054912s
  input list - 1000 elem
                           83.429  (± 1.2%) i/s -    424.000  in   5.083546s

After:

  Calculating -------------------------------------
  input list - 10 elem      3.279k (± 2.2%) i/s -     16.500k in   5.034949s
  input list - 100 elem
                          808.152  (± 1.5%) i/s -      4.080k in   5.049768s
  input list - 1000 elem
                           94.321  (± 2.1%) i/s -    477.000  in   5.058867s

[rmosolgo]

Hey, thanks for this suggestion. I'm all for performance improvements. The only thing that gives me the willies is define_method. I've had enough bad experiences where it has unexpected performance impacts that I try to steer clear of it whenever I can. I can think of an alternative suggestion, tell me what you think:

• Keep defining def one_of? to return false

• Add a new module (say, Directive::OneOf::IsOneOf 🤷 ) that implements def one_of? to return true

• In Directive::OneOf#initialize, call @owner.extend ... with that new module

  (#initialize receives the schema member which is getting the directive:

  graphql-ruby/lib/graphql/schema/directive.rb

  Lines 114 to 115 in fdfc892

  	def initialize(owner, **arguments)
  	@owner = owner

That would help me sleep better at night -- avoiding define_method 😅 -- and I think it would also retain compatibility where the directive is added manually. What do you think of that approach?

[jbourassa]

That sounds like a sensible approach to me. In this case I think the perf hit from define_method is next to none (or at least, I didn't notice anything after migrating to from another approach in my in-app monkey patch), but the incompatibility is unfortunate.

The tradeoff is that one_of? definitions are farther apart thus a little less obvious, but that's price worth paying.

[rmosolgo]

perf hit

I can believe it, but, what about, say, the binding? Ruby won't GC any local variables in scope because maybe they'll be used by the dynamically-defined method later on. (At least, it used to work that way. Maybe it's smarter now?) I've traced big memory bloat to stuff like that in the past -- and even if there's nothing of consequence in the local scope now, maybe something would get added later on...

[jbourassa]

Updated with your suggestion 👍, tests should be green, at least they were on my machine.

[jbourassa]

The gem now supports Ruby 2.7+, so this should work. Feel free to change it to something else if you prefer.

[rmosolgo]

🙀 Beautiful, I bet there are many other places in this library that would benefit from this. What a time to be alive!

[rmosolgo]

Thanks for this improvement!