Remove Symbol#to_proc from framework code

I don't understand the reason for this patch. Can you explain further?

Ah I'm sorry, of course, I forgot the most important thing.

Symbol#to_proc, the way it is defined in Rails (as a core extension), is slower than just using plain blocks (see http://www.lukeredpath.co.uk/200... - old but still true).

Ruby 1.9 defines Symbol#to_proc natively and it's reportedly faster but I'd say almost everyone running Rails is still on 1.8.x (not because of Rails, but because of other dependencies).

Anyway, this patch simply replaces invocations of Symbol#to_proc with a plain block argument in the framework code. This should improvement performance slightly.

-1

I'd argue that the slight performance increase isn't worth the readability hit introduced by this patch. Take, for example, this snippet:

filter_chain.select(&:after?).map(&:method)

Which is changed to:

filters = []
for filter in filter_chain
  filters << filter.method if filter.after?
end
filters

Also, since Symbol#to_proc will be in 1.9 (as you said), the performance issue is only temporary.

If that for loop were changed to an #each block, that would suffice for me, I don't need a return to #map followed by #select. This patch is for performance, after all, and Rails performance is a concern. A plain #each is twice as quick as a #map and #select.

I'd like to see some metrics on how much of a performance boost this produces.

That'd be nice -- but, +1 with or without.

I see no reason to remove it, especially since it's in 1.9. If you don't want it, don't use it. For people who prefer readability, they can use it. If your largest bottleneck is Symbol#to_proc, don't use it.

I'm kinda -1 on this as well. As much as I like improving Rails performance, I'm reluctant to sacrifice readability and code beauty for it. My mind could be changed if the metrics showed otherwise though.

@Tammer: I disagree, 1.8.x is here and now and will be around for quite awhile. I don't think too much readability is sacrificed here considering this is framework code, not application code.

And yes, Eric is right about the for-loop - it's an intentional change for performance by doing 1 iteration instead of 2. Enumerable#each vs. for-loop doesn't really matter to me - for-loops are very slightly faster than using #each but I personally still prefer #each as a matter of style :)

To be clear, the patch doesn't remove Symbol#to_proc - it just removes usage of it in Rails framework code.

bq. I don't think too much readability is sacrificed here considering this is framework code, not application code.

I disagree with that argument. Readability is important for code - no matter in what context it's used. Reducing readability here for no provably good reason just raises the barrier to entry for contributions.

That being said, if I see convincing stats that show a measurable increase in performance, I'll +1 it.

Yes, I see that now. Still, I question this being a useful (or even significant) speedup.

-1

People who are concerned with performance should not use a lot of things that comes with Rails. However it doesn't mean we should tweak all that code.

In current version of Rails we have the existing code which might be slow but it is fast enough for majority to not to be an issue.

The alternative being proposed is difficult to read.

I'm hearing everyone's call for readability. Really. When I first learnt about Symbol#to_proc, I loved this wonderful thing that made my code more concise and elegant (and that allowed me to show off to non-Rubyists).

I ran a similar benchmark to Luke's at http://www.lukeredpath.co.uk/200... and got the same 3-4x improvements for large enumerables (http://pastie.org/221881) and 2x improvement for small ones (http://pastie.org/221882).

It's an itsy bit of perf improvement so it's really up for debate (as is happening now).

I like to think that the readability is worth sacrificing in this case because everybody understands plain old Ruby blocks - it's not like the patched code is that much harder to read. Please do point out exactly what you find hard to read (other than what Tammer pointed out above, which may not seem as elegant nor as idiomatic but imho is easier to read).

Sorry the links to the benchmark pasties in my last comment were broken because auto_link included the closing ")". Here they are:

http://pastie.org/221881

http://pastie.org/221882

-1

I agree with Tammer et al regarding readability, and I'm also concerned about what kind of precedence this sets. Are we suggesting that future patches should be rejected in the event that they take advantage of Symbol#to_proc?

Instead of the micro benchmark on #to_proc itself, what percentage of a typical Rails request is affected by this? It's got to be so minimal that it's not even part of any significant digits.

(Questions are rhetorical)

+1.

That stuff doesn't belong in the framework. KISS and what not.

That stuff doesn't belong in the framework. KISS and what not.

Respectfully, I think that's crazy. Why stop here? Why not instead re-write rails as a giant C extension?

(Again rhetorical)

I'm sure way more human cycles were spent on this thread than all the CPU cycles "churning" through Symbol#to_proc. Let's keep things in perspective.

Respectfully, I think that's crazy. Why stop here? Why not instead re-write rails as a giant C extension?

(Again rhetorical)

Although this may have been meant as a completely absurd rhetorical question to make a point, it demands an answer.

Readability is only slightly improved, but comprehension is most definitely not. The intent of a to_proc usage is not evident unless you know that this has been defined and is used. Otherwise you end up spending time scratching your head puzzling over what's going on.

Furthermore, although Tammer's example may have been loaded, it show the potential for abuse that to_proc can lead to. Something that could have been done in a single loop is now being done in two. This sort of chaining can be hazardous for performance.

IMHO, of course.

@Jeremy: I just removed those just to be complete - sure, there're a few places where speed doesn't matter (some in Railties and migrations).

The places I think it'd help is in ActionPack (esp. wrt filters) and possibly in AR. I'll pare back the patch :)

New patch attached. If I'd misunderstood something as being runtime code when it isn't, do exclude it if and when you're trying to apply the patch.

Okay, great :)

Btw, my results for the big array on 1.8.7 (native #to_proc):

Rehearsal ------------------------------------
 1465.33 KB allocated, 300005 objects
  0.080000   0.010000   0.090000 (  0.087565)
 1074.25 KB allocated, 300003 objects
  0.070000   0.000000   0.070000 (  0.079209)
--------------------------- total: 0.160000sec

       user     system      total        real
 1465.33 KB allocated, 300005 objects
  0.070000   0.000000   0.070000 (  0.076968)
 1074.25 KB allocated, 300003 objects
  0.050000   0.000000   0.050000 (  0.055148)

So 37% slower and 36% more memory. Same number of objects though; I was wrong about that!

Actually I was completely wrong for before_filters and after_filters in ActionController::Filters - I ran some profiling and realized that those are never called within Rails.

Do leave it out at your discretion.