Re: Comments on non-unique naming convention for closures

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