Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:70459
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.47 as permitted sender)
Message-ID: <529A831C.80401@gmail.com>
Date: Sun, 01 Dec 2013 00:30:20 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.1
MIME-Version: 1.0
To: Stas Malyshev <smalyshev@sugarcrm.com>, 
 PHP Internals <internals@lists.php.net>
References: <528CE64A.1020303@gmail.com> <CAH-PCH5RP1HRzC+qz94B862zrPt5O3x=U7MDvJJDPOJmAEkPjg@mail.gmail.com> <528EA006.2090400@gmail.com> <52993190.4010905@gmail.com> <52995F97.7000901@sugarcrm.com> <5299B124.3030809@gmail.com> <529A5947.5060701@sugarcrm.com>
In-Reply-To: <529A5947.5060701@sugarcrm.com>
Content-Type: multipart/alternative;
 boundary="------------050803030102060504010301"
Subject: Re: [PHP-DEV] Comments on non-unique naming convention for closures
From: ellison.terry@gmail.com (Terry Ellison)

--------------050803030102060504010301
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 30/11/13 21:31, Stas Malyshev wrote:
>> The mangled zend_function entry is never executed; only used as a copy
>> template.
> I see. That makes it better, but not completely, please see below.
>
>> This is really a separate thread, but in order to allow opcode caching,
>> the PHP compiler *must* generate the same oparray for a given source
>> sequence,
I will expand in this further in a separate thread.
>> so in fact even if the same function (...) {...} text sequence
>> occured multiple times in the same source file, then the oparrays would
>> be identical anyway, and using the same mangled zend_function entry is
>> fine.  However, if the 1/2^64 (or thereabouts) chance of a false
> This is true. However, if multiple scripts have the same function name,
> and are added to the cache separately in different time, by default the
> engine does not allow adding the same function twice (even if it has the
> same op-array).
Unfortunately this is not correct for closures.  The current code for 
closures overwrites the previous function silently -- that is without 
error.  OPcache has a different behaviour for a cached closure and 
instead ignores the new function (again silently) leading to a different 
behaviour for compiled scripts executed out of the opcode cache.  See 
https://bugs.php.net/bug.php?id=64291 for PHPTs which demonstrate this 
failure.
> We could code around it but why create problems for
> ourselves if we could easily avoid it? So I think it would be better to
> add something to the mix - like filename/lineno or counter - to ensure
> it is not the same. I'm not worried about the hash collisions, but
> copy-paste happens much more frequent than 1/2^64 hash collision.
Filename/lineno is not unique as the PHPT shows. Use of counter can be 
flawed due to race conditions if OPcaching is enabled.  This failure can 
occur in real app frameworks e.g. those which use a common compiler 
routine to generate custom templates.

>> collision is unacceptable, we could always embed the closure compile
>> count in the name as well, or the microsecond compile time.
> I would avoid using time, as getting time is usually a system call and
> system calls are slow. Counter or filename/lineno or anything that makes
> the hash different would be fine.

+1 on avoiding time-related system calls -- even though these are pretty 
optimized on current Linux kernels -- however this is why we suggested a 
content based hash.  Any alternatives that I can think of require a 
materially larger larger patch involving more source changes.

Regards Terry

--------------050803030102060504010301--