Hi Stefan,
On 01/13/2012 02:13 PM, Stefan Marr wrote:
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.
Sorry, I didn't look into traits implementation before.
Just was pointed into the problem.
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.
static variables are separated in the same way as for inherited methods
(it works out of the box).
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.
Yes, please do. All tests are passed, but I may miss something.
Thanks. Dmitry.
Thanks
Stefan
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
