Make WordPress Core

Opened 11 years ago

Closed 6 years ago

Last modified 6 years ago

#23132 closed enhancement (maybelater)

wp_timezone_override_offset() has noticeable performance hit

Reported by: rarst's profile Rarst Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.5
Component: Date/Time Keywords: has-patch needs-refresh
Focuses: Cc:

Description

While wandering through XHProf generated callgraph of my test stack I came upon wp_timezone_override_offset() eating 3.4% of runtime (73 calls). I do not expect noticeable resource consumption from such minor configuration-related function.

The obvious performance aspect is that it creates and discards two objects (DateTime and DateTimeZone - latter seemingly more heavy) per each call, while calculating same result each run.

The less obvious aspect is that the only use of this function is to pre-filter gmt_offset option, and it essentially hijacks option cache for it.

Possible solutions:

  • calculate result once per run and cache in static variable inside function (straightforward, but retains multiple function call)
  • filter gmt_offset on set rather than get (more involved)
  • scrap the extra function and/or option altogether? is there still practical need for having two options for timezone information?

Attachments (1)

23132.patch (1.1 KB) - added by SergeyBiryukov 11 years ago.

Download all attachments as: .zip

Change History (14)

#1 @Rarst
11 years ago

  • Cc contact@… added
  • Summary changed from wp_timezone_override_offset() has notiecable performance hit to wp_timezone_override_offset() has noticeable performance hit

#2 @Otto42
11 years ago

  • Cc Otto42 added

#3 @SergeyBiryukov
11 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.6

23132.patch implements the first option.

#4 @TJNowell
11 years ago

  • Cc tom@… added

#5 @nacin
11 years ago

Using a static variable cache that wraps a get_option() call means that this function won't work for switch_to_blog(). It'd need to be keyed by the option value (which I think is okay).

#6 follow-up: @Otto42
11 years ago

Long term, this should be gutted and rewritten to eliminate gmt_offset entirely. This stuff is a holdover from when PHP 4 was still supported. The new stuff added in 2.8 was done this way to support PHP 4 as well and keep the date/time consistent across the systems. For example, the default timezone is set to GMT no matter what you select in order to keep this PHP 4/5 code working identically.

The right way:

  • Kill gmt_offset entirely.
  • Gut the various timezone calculation code to put it back on PHP 5's code to do the grunt work.
  • Most of this can be handled by using the native gmdate/date functions correctly with date_default_timezone_set.

Nacin's right that switch_to_blog would have to change timezones, but it would be able to do so by calling date_default_timezone_set to do so instead of using static cached variables.

We're PHP 5 native now, we should take advantage of this. It is a big job though.

#7 in reply to: ↑ 6 @nacin
11 years ago

Replying to Otto42:

We're PHP 5 native now, we should take advantage of this. It is a big job though.

It's also not realistic to do, as it'll be a pretty big backwards compatibility hit.

#8 @Otto42
11 years ago

I don't think it'll be as bad as you think, really. But again, it is long term.

#9 @ocean90
11 years ago

  • Milestone changed from 3.6 to Future Release

#10 @chriscct7
9 years ago

  • Keywords needs-refresh added
  • Severity changed from minor to normal

#11 @Rarst
6 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

At this point burning effort trying to address this isn't worth the effort.

Though old offset-centric timezone handling definitely needs to be burned with fire at some point, as it continues to fuel long list of ambiguities and bugs all over Date/Time component.

#12 @Otto42
6 years ago

  • Resolution changed from wontfix to maybelater

I still believe this is possible, but yes, some plugins may be gutted by doing this the right way. I'm kinda okay with a bit of breakage on this one. I think enough time has passed to reduce the impact.

Let's not kill the hope just yet

#13 @ocean90
6 years ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.