Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Tammer Saleh June 25th, 2008 @ 02:42 PM

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

• You flagged this item as spam.
  

  Cheah Chu Yeow June 25th, 2008 @ 02:53 PM

  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.

• You flagged this item as spam.
  

  Tammer Saleh June 25th, 2008 @ 02:56 PM

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

• You flagged this item as spam.
  

  Eric Mill June 25th, 2008 @ 02:58 PM

  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.

• You flagged this item as spam.
  

  Tammer Saleh June 25th, 2008 @ 02:59 PM

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

• You flagged this item as spam.
  

  Eric Mill June 25th, 2008 @ 03:00 PM

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

• You flagged this item as spam.
  

  Jon Yurek June 25th, 2008 @ 03:02 PM

  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.

• You flagged this item as spam.
  

  RSL June 25th, 2008 @ 03:03 PM

  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.

• You flagged this item as spam.
  

  Cheah Chu Yeow June 25th, 2008 @ 03:09 PM

  @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 :)

• You flagged this item as spam.
  

  Cheah Chu Yeow June 25th, 2008 @ 03:11 PM

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

• You flagged this item as spam.
  

  Tammer Saleh June 25th, 2008 @ 03:13 PM

  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.

• You flagged this item as spam.
  

  Jon Yurek June 25th, 2008 @ 03:13 PM

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

• You flagged this item as spam.
  

  Neeraj June 25th, 2008 @ 04:08 PM

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

• You flagged this item as spam.
  

  Cheah Chu Yeow June 25th, 2008 @ 04:31 PM

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

• You flagged this item as spam.
  

  Cheah Chu Yeow June 25th, 2008 @ 04:35 PM

  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

• You flagged this item as spam.
  

  Ryan McGeary June 25th, 2008 @ 06:01 PM

  -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)

• You flagged this item as spam.
  

  James Herdman June 25th, 2008 @ 09:25 PM

  +1.

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

• You flagged this item as spam.
  

  Ryan McGeary June 25th, 2008 @ 09:41 PM

  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.

• You flagged this item as spam.
  

  James Herdman June 25th, 2008 @ 09:47 PM

  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 Kemper June 26th, 2008 @ 02:52 AM

  • State changed from “new” to “open”
  • Tag changed from “patch” to “patch, performance”
  • Milestone cleared.
  • Assigned user set to “Jeremy Kemper”

  Why remove it from all these places where speed doesn't matter, also?

  I actually removed one usage from Array#to_param today because it's used often enough to show up in profiling results. I think most of these other cases don't meet that standard.

  Also btw, Symbol#to_proc is native in Ruby 1.8.7 :) Speed is not a big issue, but object allocation is (literal syntax doesn't create a Proc per invocation).

  Could you pare back the patch to just remove usage that results in a significant runtime penalty?

• You flagged this item as spam.
  

  Cheah Chu Yeow June 26th, 2008 @ 03:01 AM

  @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 :)

• You flagged this item as spam.
  

  Cheah Chu Yeow June 26th, 2008 @ 03:24 AM

  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.

  • sym_to_proc_2.diff 9.2 KB
• 

  Jeremy Kemper June 26th, 2008 @ 03:28 AM

  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!

• You flagged this item as spam.
  

  Cheah Chu Yeow June 26th, 2008 @ 03:47 AM

  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.

• 

  Repository July 9th, 2008 @ 06:47 PM

  • State changed from “open” to “resolved”

  (from [ce4a1bb8538bd7cc5ee3cbf1156dc587482a7839]) Remove some Symbol#to_proc usage in runtime code. [#484 state:resolved]

  http://github.com/rails/rails/co...