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?

@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.

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.

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.

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

@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.

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

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

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

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.

@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.

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.

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

@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

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

@rafaelfranca Can this be merged now?

@rafaelfranca Wow! That was quick. Thanks!

Thanks 😃