Skip to content

Replace excess &block in arguments to yield construction #1308

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

Merged
merged 1 commit into from
Mar 10, 2015

Conversation

davydovanton
Copy link
Contributor

No description provided.

@cupakromer
Copy link
Member

Thanks for the update. Is this solving a specific problem you ran into?

@davydovanton
Copy link
Contributor Author

@cupakromer for me yield cleaner and yield constructions more faster (almost five times) that &block

Benchmarks

(all developers love benchmarks 😃)

Code for compare yield and block

require 'benchmark/ips'

def yield_proc
  yield
end

def block_proc(&block)
  block.call
end

Benchmark.ips do |x|
  x.report('yield'){ yield_proc{ 1 + 1 } }
  x.report('block'){ block_proc{ 1 + 1 } }
end

Results

Calculating -------------------------------------
               yield   128.265k i/100ms
               block    62.716k i/100ms
-------------------------------------------------
               yield      5.821M (± 5.0%) i/s -     29.116M
               block      1.079M (±24.0%) i/s -      5.143M

@cupakromer
Copy link
Member

That's a legit reason 😸 Would be interesting to see what sort of difference that makes in an actual spec.

@davydovanton
Copy link
Contributor Author

@cupakromer i use this code for run benchmarks

require 'rails_helper'
require 'benchmark/ips'

# A very simple Rails engine
module MyEngine
  class Engine < ::Rails::Engine
    isolate_namespace MyEngine
  end

  Engine.routes.draw do
    resources :widgets, only: [:show] do
      get :random, on: :collection
    end
  end

  class WidgetsController < ::ActionController::Base
    def random
      @random_widget = Widget.all.shuffle.first
      redirect_to widget_path(@random_widget)
    end

    def show
      @widget = Widget.find(params[:id])
      render text: @widget.name
    end
  end
end

RSpec.describe MyEngine::WidgetsController, type: :controller do
  Benchmark.ips do |x|
    x.report('#routes with yield') do
      routes { MyEngine::Engine.routes }
    end
  end
end

This engine i found here

And here are the results:

Calculating -------------------------------------
      Master #routes     3.537k i/100ms
-------------------------------------------------
      Master #routes    218.726k (±41.3%) i/s -    565.920k

Calculating -------------------------------------
  #routes with yield     3.622k i/100ms
-------------------------------------------------
  #routes with yield    234.621k (±40.6%) i/s -    626.606k

@cupakromer
Copy link
Member

Thanks for the extra benchmark. As I was reviewing it I realized that it's not actually measuring the change since the block isn't actually invoked on the call to route. However, that got me thinking at which point I remembered, rspec-core has already done many benchmarks about this:

The results are very interesting:

This benchmark demonstrates that capturing a block (e.g. &block) has
a high constant cost, taking about 5x longer than a single yield
(even if the block is never used!).

However, fowarding a captured block can be faster than using yield
if the block is used many times (the breakeven point is at about 20-25
invocations), so it appears that he per-invocation cost of yield
is higher than that of a captured-and-forwarded block.

Note that there is no circumstance where using block.call is faster.

See also flat_map_vs_inject.rb, which appears to contradict these
results a little bit.

-- https://github.com/rspec/rspec-core/blob/master/benchmarks/capture_block_vs_yield.rb#L83-L95

and

Surprisingly, flat_map(&block) appears to be faster than
flat_map { yield } in spite of the fact that our array here
is smaller than the break-even point of 20-25 measured in the
capture_block_vs_yield.rb benchmark. In fact, the forwaded-block
version remains faster in my benchmarks here no matter how small
I shrink the words array. I'm not sure why!

-- https://github.com/rspec/rspec-core/blob/master/benchmarks/flat_map_vs_inject.rb#L37-L42

Given this information and how before is implemented I wrote this modified benchmark:

require 'benchmark/ips'

def before_n_times(n, &block)
  n.times { instance_exec(&block) }
end

def yield_n_times(n)
  before_n_times(n) { yield }
end

def capture_block_and_yield_n_times(n, &block)
  before_n_times(n) { yield }
end

def capture_block_and_call_n_times(n, &block)
  before_n_times(n) { block.call }
end

[10, 25, 50, 100, 1000, 10000].each do |count|
  puts "\n\nInvoking the block #{count} times\n"

  Benchmark.ips do |x|
    x.report("Yield #{count} times                  ") do
      yield_n_times(count) { }
    end

    x.report("Capture block and yield #{count} times") do
      capture_block_and_yield_n_times(count) { }
    end

    x.report("Capture block and call #{count} times ") do
      capture_block_and_call_n_times(count) { }
    end
  end
end

My results using Ruby 2.2.0:

Invoking the block 10 times
Calculating -------------------------------------
Yield 10 times                  
                        13.127k i/100ms
Capture block and yield 10 times
                        12.975k i/100ms
Capture block and call 10 times 
                        11.524k i/100ms
-------------------------------------------------
Yield 10 times                  
                        165.030k (± 5.7%) i/s -    827.001k
Capture block and yield 10 times
                        163.866k (± 5.9%) i/s -    817.425k
Capture block and call 10 times 
                        137.892k (± 7.3%) i/s -    691.440k


Invoking the block 25 times
Calculating -------------------------------------
Yield 25 times                  
                         7.305k i/100ms
Capture block and yield 25 times
                         7.314k i/100ms
Capture block and call 25 times 
                         6.047k i/100ms
-------------------------------------------------
Yield 25 times                  
                         84.167k (± 5.6%) i/s -    423.690k
Capture block and yield 25 times
                         82.110k (± 6.4%) i/s -    409.584k
Capture block and call 25 times 
                         67.144k (± 6.2%) i/s -    338.632k


Invoking the block 50 times
Calculating -------------------------------------
Yield 50 times                  
                         4.211k i/100ms
Capture block and yield 50 times
                         4.181k i/100ms
Capture block and call 50 times 
                         3.410k i/100ms
-------------------------------------------------
Yield 50 times                  
                         45.223k (± 5.0%) i/s -    227.394k
Capture block and yield 50 times
                         45.253k (± 4.9%) i/s -    225.774k
Capture block and call 50 times 
                         36.181k (± 5.7%) i/s -    180.730k


Invoking the block 100 times
Calculating -------------------------------------
Yield 100 times                  
                         2.356k i/100ms
Capture block and yield 100 times
                         2.305k i/100ms
Capture block and call 100 times 
                         1.842k i/100ms
-------------------------------------------------
Yield 100 times                  
                         23.677k (± 7.1%) i/s -    117.800k
Capture block and yield 100 times
                         24.039k (± 4.7%) i/s -    122.165k
Capture block and call 100 times 
                         18.888k (± 6.6%) i/s -     95.784k


Invoking the block 1000 times
Calculating -------------------------------------
Yield 1000 times                  
                       244.000  i/100ms
Capture block and yield 1000 times
                       245.000  i/100ms
Capture block and call 1000 times 
                       192.000  i/100ms
-------------------------------------------------
Yield 1000 times                  
                          2.540k (± 4.3%) i/s -     12.688k
Capture block and yield 1000 times
                          2.499k (± 5.6%) i/s -     12.495k
Capture block and call 1000 times 
                          1.975k (± 5.1%) i/s -      9.984k


Invoking the block 10000 times
Calculating -------------------------------------
Yield 10000 times                  
                        24.000  i/100ms
Capture block and yield 10000 times
                        24.000  i/100ms
Capture block and call 10000 times 
                        19.000  i/100ms
-------------------------------------------------
Yield 10000 times                  
                        232.923  (±15.5%) i/s -      1.128k
Capture block and yield 10000 times
                        212.504  (±21.6%) i/s -    936.000 
Capture block and call 10000 times 
                        184.090  (±10.3%) i/s -    912.000 

This seems to show that the error margin is enough to negate any benefit from capturing the block initially. It also shows that not capturing the block is still faster at low rates of calling. If this holds for your system, I think this PR is good as is and we won't need to capture the block in the route method, but still use yield.

@davydovanton
Copy link
Contributor Author

@cupakromer wow! thanks for this great answer and the investigation of the problem 💛 💚 💜

@davydovanton
Copy link
Contributor Author

@cupakromer ping 😃 What do we with this PR?

cupakromer added a commit that referenced this pull request Mar 10, 2015
Replace excess &block in arguments to yield construction
@cupakromer cupakromer merged commit e28cb1a into rspec:master Mar 10, 2015
@cupakromer
Copy link
Member

Thanks again for the work on this. 😸

cupakromer added a commit that referenced this pull request Mar 10, 2015
cupakromer added a commit that referenced this pull request Mar 10, 2015
@davydovanton
Copy link
Contributor Author

@cupakromer thanks you too! 💚 💛 ❤️

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