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

Pass symbol as an argument instead of a block #16833

Merged
merged 1 commit into from Nov 29, 2014

Conversation

Projects
None yet
8 participants
@sferik
Copy link
Contributor

sferik commented Sep 7, 2014

Using Ruby’s Symbol#to_proc is considerably more concise than using block syntax. It’s also about 20% faster (see benchmarks below). Symbol#to_proc is already used in many places throughout the Rails codebase, but not everywhere. This patch makes the codebase more consistent and concise. In some cases, it reduces the number of lines of code. For example, in railties/lib/rails/application/routes_reloader.rb:

def finalize!
  route_sets.each do |routes|
    routes.finalize!
  end
end

becomes:

def finalize!
  route_sets.each(&:finalize!)
end

The net result is 30 fewer lines of code.

For those who don’t know the history, Symbol#to_proc was originally part of Active Support, before being merged into the Ruby language in version 1.9 (and back-ported to 1.8.7). The Active Support implementation was slightly slower than using a block but the C implementation in MRI is considerably faster.

Benchmark
require 'benchmark/ips'

def block
  (1..100).map{|i| i.to_s}
end

def to_proc
  (1..100).map(&:to_s)
end

Benchmark.ips do |x|
  x.report('block')   { block   }
  x.report('to_proc') { to_proc }
end
Ruby 2.1.2p95
  block 46707.3 (±7.8%) i/s - 235768 in 5.079617s
to_proc 55143.5 (±7.2%) i/s - 276554 in 5.040752s
Ruby 2.0.0p481
  block 33271.0 (±6.8%) i/s - 168646 in 5.093196s
to_proc 37346.0 (±7.6%) i/s - 188870 in 5.087220s
Ruby 1.9.3p545
  block 31478.7 (±11.3%) i/s - 156876 in 5.056661s
to_proc 39219.9  (±7.2%) i/s - 195210 in 5.004236s

@sferik sferik force-pushed the sferik:symbol_to_proc branch Sep 7, 2014

@jeremy

This comment has been minimized.

Copy link
Member

jeremy commented Sep 7, 2014

We're generally neutral to negative on en masse cosmetic changes (considering few/none of these are in perf hotspots). But it feels like a good time to clean house and sweep out some cobwebs, too. Maybe after 4-2-stable branches?

@sferik

This comment has been minimized.

Copy link
Contributor Author

sferik commented Sep 7, 2014

@jeremy I included the benchmarks to try to make the case that these changes were not purely cosmetic. I understand the folly of micro-benchmarks but these 20% performance improvements, spread over 250 spots in the code, could add up to real-world benefits for Rails users, depending on their usage of the framework. For example, I wouldn’t be surprised if Hash#stringify_keys (including variants like stringify_keys! and deep_stringify_keys) are invoked thousands of times per HTTP request in many Rails applications. Of course, it’s hard to know what are hotspots in a framework since everyone invokes the code differently, depending on their particular use case.

Since I believe this is a real performance win, I’d obviously prefer it to be included in version 4.2.0 but I’d also be happy if this was merged after the 4.2.0 release, if that is preferred for some reason.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Sep 7, 2014

If we're going to merge it, we're probably better off having it in 4-2-stable. Otherwise, it'll cause (admittedly, minor) backport pain for the entire life of 5.x.

Edit: On the other hand, as we're already planning a load of rearrangement post-4.2 (kwargs, etc), maybe it is actually better grouped with that -- and leave 4.2 looking more like 4.1.

@sferik

This comment has been minimized.

Copy link
Contributor Author

sferik commented Sep 7, 2014

We're generally neutral to negative on en masse cosmetic changes…

@jeremy I’m not sure whether you italicized en masse for emphasis or because it has a foreign origin (French, I believe). Anyway, in case this pull request would be accepted if it was broken up into smaller commits (e.g. one for each Rails component) that could be cherry-picked, I’d be happy to make that change. Just say the word.

@thedarkone

This comment has been minimized.

Copy link
Contributor

thedarkone commented Sep 8, 2014

How times have changed :), I remember when Symbol#to_proc was being purged from Rails :). 37b67df ce4a1bb

@sferik

This comment has been minimized.

Copy link
Contributor Author

sferik commented Sep 8, 2014

@thedarkone Thanks for the history lesson. I didn’t know about those commits. Since the reason those commits were made has not been valid for a while, I think it’s high time to reverse them.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Sep 8, 2014

👍 for merging this after the 4-2-stable branch is created.

@sferik

This comment has been minimized.

Copy link
Contributor Author

sferik commented Sep 8, 2014

@rafaelfranca Sounds good. I’ll rebase after 4-2-stable branches (if necessary) and bump this PR.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Sep 8, 2014

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Sep 8, 2014

Great! I'll leave the tab open in my browser so I'll not forgot about it 😄

@sferik sferik force-pushed the sferik:symbol_to_proc branch 2 times, most recently Sep 12, 2014

@mikz mikz referenced this pull request Sep 23, 2014

Merged

Fix Ruby 2.1 warnings #10

@bquorning

This comment has been minimized.

Copy link
Contributor

bquorning commented Sep 26, 2014

I’m all for the consistency and conciseness arguments of this pull request. But regarding the performance argument, you need to consider the size of the enumerables we’re looping. Here are my results of your benchmark code, run with (1..2) instead of (1..100):

Ruby 2.1.2p95
  block 1460633.3 (±2.2%) i/s - 7326816 in 5.018898s
to_proc 1321673.3 (±2.1%) i/s - 6675760 in 5.053373s
Ruby 2.0.0p451
  block 1318228.3 (±6.1%) i/s - 6592362 in 5.023987s
to_proc 1182754.1 (±2.7%) i/s - 5952696 in 5.036684s
Ruby 1.9.3p545
  block 1384769.2 (±2.1%) i/s - 6973778 in 5.038321s
to_proc 1235268.2 (±4.7%) i/s - 6214500 in 5.043843s

So, for very small arrays, #to_proc is ~10% slower than the block syntax. In my tests, it looks like the two syntaxes are equally fast on arrays of around size 5.

@sferik

This comment has been minimized.

Copy link
Contributor Author

sferik commented Sep 26, 2014

@bquorning Thanks for running your own benchmarks and pointing this out.

I’d argue that for small enumerables, performance is less critical. I was optimizing to make the worst-case scenario 20% faster, even if that comes at the cost of making the best-case scenario 10% slower. I stand by that trade-off but it might be worth looking at the individual cases and applying this change more selectively, rather than across the board.

In the example I used above (iterating over route_sets), I think it’s reasonable to assume that number will be greater than 5 for any production Rails application. However, there are other examples (e.g. iterating over email attachments), where the number is likely to be less than 5, most of the time.

I decided to optimize for the worst-case scenario and apply the change consistently throughout the codebase but I’m happy to consider each case individually and make a judgment about whether the enumerable is likely to be greater or less than 5.

@bquorning

This comment has been minimized.

Copy link
Contributor

bquorning commented Sep 28, 2014

I’d argue that for small enumerables, performance is less critical. I was optimizing to make the worst-case scenario 20% faster, even if that comes at the cost of making the best-case scenario 10% slower.

I agree. The point of my benchmarks was just to highlight that the 20% performance improvement doesn’t apply for all use cases.

I’m fine with changing to use #to_proc across the board.

@huacnlee

This comment has been minimized.

Copy link
Contributor

huacnlee commented Sep 30, 2014

to_proc not always faster than block, like this case

ruby 2.1.3p242

class Foo
  def initialize
    @foo = 1
    @bar = 2
    @dar = 3
  end
end
@foo = Foo.new

Benchmark.ips do |x|
  x.report("block") {
    @foo.instance_variables.map { |var| var.to_s }
  }
  x.report("to_proc") {
    @foo.instance_variables.map(&:to_s)
  }
  x.compare!
end
Calculating -------------------------------------
               block      1829 i/100ms
             to_proc      1819 i/100ms
-------------------------------------------------
               block  1057345.0 (±15.3%) i/s -    2756303 in   2.787981s
             to_proc  1000665.4 (±14.9%) i/s -    4532948 in   4.839811s

Comparison:
               block:  1057345.0 i/s
             to_proc:  1000665.4 i/s - 1.06x slower
@sferik

This comment has been minimized.

Copy link
Contributor Author

sferik commented Sep 30, 2014

@huacnlee You seem to be raising the same objection as @bquorning. For small values of n, passing a block is slightly faster (because it doesn’t have to instantiate a new Proc object), however, if you have more instance variables (over 5), the comparison will favor to_proc.

require 'benchmark/ips'

class Foo
  def initialize
    @a, @b, @c, @d, @e, @f, @g, @h, @i = 1, 2, 3, 4, 5, 6, 7, 8, 9
  end
end

@foo = Foo.new

Benchmark.ips do |x|
  x.report('block') {
    @foo.instance_variables.map { |var| var.to_s }
  }
  x.report('to_proc') {
    @foo.instance_variables.map(&:to_s)
  }
  x.compare!
end
Calculating -------------------------------------
               block     26506 i/100ms
             to_proc     25584 i/100ms
-------------------------------------------------
               block   353692.8 (±14.3%) i/s -    1749396 in   5.057290s
             to_proc   384096.3 (±12.8%) i/s -    1893216 in   5.020770s

Comparison:
             to_proc:   384096.3 i/s
               block:   353692.8 i/s - 1.09x slower

@sferik sferik force-pushed the sferik:symbol_to_proc branch 2 times, most recently Oct 27, 2014

@sferik sferik force-pushed the sferik:symbol_to_proc branch Nov 14, 2014

@sferik sferik force-pushed the sferik:symbol_to_proc branch to d1374f9 Nov 29, 2014

@sferik

This comment has been minimized.

Copy link
Contributor Author

sferik commented Nov 29, 2014

👍 for merging this after the 4-2-stable branch is created.

@rafaelfranca Can this be merged now?

rafaelfranca added a commit that referenced this pull request Nov 29, 2014

Merge pull request #16833 from sferik/symbol_to_proc
Pass symbol as an argument instead of a block

@rafaelfranca rafaelfranca merged commit 390449a into rails:master Nov 29, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
@sferik

This comment has been minimized.

Copy link
Contributor Author

sferik commented Nov 29, 2014

@rafaelfranca Wow! That was quick. Thanks!

@jonatack

This comment has been minimized.

Copy link
Contributor

jonatack commented Nov 30, 2014

Thanks 😃

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.