Re: com php-src: Bug #63699: performance improvements for varios ext/date functions: NEWS ext/date/php_date.c ext/date/php_date.h

[CC'ing internals.] On 01/16/2013 03:15 PM, Paul Taulborg wrote:

On Wed, Jan 16, 2013 at 4:21 PM, Nuno Lopes <nlopess@php.net> wrote:

On Thu, Jan 10, 2013 at 4:59 PM, Nuno Lopes <nlopess@php.net> wrote:

On 01/07/2013 07:27 AM, Paul Taulborg wrote:

I would love to write this patch, I'm all in favor of simplifying this. :) We should probably get with derick (cc'd) on this as well, to make sure something else obvious isn't being missed. Let me know how you want me to proceed. Thanks :)

Paul, For small changes submit bugs (and/or github pull requests) like you did before. For bigger changes, start an RFC at https://wiki.php.net/rfc This will help focus the change, and provides a source for any doc updates. Since it is really just you and Derick looking at this area, be prepared to be a leader. Don't forget to CC internals. Chris PS Note that Derick tends to ignore top-posted email.

Bug/patch submitted: https://bugs.php.net/bug.php?id=63941 Will post on internals momentarily. Turns out removing default_timezone entirely won't work due to the callback for ini_set, but this isn't a major problem. We could probably remove it entirely with a future patch, but it would require some other internal variable somewhere along the line anyway.

I have another suggestion that avoids BC break. Pseudo-code: guess_timezone():

    if (timezone) return timezone
    if (default_timezone) return default_timezone
    return "UTC

OnUpdate ini option:

    if (valid) {
        free(default_timezone)
        default_timezone = new
    }

PHP's date_default_timezone_set():

    if (valid) {
        free(timezone)
        timezone = new
    } else {
        timezone = NULL
    }

I belive this causes no major BC break and enables efficient code. Note that according to the manual, date_default_timezone_set() takes precedence over the ini setting, and therefore we cannot set the 'timezone' var in the ini handler (as I previously wrongly suggested). Nuno

Nuno, Unless I'm misunderstanding or missing something here, this is already what I did in the patch submitted 4 days ago: https://bugs.php.net/bug.php?id=63941 I'm still waiting for feedback and for the patch to be applied. Thanks

Sorry for the delay, but I've been quite busy.. So, to answer your question: I don't think your patch implements the pseudo-code I suggested above. In particular your patch change the documented behaviour for intermixed ini_set/date_set_default_tz calls. Nuno

I would recommend this be changed, and it would not conflict with the documentation. First, the ini_set() does not say "this will not override date_set_default_timezone()", even though that is the functionality. A user could easily presume (wrongly), that both do the same thing (and they should, for consistency). I see no logical reason to have two values in the core, that do the same thing. For instance, I could use date_set_default_timezone() in one spot, then later ini_set, expecting it to override, but it doesn't. This is an extra nuance for no gain. If the argument is "but I might want to revert back to the ini value!", well, you can't currently without a literal ini_get() and then another date_set_default_timezone() call. This is no different than what my patch does, you would have to grab the original value with ini_get, and then set it via EITHER method. This simplifies the internal code handling and makes it more consistent and makes the code a bit easier to follow with regards to this particular subset of the date section. If we're going to get in here and start optimizing code, we should do so, not continue to throw band-aids on that do nothing in terms of performance gains or code readability/manageability. That is the reason I tackled this head-on in the way I did, because having both values internally checked is not necessary. I cannot see any reason to have two values internally to represent essentially the same thing. If there is something I'm missing here regarding that, please let me know. Thanks

-- christopher.jones@oracle.com http://twitter.com/ghrd Newly updated, free PHP & Oracle book: http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html