Re: [PHP-DEV] Complete traits redesign for 5.5
On 12/21/2012 09:28 AM, Dmitry Stogov wrote: 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Complete traits redesign for 5.5
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. On Tue, Dec 18, 2012 at 3:46 PM, Stefan Marr p...@stefan-marr.de wrote: Hi Dmitry: On 18 Dec 2012, at 12:37, Dmitry Stogov wrote: 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
Re: [PHP-DEV] Complete traits redesign for 5.5
hi Dmitry! On Thu, Dec 20, 2012 at 7:54 AM, Dmitry Stogov dmi...@zend.com wrote: 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Complete traits redesign for 5.5
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). On Thu, Dec 20, 2012 at 9:02 AM, Pierre Joye pierre@gmail.com wrote: hi Dmitry! On Thu, Dec 20, 2012 at 7:54 AM, Dmitry Stogov dmi...@zend.com wrote: 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Complete traits redesign for 5.5
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.
On Thu, Dec 20, 2012 at 12:07 PM, Pierre Joye pierre@gmail.com wrote:
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).
On Thu, Dec 20, 2012 at 9:02 AM, Pierre Joye pierre@gmail.com wrote:
hi Dmitry!
On Thu, Dec 20, 2012 at 7:54 AM, Dmitry Stogov dmi...@zend.com wrote:
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
Re: [PHP-DEV] Complete traits redesign for 5.5
On 12/20/2012 06:36 AM, Dmitry Stogov wrote:
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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Complete traits redesign for 5.5
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.
On Thu, Dec 20, 2012 at 9:04 PM, Rasmus Lerdorf ras...@lerdorf.com wrote:
On 12/20/2012 06:36 AM, Dmitry Stogov wrote:
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
Re: [PHP-DEV] Complete traits redesign for 5.5
On 12/19/2012 01:39 AM, Dmitry Stogov wrote: 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Complete traits redesign for 5.5
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. On Wed, Dec 19, 2012 at 11:09 PM, Rasmus Lerdorf ras...@lerdorf.com wrote: On 12/19/2012 01:39 AM, Dmitry Stogov wrote: 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
[PHP-DEV] Complete traits redesign for 5.5
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.
Re: [PHP-DEV] Complete traits redesign for 5.5
Hi Dmitry: On 18 Dec 2012, at 12:37, Dmitry Stogov wrote: 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Complete traits redesign for 5.5
Hi Dmitry On 18 December 2012 11:37, Dmitry Stogov dmi...@zend.com wrote: 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.
Re: [PHP-DEV] Complete traits redesign for 5.5
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Complete traits redesign for 5.5
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. On Tue, Dec 18, 2012 at 5:03 PM, Leigh lei...@gmail.com wrote: Hi Dmitry On 18 December 2012 11:37, Dmitry Stogov dmi...@zend.com wrote: 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.
Re: [PHP-DEV] Complete traits redesign for 5.5
Hi Stas, On Wed, Dec 19, 2012 at 1:25 AM, Stas Malyshev smalys...@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
