# Complete traits redesign for 5.5

- Thread: https://externals.io/message/64335
- Original archive: https://news-web.php.net/php.internals/64335
- Started by Dmitry Stogov on 2012-12-18T11:37:17+00:00
- Messages: 15
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #64335 — Dmitry Stogov — 2012-12-18T11:37:17+00:00

URL: https://externals.io/message/64335#64335

Hi,

I'm going to take a deep look into trait implementation and provide a
better solution for 5.5.
The current implementation is really wired and makes a lot of troubles for
maintenance and each new fix, makes new troubles :(
I'm really sorry, I didn't pay enough attention to treats before 5.4
release :(

The new solution may significantly change implementation and even behavior
in some cases (e.g https://bugs.php.net/bug.php?id=62069).

I'm going to work on it with top priority during last few days and then
send a patch.
Any ideas are welcome...

Thanks. Dmitry.

---

## #64336 — Stefan Marr — 2012-12-18T11:46:28+00:00

URL: https://externals.io/message/64335#64336 · In reply to: #64335

Hi Dmitry:



> I'm going to take a deep look into trait implementation and provide a
> better solution for 5.5.
> The current implementation is really wired and makes a lot of troubles for
> maintenance and each new fix, makes new troubles :(
Sorry, that's mostly me lacking understanding of the PHP internals.
And there are many wired corner cases that have to be covered.

> I'm going to work on it with top priority during last few days and then
> send a patch.

Thanks for looking into it.
I'll be able to test things over christmas.

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

---

## #64339 — Leigh — 2012-12-18T13:03:09+00:00

URL: https://externals.io/message/64335#64339 · In reply to: #64335

Hi Dmitry



> The new solution may significantly change implementation and even behavior
> in some cases (e.g https://bugs.php.net/bug.php?id=62069).
>

If you have any idea, do you know what the implications of your changes are
on APC?

We're preparing for 5.5 and we still don't have a "stable" tag of APC for
5.4 yet.

I'd hate to see us in the same situation with 5.5, where people simply will
not upgrade because APC needs massive amounts of work to get it back to a
stable state.

---

## #64350 — Stas Malyshev — 2012-12-18T21:25:46+00:00

URL: https://externals.io/message/64335#64350 · In reply to: #64335

Hi!

> I'm going to take a deep look into trait implementation and provide a
> better solution for 5.5.
> The current implementation is really wired and makes a lot of troubles for
> maintenance and each new fix, makes new troubles :(
> I'm really sorry, I didn't pay enough attention to treats before 5.4
> release :(
> 
> The new solution may significantly change implementation and even behavior
> in some cases (e.g https://bugs.php.net/bug.php?id=62069).

Thanks for looking into it! Could you write some description of what
functionality changes are needed and why? An RFC would be helpful, so we
have a definite reference and instead of going through all bug comments
and risking missing something.

BTW, could you after that take a look at bug #63462? It's kind of weird,
and part of it with protected seems to be a bug (same property name is
used both mangled and unmangled when using guards) but I'm not sure yet
what to do with it. I may have time to look into it on the weekend, but
would appreciate second opinion.

Thanks,
-- 
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

---

## #64352 — Dmitry Stogov — 2012-12-19T06:39:23+00:00

URL: https://externals.io/message/64335#64352 · In reply to: #64339

Hi,

opcode caches support is one of the problem we have with current
implementation.
5.4.10 seems just can't work with any cache at all.
Of course, I'll care about it, and may give suggestions for necessary APC
changes.

Thanks. Dmitry.



>
> Hi Dmitry
>
> 
>
> > […]
>
> If you have any idea, do you know what the implications of your changes
> are on APC?
>
> We're preparing for 5.5 and we still don't have a "stable" tag of APC for
> 5.4 yet.
>
> I'd hate to see us in the same situation with 5.5, where people simply
> will not upgrade because APC needs massive amounts of work to get it back
> to a stable state.

---

## #64353 — Dmitry Stogov — 2012-12-19T06:45:38+00:00

URL: https://externals.io/message/64335#64353 · In reply to: #64350

Hi Stas,

On Wed, Dec 19, 2012 at 1:25 AM, Stas Malyshev <smalyshev@sugarcrm.com>wrote:

> Hi!
>
> > […]
> for
> > […]
> behavior
> > […]
>
> Thanks for looking into it! Could you write some description of what
> functionality changes are needed and why? An RFC would be helpful, so we
> have a definite reference and instead of going through all bug comments
> and risking missing something.
>

For now I don't know exactly, what is going to be changed, but it must be
mainly implementation.
I'll try to summarize the required changes, when I know myself.


> BTW, could you after that take a look at bug #63462? It's kind of weird,
> and part of it with protected seems to be a bug (same property name is
> used both mangled and unmangled when using guards) but I'm not sure yet
> what to do with it. I may have time to look into it on the weekend, but
> would appreciate second opinion.
>

Sure. Assigned to myself.

Thanks. Dmitry.


> Thanks,
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227

---

## #64363 — Rasmus Lerdorf — 2012-12-19T19:09:45+00:00

URL: https://externals.io/message/64335#64363 · In reply to: #64352

> Hi,
> 
> opcode caches support is one of the problem we have with current
> implementation.
> 5.4.10 seems just can't work with any cache at all.
> Of course, I'll care about it, and may give suggestions for necessary
> APC changes.

Do you have an example use of traits that doesn't work with APC? I
actually thought we had gotten to all of the edge cases here. At least
all the trait tests pass currently with APC. But yes, it was a pain, so
some cleanup would be good.

-Rasmus

---

## #64373 — Dmitry Stogov — 2012-12-20T06:54:35+00:00

URL: https://externals.io/message/64335#64373 · In reply to: #64363

Hi Rasmus,

I'm not sure about APC, I saw the problem in ZendOptimizerPlus with
php-5.4.10.
O+ crashes (or corrupts memory and crashes on following requests) on each
trait usage.
The problem that PHP tries to deallocate names of methods defined in
traits, but O+ keeps them in SHM.
I believe APC must have the same problem.

Thanks. Dmitry.



> 
> > […]
>
> Do you have an example use of traits that doesn't work with APC? I
> actually thought we had gotten to all of the edge cases here. At least
> all the trait tests pass currently with APC. But yes, it was a pain, so
> some cleanup would be good.
>
> -Rasmus

---

## #64374 — Pierre Joye — 2012-12-20T08:02:32+00:00

URL: https://externals.io/message/64335#64374 · In reply to: #64373

hi Dmitry!



Thanks a lot to work on that :)

> I'm not sure about APC, I saw the problem in ZendOptimizerPlus with
> php-5.4.10.
> O+ crashes (or corrupts memory and crashes on following requests) on each
> trait usage.
> The problem that PHP tries to deallocate names of methods defined in
> traits, but O+ keeps them in SHM.
> I believe APC must have the same problem.

Do you have some phpt to share? I can run them in our tests labs and
report issues to the bugs tracker, and see if Anatolyi can help to fix
them as well.


Cheers,
--
Pierre

@pierrejoye

---

## #64375 — Pierre Joye — 2012-12-20T08:07:37+00:00

URL: https://externals.io/message/64335#64375 · In reply to: #64374

btw, same to test your changes/branch, as we have seen some crashes
happening more easily on windows (same bug(s) on linux but with harder
to get them crash).


> hi Dmitry!
>
> 
>
> Thanks a lot to work on that :)
>
> > […]
>
> Do you have some phpt to share? I can run them in our tests labs and
> report issues to the bugs tracker, and see if Anatolyi can help to fix
> them as well.
>
>
> Cheers,
> --
> Pierre
>
> @pierrejoye



-- 
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

---

## #64377 — Dmitry Stogov — 2012-12-20T11:36:43+00:00

URL: https://externals.io/message/64335#64377 · In reply to: #64375

Hi Pierre,

The following test may crash on the second request with opcode cache.

<?php

trait THello {

  public function hello() { echo 'Hello'; }
}

class TraitsTest { use THello; }

$test = new TraitsTest();
$test->hello();

?>

Valgrind shows the problem even if PHP doesn't crash.

==2623== Invalid free() / delete / delete[] / realloc()
==2623== at 0x4007F0F: free (vg_replace_malloc.c:446)
==2623== by 0x837CC7C: zend_clear_trait_method_name (zend_opcode.c:273)
==2623== by 0x8393B1B: zend_hash_apply (zend_hash.c:716)
==2623== by 0x837D4C7: destroy_zend_class (zend_opcode.c:312)
==2623== by 0x8392464: zend_hash_apply_deleter (zend_hash.c:650)
==2623== by 0x8393D38: zend_hash_reverse_apply (zend_hash.c:804)
==2623== by 0x8378C3B: shutdown_executor (zend_execute_API.c:305)
==2623== by 0x8386C09: zend_deactivate (zend.c:938)
==2623== by 0x8329815: php_request_shutdown (main.c:1790)
With opcode cache op_code->function_name is actually allocated in shared
memory .

Thanks. Dmitry.



> btw, same to test your changes/branch, as we have seen some crashes
> happening more easily on windows (same bug(s) on linux but with harder
> to get them crash).
>
> 
> > […]
> each
> > […]
>
>
>
> --
> Pierre
>
> @pierrejoye | http://blog.thepimp.net | http://www.libgd.org

---

## #64385 — Rasmus Lerdorf — 2012-12-20T17:04:18+00:00

URL: https://externals.io/message/64335#64385 · In reply to: #64377

> Hi Pierre,
> 
> The following test may crash on the second request with opcode cache.
> 
> <?php
> 
> trait THello {
> 
>   public function hello() { echo 'Hello'; }
> }
> 
> class TraitsTest { use THello; }
> 
> $test = new TraitsTest();
> $test->hello();
> 
> ?>
> 
> Valgrind shows the problem even if PHP doesn't crash.

I'm not seeing anything from Valgrind's memcheck on this under php -S
with current PHP 5.4 and APC. Testing on 64-bit Linux.

-Rasmus

---

## #64392 — Dmitry Stogov — 2012-12-20T23:33:41+00:00

URL: https://externals.io/message/64335#64392 · In reply to: #64385

hi Rasmus,

I don't know all the APC internals, but it seems it just doesn't free
memory carefully.
It sets zend_class_entry->refcount to something above 1000 and as result
all the nested structures are not freed as expected by destroy_zend_class().

I'm not sure which side effects this may have, but one of them is avoiding
of the trait related problem :)

Of course, It produces many memory leaks but most of them (probably all)
must be handled by zend_alloc

$ PHP_FCGI_MAX_REQUESTS=2 USE_ZEND_ALLOC=0 valgrind sapi/cgi/php-cgi -b
/tmp/fcgi-php5
...
==30930==    definitely lost: 676 bytes in 10 blocks
==30930==    indirectly lost: 1,213 bytes in 24 blocks
...

~2K memory leaks on 2 HTTP requests (the same trait.php test)

It also must change the destruction order of static members and methods
static variables, but I'm too lazy to prove it.

Anyway, the traits problem really exists and I almost found a solution for
5.4 without BC break.
I'll post a patch tomorrow or on next week.

Thanks. Dmitry.



> 
> > […]
>
> I'm not seeing anything from Valgrind's memcheck on this under php -S
> with current PHP 5.4 and APC. Testing on 64-bit Linux.
>
> -Rasmus

---

## #64411 — Dmitry Stogov — 2012-12-21T14:28:43+00:00

URL: https://externals.io/message/64335#64411 · In reply to: #64336

Hi,

This is more or less final proposed patch for 5.4

http://pastebin.com/ceiWWD4N

It fixes implementation mistakes and makes the whole implementation much
simpler.
I hope I didn't introduce new bugs :)
Of course I checked it with PHP test suite, but it would be great to test
it with real life applications that use traits.
The patch keeps binary compatibility, and changes only error messages and
behavior in error situations.
It must not affect any applications.

I like to commit the patch in 5.4 and above on next week.

I would simplify the trait implementation much more, but the conflict
resolution rules allows to make so many tricks that all needs to be checked
:(
Anyway, I'll think about it a bit more.

Any feedback is welcome.

Thanks. Dmitry.





> Hi Dmitry:
>
> 
>
> > […]
> for
> > […]
> Sorry, that's mostly me lacking understanding of the PHP internals.
> And there are many wired corner cases that have to be covered.
>
> > […]
>
> Thanks for looking into it.
> I'll be able to test things over christmas.
>
> 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

---

## #64415 — Rasmus Lerdorf — 2012-12-22T12:30:43+00:00

URL: https://externals.io/message/64335#64415 · In reply to: #64411

> Hi,
> 
> This is more or less final proposed patch for 5.4
> 
> http://pastebin.com/ceiWWD4N
> 
> It fixes implementation mistakes and makes the whole implementation much
> simpler.
> I hope I didn't introduce new bugs :)
> Of course I checked it with PHP test suite, but it would be great to
> test it with real life applications that use traits.
> The patch keeps binary compatibility, and changes only error messages
> and behavior in error situations.
> It must not affect any applications.
> 
> I like to commit the patch in 5.4 and above on next week.
> 
> I would simplify the trait implementation much more, but the conflict
> resolution rules allows to make so many tricks that all needs to be
> checked :(
> Anyway, I'll think about it a bit more.
> 
> Any feedback is welcome.

You keep spelling "abstract" as "abstarct" throughout the patch. But it
looks good to me. I always like it when a patch removes more lines than
it adds.

-Rasmus