Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:70544
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain zend.com from 209.85.212.175 cause and error)
MIME-Version: 1.0
In-Reply-To: <52A0B513.2010108@gmail.com>
References: <528CE64A.1020303@gmail.com>
	<52A0B513.2010108@gmail.com>
Date: Mon, 9 Dec 2013 11:39:05 +0400
Message-ID: <CA+9eiLtoa6xewuFNGr50Nw+MfbKu4FaPrViVhDXw2UQyS28g_A@mail.gmail.com>
To: Terry Ellison <ellison.terry@gmail.com>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=f46d044280b431725104ed15176e
Subject: Re: Comments on non-unique naming convention for closures
From: dmitry@zend.com (Dmitry Stogov)

--f46d044280b431725104ed15176e
Content-Type: text/plain; charset=UTF-8

hi Terry,

Please share the new PHPT tests or better attach them to bug report.

(D) looks reasonable simple. I thinks it must be a good enough solution.
For ZTS we can use thread_id in addition to PID (or instead of it). Also we
may use a simple thread specific counter instead of microtime -
CG(key_counter).

Thanks. Dmitry.



On Thu, Dec 5, 2013 at 9:17 PM, Terry Ellison <ellison.terry@gmail.com>wrote:

>  Dmitry,
>
> The underlying issue here is the assumption that
> build_runtime_defined_function_key() in zend_compile.c generates unique
> keys.  I've realised that this impacts not only closures but also
> late-bound functions and classes.  I've now produced PHPTs which
> demonstrates the failure in all three cases.  Whilst hashing a closure
> source text has an acceptable compile-time overhead,  the case for classes
> (with their larger source lengths) is more of a concern.
>
> I am now proposing an alternative hash based on:
>
> A.  The class / function name (as per current)
> B.  The reference source file (as per current)
> C.  The address of the buffer (as per current)
> D.  The PID and microtime at the request start.
> E.  A running count of the function/classed compiled during the request.
>
> (A-C) were assumed unique but, as #64291 and #65915 show, are not.
>
> (D) should be sufficient to prevent race conditions across different
> OPcached threads / processes, but this has the advantage that it only needs
> to be computed once per request.
>
> (E) guarantees uniqueness within thread.
>
> The only weakness of D and E alone is a possible race on ZTS builds where
> two clashing threads start at the same microsecond, which (C) will split.
> If this is considered too unwieldy then we could always drop B as C-E
> guarantee uniqueness, and B is never exposed to the programmer anyway
> (Xdebug and the error logger use the appropriate fields in the
> zend_function / zend_class record and not its key in the corresponding EG
> table.
>
> This also has the advantage that the main change can now be localised to
> build_runtime_defined_function_key().
>
> //Terry
>
>  The following bugs relate to this discussion:
>
>     #64291 Indeterminate GC of evaled lambda function resources
>     #65915 Inconsistent results with require return value
>
> I don't want to discuss these or the specific fixes here, since I can work
> up a fix and discuss them in the bugrep.  However,
> my one-sentence Q is "should we replace the naming convention for closures
> with a truly unique one?"  ...
>
>
>

--f46d044280b431725104ed15176e--