Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:34715
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain redhat.com designates 66.187.233.31 as permitted sender)
Date: Fri, 11 Jan 2008 12:59:17 +0000
To: PHP Developers Mailing List <internals@lists.php.net>
Message-ID: <20080111125917.GA17057@redhat.com>
Mail-Followup-To: PHP Developers Mailing List <internals@lists.php.net>
References: <20080109141541.GA14571@redhat.com> <alpine.DEB.0.98.0801092102560.4679@kossu.ez.no> <20080110120535.GA27518@redhat.com> <alpine.DEB.0.98.0801110949320.4588@kossu.ez.no>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <alpine.DEB.0.98.0801110949320.4588@kossu.ez.no>
User-Agent: Mutt/1.5.17 (2007-11-01)
Organization: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom.
  Registered in UK and Wales under Company Registration No. 03798903
  Directors: Michael Cunningham (USA), Brendan Lane (Ireland), Matt Parson (USA), Charlie Peters (USA)
Subject: Re: [PHP-DEV] [PATCH] date/timelib: use system timezone database
From: jorton@redhat.com (Joe Orton)

On Fri, Jan 11, 2008 at 09:59:46AM +0100, Derick Rethans wrote:
> On Thu, 10 Jan 2008, Joe Orton wrote:
> > On Wed, Jan 09, 2008 at 09:03:02PM +0100, Derick Rethans wrote:
> > > It's not used (anymore). However, there is a PHP function 
> > > timezone_identifiers_list that uses the index information to enumerate 
> > > all entries. Your patch prevents this function from working as both 
> > > struct elements index_size and index are set to 0/null.
> > 
> > OK, fixed in attached patch.
> 
> This is a fix that scans the whole directory - which is not really fast. 
> I find this an unacceptable action to cripple to PHP.

The performance cost is once per process invocation.  To quantify it, I 
timed a HEAD sapi/cli/php build with a fairly minimal statically-linked 
module set, running a script which does:

<? date_default_timezone_set('Antarctica/South_Pole'); ?>

...does that seem like a reasonable benchmark?  The average run times 
reported by time(1) were (elapsed real time):

default:   0m0.018s
system tz: 0m0.021s

so a ~0.003s hit seems OK to me.  I would hazard a guess that this 
performance hit is little different to that of building a few extensions 
as DSOs, or to using any other extension which has high init overhead 
(openssl or snmp spring to mind).  Would it help if this filesystem 
trawl was hooked into the date extension's MINIT function?

(For perspective; the /usr/bin/php from the Fedora 5.2.5 package with a 
much larger set of extensions built as DSOs takes an average 0m0.068s to 
run the same script on the same system)

> > > Not having the version creates another problem, as the PECL timezonedb 
> > > extension will then *always* override the builtin database. The 
> > > extension uses the version number to see if the extension provides a 
> > > later rules set. With your patch, this extra check will not work 
> > > correctly anymore if PHP's built-in version is newer (which would happen 
> > > if PHP got upgraded).
> > 
> > This seems fine, even desirable.  Anybody who configures PHP 
> > --with-system-tzdata *and* installs the PECL timezonedb always gets the 
> > data provided by the timezonedb; the risk of stale timezone data is 
> > little different to the current risk.
> 
> However, how does the user know he can actually get a newer ruleset if 
> he does not have any way to see which database is currently active? Not 
> being able to see which version is currently active is yet another 
> degeneration of PHP's timezone functionality.

I'm not sure I understand the question.  For users of PECL timezonedb, 
the situation is no different to the status quo.  Otherwise; it's kind 
of the whole point is that they don't have to care -- just as they don't 
have to care for all the other apps on the system; they just have to 
update the system tzdata package.

> Ok, good - i wasn't totally sure here. However, like I mentioned in my 
> other mail, the TZif format is not the best way *for PHP* to access the 
> rulesets, and there is the possibility that while building the data the 
> format might change to make it easier to access rulesets for PHP. In 
> that case you can't simply switch the parsing to the system provided 
> database because the format will be different. This is at the moment 
> hypotetical though.

That doesn't sound like it would be an intractable problem even so; 
if&when such a change happened, the current read_* functions could be 
retained to parse the system's TZif-format files; the raw data is there 
either way, regardless of what transformation is needed.

(BTW I noticed that the timezonedb*_builtin arrays could be marked 
static in timezonedb.h; it doesn't look like they need to be exposed?)

Regards,

joe