Complete traits redesign for 5.5

12 years ago by Stefan Marr — view source — reply

unread

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

12 years ago by Dmitry Stogov — view source — reply

unread

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:

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

12 years ago by Rasmus Lerdorf — view source — reply

unread

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

12 years ago by Leigh — view source — reply

unread

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.

12 years ago by Dmitry Stogov — view source — reply

unread

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

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.

12 years ago by Rasmus Lerdorf — view source — reply

unread

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

12 years ago by Dmitry Stogov — view source — reply

unread

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.

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

12 years ago by Pierre Joye — view source — reply

unread

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

12 years ago by Pierre Joye — view source — reply

unread

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 :)

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

--
Pierre

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

12 years ago by Dmitry Stogov — view source — reply

unread

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).

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

--
Pierre

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

12 years ago by Rasmus Lerdorf — view source — reply

unread

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

12 years ago by Dmitry Stogov — view source — reply

unread

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.

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

12 years ago by Stas Malyshev — view source — reply

unread

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

12 years ago by Dmitry Stogov — view source — reply

unread

Hi Stas,

On Wed, Dec 19, 2012 at 1:25 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

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.

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