Rails Ruby on Rails

Comments and changes to this ticket

• 

  Pratik May 15th, 2008 @ 11:38 AM

  • Assigned user set to “Geoff Buesing”

  1. I don't like @ALL_CAPS var names.

  2. What are the performance benefits of this ?

• You flagged this item as spam.
  

  Martin Eisenhardt May 15th, 2008 @ 12:02 PM

  Hi Pratik, thank you for your review and comment.

  Ad 1.) OK, I can understand that this does not meet your personal coding standard, or the coding standard of the Rails community. I use REGION_ZONES in reference to the US_ZONES regex; the US_ZONES were already in the code, so I just went along.

  I am more than willing to change that and resubmit a changed patch. Just say the word! :-D

  Ad 2.) When you call the method us_zones before the patch, it calls all.find_all( |z| z.name =~ US_ZONES) which will go through all time zones and match them against the regex US_ZONES. In an web app which uses time zones a lot this will account for several hundreds (or even thousands) regex operations for a single page view.

  After applying my patch, the regex is only evaluated once (upon the first invocation of the us_zones method) and the result stored away in a class attribute named @US_ZONES (which I will have to rename, see above point 1). Subsequent invocations of us_zones will not have to evaluate the regex but use the cached result immediately.

  Thus, the computational costs for us_zones is decreased sharply.

  Please tell me if I see this wrong - this is not unlikely, and I am always eager to learn.

• 

  Geoff Buesing May 15th, 2008 @ 02:32 PM

  If we're going to preload us_zones, we need to make it thread-safe -- see TimeZone.all for an example of how to do this, if you're interested in updating this patch.

• You flagged this item as spam.
  

  Martin Eisenhardt May 15th, 2008 @ 03:05 PM

  Thanks for the pointer, Geoff, I will definitely have a look (and a go) at it.

• 

  Geoff Buesing May 15th, 2008 @ 02:45 PM

  It's probably also worth exploring, is the current method of matching against a regex really all that slow? Maybe a quick benchmark would help justify your effort here.

• You flagged this item as spam.
  

  Martin Eisenhardt May 15th, 2008 @ 02:55 PM

  While regular expressions are by far the fastest way to search for patterns, they nevertheless are slow compared to using pre-computed values.

  I really do like the idea of having convenience methods for "regional" or "continental" time zones. I just want to make sure that it is done "right" (no offence meant) and no inefficient code makes it to a release.

  Sure, for most applications, the benefits will be small. But consider a web application that makes heavy use of time zones and calls us_zones (or whatever) several times for a certain view - then the benefit of re-using pre-computed results becomes clear.

  From the top of my head, I can think of several applications that will benefit: f.e. a calendar application for a globally diversified audience (every time you enter an event, you get to choose which time zone the event will be for).

  I will provide an updated patch in a timely manner and will hope for the best :-D

  BTW: Making the patch thread-safe in the way you mentioned means pre-computing at the time of class loading - did I get this right?

  BTW2: Why did my last comment get marked as spam? Did I use any keywords, or was it just to short?

• 

  josh May 15th, 2008 @ 03:15 PM

  I'd compute on load them right under "ZONES" and put them in a frozen constant "US_ZONES".

• 

  Repository May 15th, 2008 @ 03:23 PM

  • State changed from “new” to “resolved”

  (from [fc02eabf296d6edb74a95174c7322293a54c9492]) Precompute TimeZone.us_zones [#199 state:resolved]

  Signed-off-by: Joshua Peek

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

• You flagged this item as spam.
  

  Martin Eisenhardt May 15th, 2008 @ 03:34 PM

  Hi Joshua,

  thank you for the advice. It is the same I had in mind. Together with this comment, I upload an updated patch the pre-computes a US_ZONES constant, sorts and freezes it.

  Thanks to Geoff for pointing out the shortcomings of my first patch.

  BTW: Forget about views calling us_zones several times - this should be clearly cached in the view or the corresponding controller action. But still, consider multiple views calling us_zones on a web app with time zone support.

  • 0001-Pre-compute-collection-for-us_zones-convenience-meth.patch 1.9 KB