Hi Dmitry: 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. Thanks Stefan -- Stefan Marr Software Languages Lab Vrije Universiteit Brussel Pleinlaan 2 / B-1050 Brussels / Belgium http://soft.vub.ac.be/~smarr Phone: +32 2 629 2974 Fax: +32 2 629 3525 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
