FAQ
Hi,

Recently we've found a huge problem in PHP traits implementation.

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.

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.

Thanks. Dmitry.

Search Discussions

  • Sebastian Bergmann at Jan 13, 2012 at 9:59 am

    On 01/13/2012 10:36 AM, Dmitry Stogov wrote:
    It performs copying of each op_array (with all opcodes!) for each method
    used from trait.
    That is bad, indeed.
    As result some extensions (e.g. xdebug and some Zend extensions) will
    just crash on traits usage.
    I have used PHP 5.4-dev, Xdebug 2.2-dev, and traits without problems.
    Xdebug's code coverage functionality "just works" with PHPUnit.

    --
    Sebastian Bergmann Co-Founder and Principal Consultant
    http://sebastian-bergmann.de/ http://thePHP.cc/
  • Stefan Marr at Jan 13, 2012 at 10:13 am
    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
  • Stefan Marr at Jan 13, 2012 at 10:33 am
    Hi:
    On 13 Jan 2012, at 11:13, Stefan Marr wrote:

    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.
    Seems to work fine, is checked with a very basic test in PHP_5_4/Zend/tests/traits/language013.phpt

    Best regards
    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
  • Dmitry Stogov at Jan 13, 2012 at 10:40 am
    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
  • Lester Caine at Jan 13, 2012 at 10:51 am
    (Several cc's removed)

    Stefan Marr wrote:
    Sorry, but I am a bit annoyed that 'the community' does not care enough about reviewing such engine changes.
    I feel that it is worth pointing out that there still needs to be a case made
    for the general use of traits. Currently I see no advantage to using them so it
    is not an area I have even looked at. If developments like this slow down the
    general performance of PHP, then they need to be able to be disabled as well,
    and at present my own existing code does seem to be running slower on PHP5.4
    over 5.3 (and also 5.2). The majority of us ARE simple testing that existing
    code works and working out how to fix stuff that gets broken, so spending time
    on 'new features' tends to be a lot lower down the todo list for many of us?

    With regards to 'traits' ... why would I not put this code into a base class and
    simply extend from that. To my mind this is a little like 'goto', a bodge
    because designing the class structure is too difficult?

    --
    Lester Caine - G8HFL
    -----------------------------
    Contact - http://lsces.co.uk/wiki/?page=contact
    L.S.Caine Electronic Services - http://lsces.co.uk
    EnquirySolve - http://enquirysolve.com/
    Model Engineers Digital Workshop - http://medw.co.uk//
    Firebird - http://www.firebirdsql.org/index.php
  • Ferenc Kovacs at Jan 13, 2012 at 10:59 am

    On Fri, Jan 13, 2012 at 11:51 AM, Lester Caine wrote:

    (Several cc's removed)


    Stefan Marr wrote:
    Sorry, but I am a bit annoyed that 'the community' does not care enough
    about reviewing such engine changes.
    I feel that it is worth pointing out that there still needs to be a case
    made for the general use of traits. Currently I see no advantage to using
    them so it is not an area I have even looked at. If developments like this
    slow down the general performance of PHP, then they need to be able to be
    disabled as well, and at present my own existing code does seem to be
    running slower on PHP5.4 over 5.3 (and also 5.2). The majority of us ARE
    simple testing that existing code works and working out how to fix stuff
    that gets broken, so spending time on 'new features' tends to be a lot
    lower down the todo list for many of us?

    With regards to 'traits' ... why would I not put this code into a base
    class and simply extend from that. To my mind this is a little like 'goto',
    a bodge because designing the class structure is too difficult?

    could you please open a separate thread for that?
    I think it would be a bad idea hijacking this thread to discuss the usage
    of traits.


    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu
  • Sebastian Bergmann at Jan 13, 2012 at 11:08 am

    On 01/13/2012 11:51 AM, Lester Caine wrote:
    If developments like this slow down the general performance of PHP,
    then they need to be able to be disabled as well
    While we're at it we can also add configuration directives to disable
    namespaces, classes, functions, and every syntax features that
    facilitates the separation of concerns and the decomposition of
    software.

    Of course we also need to add a function that allows developers to check
    which syntax features are enabled in the PHP interpreter that is
    executing their code ...

    </sarcasm>

    --
    Sebastian Bergmann Co-Founder and Principal Consultant
    http://sebastian-bergmann.de/ http://thePHP.cc/
  • Sebastian Bergmann at Jan 13, 2012 at 11:10 am

    On 01/13/2012 11:51 AM, Lester Caine wrote:
    why would I not put this code into a base class and simply extend
    from that.
    Could you please refrain from asking questions such as this BEFORE you
    even bothered to research the topic? Thank you very much.

    --
    Sebastian Bergmann Co-Founder and Principal Consultant
    http://sebastian-bergmann.de/ http://thePHP.cc/
  • Stefan Marr at Jan 13, 2012 at 11:35 am
    Hi Lester:
    On 13 Jan 2012, at 11:51, Lester Caine wrote:

    If developments like this slow down the general performance of PHP
    There shouldn't be any significant performance impact of the traits addition to the engine.
    Traits only cost you if you use them, and their cost is restricted to compilation time.
    With an adapted opcode cache, and with Dmitry's optimization even that cost should be gone.

    There is some general performance impact, and that comes from additional fields in the class struct. Thus, every class takes slightly more memory.

    Beside that, I do not recall performance relevant changes from the top of my head.

    Best regards
    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
  • Pierre Joye at Jan 13, 2012 at 12:25 pm
    adding david too to the loop.
    On Fri, Jan 13, 2012 at 10:36 AM, Dmitry Stogov wrote:
    Hi,

    Recently we've found a huge problem in PHP traits implementation.

    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.

    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.

    Thanks. Dmitry.

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Pierre

    @pierrejoye | http://blog.thepimp.net | http://www.libgd.org
  • Stas Malyshev at Jan 13, 2012 at 6:53 pm
    Hi!
    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.
    I've done some checks and discovered __CLASS__ behaves somewhat
    strangely with traits. COnsider this code:

    <?
    trait foo {
    public $bar = __CLASS__;
    function bar() { var_dump($this->bar); }
    }

    class foofoo {
    use foo;
    }

    class foofoo2 {
    use foo;
    }

    $a = new foofoo();
    $a->bar();
    $a = new foofoo2();
    $a->bar();
    echo "OK!";

    Before the patch, it prints $this->bar as NULL. After the patch, I get:

    Fatal error: Invalid binding type in - on line 5

    Both behaviors seem to be wrong, as __CLASS__ works as constant in
    regular classes. Looks like even before the patch __CLASS__ handling
    wasn't correct, but patch also seems to break something.
    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227
  • Stefan Marr at Jan 14, 2012 at 12:24 am
    Hi Stas:
    On 13 Jan 2012, at 19:53, Stas Malyshev wrote:

    I've done some checks and discovered __CLASS__ behaves somewhat strangely with traits. COnsider this code:

    <?
    trait foo {
    public $bar = __CLASS__;
    function bar() { var_dump($this->bar); }
    }

    class foofoo {
    use foo;
    }

    class foofoo2 {
    use foo;
    }

    $a = new foofoo();
    $a->bar();
    $a = new foofoo2();
    $a->bar();
    echo "OK!";

    Before the patch, it prints $this->bar as NULL.
    Yes looks like another hole in the things I covered :-/

    The attached test defines the intended behavior. Like you said, __CLASS__ is expected to give the using class. My testing covered so far only the handling of __CLASS__ in methods.
  • Stefan Marr at Jan 14, 2012 at 10:22 pm
    Hi Dmitry:
    On 14 Jan 2012, at 01:24, Stefan Marr wrote:
    On 13 Jan 2012, at 19:53, Stas Malyshev wrote:

    trait foo {
    public $bar = __CLASS__;
    }
    Breakpoint 3, zend_do_early_binding () at zend_compile.c:4602
    4602 zend_error(E_COMPILE_ERROR, "Invalid binding type");
    (gdb) p opline->opcode
    $1 = 159 '?' // == ZEND_CLASS_NAME
    What would be the best approach to handle the case of property definitions?

    I was thinking that we might want to handle that in zend_do_early_binding but usually these ops get changed to NOPs after they have been evaluated.
    And that's not what we need when the op_arrays are shared, I think.

    However, the current implementation of ZEND_CLASS_NAME you proposed relies on EG(scope) which is not set for the case below.

    I might be able to look into it further on Monday, but not earlier, unfortunately.

    Best regards
    Stefan


    trait Foo {
    public $c = __CLASS__;
    }

    class Bar {
    use Foo;
    }

    $bar = new Bar();
    var_dump($bar);


    --
    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
  • Stas Malyshev at Jan 15, 2012 at 2:56 am
    Hi!
    What would be the best approach to handle the case of property
    definitions?
    I don't think this opcode should be there in this case. Maybe we should
    use IS_CONSTANT in this case and handle it when constants are updated as
    a special case?
    Alternatively, if we find no solution for it we may just ban __CLASS__
    in trait properties for now until we find a solution... We need some
    resolution for it fast, if we want to release 5.4.0 anytime soon.
    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227
  • Dmitry Stogov at Jan 16, 2012 at 12:03 pm
    Hi,

    The updated patch is attached.

    Now especially for traits it delays resolution of special constant
    __CONST__ til run-time.

    Thanks. Dmitry.
    On 01/15/2012 06:56 AM, Stas Malyshev wrote:
    Hi!
    What would be the best approach to handle the case of property
    definitions?
    I don't think this opcode should be there in this case. Maybe we should
    use IS_CONSTANT in this case and handle it when constants are updated as
    a special case?
    Alternatively, if we find no solution for it we may just ban __CLASS__
    in trait properties for now until we find a solution... We need some
    resolution for it fast, if we want to release 5.4.0 anytime soon.
  • Stas Malyshev at Jan 17, 2012 at 5:42 am
    Hi!
    Now especially for traits it delays resolution of special constant
    __CONST__ til run-time.
    Looks good, I don't see any problems with it so far. If nobody else
    does, let's commit it,
    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227
  • Stefan Marr at Jan 17, 2012 at 8:32 am

    On 17 Jan 2012, at 06:42, Stas Malyshev wrote:

    Hi!
    Now especially for traits it delays resolution of special constant
    __CONST__ til run-time.
    Looks good, I don't see any problems with it so far. If nobody else does, let's commit it,
    I only had a chance to have a very brief look, but the code seems to look good and the tests are there and work, too.

    Thanks Dmitry!

    Best regards
    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
  • Stefan Marr at Jan 15, 2012 at 9:58 am
    Hi Dmitry:

    After Stas nice and encouraging words, I had another look.
    But to me it seems that I am not able to get to any of the relevant bits via the scoping infos in the globals. And I do not see how we could encode something into ZEND_CLASS_NAME.

    But I don't have a good overview of the engine, so I probably just miss the obvious.

    And Stas, to assuage your pain:
    Yes, in the worst case, Dmitry's patch would need a null check on EG(scope) and we make it return NULL for those properties.

    But I bet, someone with a little more insight will have a solution.

    Best regards
    Stefan
    On 14 Jan 2012, at 23:22, Stefan Marr wrote:

    I was thinking that we might want to handle that in zend_do_early_binding but usually these ops get changed to NOPs after they have been evaluated.
    And that's not what we need when the op_arrays are shared, I think.

    However, the current implementation of ZEND_CLASS_NAME you proposed relies on EG(scope) which is not set for the case below.

    I might be able to look into it further on Monday, but not earlier, unfortunately.

    Best regards
    Stefan


    trait Foo {
    public $c = __CLASS__;
    }

    class Bar {
    use Foo;
    }

    $bar = new Bar();
    var_dump($bar);


    --
    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
    --
    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
  • Dmitry Stogov at Jan 16, 2012 at 6:46 am
    Hi Stefan,

    It couldn't be done using early binding.
    It should be done delaying __CLASS__ constant resolution till run-time.
    Doing this we can even substitute he proposed ZEND_CLASS_NAME with
    ZEND_FETCH_CONSTANT(__CLASS__). For default values we'll have just store
    the corresponding constant.

    I'll try to prepare a patch myself.

    Thanks. Dmitry.
    On 01/15/2012 02:22 AM, Stefan Marr wrote:
    Hi Dmitry:
    On 14 Jan 2012, at 01:24, Stefan Marr wrote:
    On 13 Jan 2012, at 19:53, Stas Malyshev wrote:

    trait foo {
    public $bar = __CLASS__;
    }
    Breakpoint 3, zend_do_early_binding () at zend_compile.c:4602
    4602 zend_error(E_COMPILE_ERROR, "Invalid binding type");
    (gdb) p opline->opcode
    $1 = 159 '?' // == ZEND_CLASS_NAME
    What would be the best approach to handle the case of property definitions?

    I was thinking that we might want to handle that in zend_do_early_binding but usually these ops get changed to NOPs after they have been evaluated.
    And that's not what we need when the op_arrays are shared, I think.

    However, the current implementation of ZEND_CLASS_NAME you proposed relies on EG(scope) which is not set for the case below.

    I might be able to look into it further on Monday, but not earlier, unfortunately.

    Best regards
    Stefan


    trait Foo {
    public $c = __CLASS__;
    }

    class Bar {
    use Foo;
    }

    $bar = new Bar();
    var_dump($bar);
  • Dmitry Stogov at Jan 16, 2012 at 6:26 am
    Stas,

    Thank you for looking into it and finding out a new problem :)
    I'll take a look into it and will try to fix.

    Dmitry.
    On 01/13/2012 10:53 PM, Stas Malyshev wrote:
    Hi!
    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.
    I've done some checks and discovered __CLASS__ behaves somewhat
    strangely with traits. COnsider this code:

    <?
    trait foo {
    public $bar = __CLASS__;
    function bar() { var_dump($this->bar); }
    }

    class foofoo {
    use foo;
    }

    class foofoo2 {
    use foo;
    }

    $a = new foofoo();
    $a->bar();
    $a = new foofoo2();
    $a->bar();
    echo "OK!";

    Before the patch, it prints $this->bar as NULL. After the patch, I get:

    Fatal error: Invalid binding type in - on line 5

    Both behaviors seem to be wrong, as __CLASS__ works as constant in
    regular classes. Looks like even before the patch __CLASS__ handling
    wasn't correct, but patch also seems to break something.
  • Stefan Marr at Jan 13, 2012 at 11:34 pm
    Hi Dmitry:
    On 13 Jan 2012, at 10:36, Dmitry Stogov wrote:

    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.

    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 looked at the patch, looks good as far as I can see.

    The comment in zend_traits_copy_function (has a typo 'destroied') makes me wonder what it means exactly. Does it mean we got a memory leak with regard to trait aliases?


    And, I would have one stylistical request: could you separate out the optimizations you do in zend_language_scanner.l? I think it would be better to have the proper use of interned strings committed on their own. Especially, since they regard not only trait-related functionality.

    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
  • Dmitry Stogov at Jan 16, 2012 at 6:33 am

    On 01/14/2012 03:34 AM, Stefan Marr wrote:
    Hi Dmitry:
    On 13 Jan 2012, at 10:36, Dmitry Stogov wrote:

    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.

    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 looked at the patch, looks good as far as I can see.

    The comment in zend_traits_copy_function (has a typo 'destroied') makes me wonder what it means exactly. Does it mean we got a memory leak with regard to trait aliases?
    No. We might get a memory leak or double free in case trait is destroyed
    before the class that used it and renames some of its methods.
    And, I would have one stylistical request: could you separate out the optimizations you do in zend_language_scanner.l? I think it would be better to have the proper use of interned strings committed on their own. Especially, since they regard not only trait-related functionality.
    OK.

    Thanks. Dmitry.
    Thanks
    Stefan
  • Yoram bar haim at Jan 16, 2012 at 10:15 am
    If we want __CLASS__ to be resolved at runtime (at list in case of trait),
    then what about __FILE__ and __LINE__ ? should they be resolved at compile
    time and reflect the original code in the trait or should they reffer to the
    using class (which is a problem for the __LINE__ ...) ?
    On Monday, January 16, 2012 08:33:04 AM Dmitry Stogov wrote:
    On 01/14/2012 03:34 AM, Stefan Marr wrote:
    Hi Dmitry:
    On 13 Jan 2012, at 10:36, Dmitry Stogov wrote:
    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.

    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 looked at the patch, looks good as far as I can see.

    The comment in zend_traits_copy_function (has a typo 'destroied') makes
    me wonder what it means exactly. Does it mean we got a memory leak with
    regard to trait aliases?
    No. We might get a memory leak or double free in case trait is destroyed
    before the class that used it and renames some of its methods.
    And, I would have one stylistical request: could you separate out the
    optimizations you do in zend_language_scanner.l? I think it would be
    better to have the proper use of interned strings committed on their
    own. Especially, since they regard not only trait-related functionality.
    OK.

    Thanks. Dmitry.
    Thanks
    Stefan
  • Stefan Marr at Jan 16, 2012 at 10:25 am
    Hi:
    On 16 Jan 2012, at 11:15, yoram bar haim wrote:

    If we want __CLASS__ to be resolved at runtime (at list in case of trait),
    then what about __FILE__ and __LINE__ ? should they be resolved at compile
    time and reflect the original code in the trait or should they reffer to the
    using class (which is a problem for the __LINE__ ...) ?
    I would argue that __FILE__ and __LINE__ are not referring to conceptual entities, but the literal code. And, I guess, they are mostly used for debugging purposes to identify the relevant code. Thus, I would not change them, but keep them as compile-time constants referring to the actual place where they occur.

    Adapting them to reflect the using classes would at least not seem to be the intuitive semantics, I think.


    Best regards
    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
  • Florian Anderiasch at Jan 16, 2012 at 12:07 pm

    On 01/16/2012 11:25 AM, Stefan Marr wrote:
    I would argue that __FILE__ and __LINE__ are not referring to conceptual entities, but the literal code. And, I guess, they are mostly used for debugging purposes to identify the relevant code. Thus, I would not change them, but keep them as compile-time constants referring to the actual place where they occur.

    Adapting them to reflect the using classes would at least not seem to be the intuitive semantics, I think.
    +1, absolutely.


    Greetings,
    Florian

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedJan 13, '12 at 9:36a
activeJan 17, '12 at 8:32a
posts26
users9
websitephp.net

People

Translate

site design / logo © 2022 Grokbase