On 13 Jan 2012, at 10:36, Dmitry Stogov wrote:
Recently we've found a huge problem in PHP traits implementation.
Thanks for taking care of it, but just to be explicit here: I pointed out the implementation details in the various discussions. I never made it 'a secret' that there is copying going on. I even tried to point out the potential for this kind of sharing.
See for instance: http://markmail.org/message/okhq2vp7h3yuegot
And the comment of the initial commit: http://svn.php.net/viewvc?view=revision&revision=298348
Sorry, but I am a bit annoyed that 'the community' does not care enough about reviewing such engine changes.
I got plenty of help from various people, but 'discovering such a huge problem' so late in the process points out certain problems.
Anyway, back to the technical details.
It performs copying of each op_array (with all opcodes!) for each method used from trait. This not only makes traits extremely slow and reduce effect of opcode caches, but also prohibits extensions from modifying op_array in some way, e.g. extending op_arrays with additional information in op_array.reserved* fields. As result some extensions (e.g. xdebug and some Zend extensions) will just crash on traits usage.
As I understood the copying was done only for proper handling of __CLASS__ constant in trait methods. I think it's too radical solution.
I've introduced ZEND_CLASS_NAME instruction instead and made op_arrays to share their opcodes (in the same way as inherited methods). The only difference that methods from traits may be renamed.
From the top of my head, it is the handling of __CLASS__ and the handling of static variables in methods. You did not mention that, is it taken care of explicitly, or do traits now share static state? The later would not be intended. Will check whether we got that documented with a test.
The patch is attached (it requires executor/scanner/parser regeneration)
I would like to commit it into 5.4 and trunk. Note that the patch makes binary compatibility break and can't be applied to 5.4.* after 5.4.0 release.
I know that applying it may delay the PHP 5.4.0 release, but it's better to fix the problem now.
I would be in favor of getting it into the process now, too.
Especially if it breaks binary compatibility.
I will take at a look at the patch later today.
Software Languages Lab
Vrije Universiteit Brussel
Pleinlaan 2 / B-1050 Brussels / Belgiumhttp://soft.vub.ac.be/~smarr
Phone: +32 2 629 2974
Fax: +32 2 629 3525