This project is archived and is in readonly mode.

#199 ✓resolved
Martin Eisenhardt

Use lazy evalution and caching for us_zones in TimeZones

Reported by Martin Eisenhardt | May 14th, 2008 @ 07:45 PM

The us_zones convenience method in TimeZone generates the list of TimeZone objects in the U.S. every time it is called. This is inefficient, especially in applications that use time zones very heavily.

The patch attached to this ticket changes us_zones so that it uses lazy evaluation and result caching. It generates the collection once (upon the first invocation) and stores the collection in a class attribute. Subsequent invocations use the cached collection and are much faster.

Comments and changes to this ticket

  • Pratik

    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 ?

  • Martin Eisenhardt

    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

    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.

  • Martin Eisenhardt

    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

    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.

  • Martin Eisenhardt

    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

    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

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

  • Martin Eisenhardt

    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.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Referenced by

Pages