Re: Comments on non-unique naming convention for closures
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?" ...
>
>
>
Thread (14 messages)