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

Speed up lookup of possible types for Fragments #4506

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

amomchilov
Copy link
Contributor

@amomchilov amomchilov commented Jun 7, 2023

Really this is just a readability improvement, but there's a nice perf side effect, too :)

This change speeds up rake bench:profile_large_result benchmark by about 1.8%.

Baseline (master at cbdae76)

Warming up --------------------------------------
Querying for 1000 objects
                         1.000  i/100ms
Calculating -------------------------------------
Querying for 1000 objects
                          8.149  (± 0.0%) i/s -    815.000  in 100.061220s

After this change:

Warming up --------------------------------------
Querying for 1000 objects
                         1.000  i/100ms
Calculating -------------------------------------
Querying for 1000 objects
                          8.295  (± 0.0%) i/s -    830.000  in 100.093047s

This logic used to be more complicated (t.metadata[:type_class] == owner_type), but has since been simplified to doing a search for an exact-match (t == owner_type).

# Faster than .map{}.include?()
query.warden.possible_types(type_defn).each do |t|
if t.metadata[:type_class] == owner_type
gather_selections(owner_type, node.selections, selections_by_name)
break
end
end

include? can do this more expressively than each + break, and has the added benefit of being way faster (since it can run the loop body in C code, rather than needing to call into a Ruby block). Here's a dumb little synthetic benchmark to demonstrate that:

Benchmark
#!/usr/bin/ruby

require "benchmark/ips"

a = Array(1...100)

def found_it!; end

Benchmark.ips do |x|
  x.time = 30
  x.warmup = 10
  x.time = 1
  x.warmup = 1
  
  x.report("each+break") do |times|
    i = 0
    while i < times
      i += 1
    
      a.each do |x|
        if x == 90
          found_it!
          break
        end
      end
    
    end
  end
  
  x.report("include?") do |times|
    i = 0
    while i < times
      i += 1
      
      if a.include?(90)
        found_it!
      end
      
    end
  end
  
  x.compare!
end
Warming up --------------------------------------
          each+break    32.413k i/100ms
            include?   264.562k i/100ms
Calculating -------------------------------------
          each+break    322.606k (± 2.6%) i/s -    324.130k in   1.005439s
            include?      2.631M (± 1.1%) i/s -      2.646M in   1.005860s

Comparison:
            include?:  2630515.3 i/s
          each+break:   322606.5 i/s - 8.15x  slower

@rmosolgo rmosolgo merged commit 250a724 into rmosolgo:master Jun 9, 2023
13 checks passed
@rmosolgo
Copy link
Owner

rmosolgo commented Jun 9, 2023

Nice find! Thanks for sharing this improvement.

@rmosolgo rmosolgo added this to the 2.0.23 milestone Jun 9, 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