Hello Dmitry,
it's just a glance i took at your patch and what you touched. And since i
looked of some of those before i was more interested in reminding you that
the places that formerly had ZEND_HASH_APPLY_STOP actually were doing
ZEND_HASH_APPLY_REMOVE. Apart i discussed with Andi that we should indeed
make all apply functions adhere to the same semantics by making them
understand/respect ZEND_HASH_APPLY_STOP. Once we have that in place a few
apply usage cases can be made faster. E.g. those that only look for a
certain member or condition once.
best regards
marcus
Tuesday, March 14, 2006, 12:32:59 PM, you wrote:
> Hi Marcs,
>> Hello Dmitry, Ilia,
>>
>> in addition to the earlier mail here is the patch against
>> 5.1. And in contrast to my HEAD checkout it works pretty fine
>> with it. I ran the tests and got no new failures. I also
>> started to run gcov tests and the first tests didn't show new
>> failures either.
> I think this patch makes full sense, but if ZEND_HASH_APPLY_STOP will never
> used it will bring just slowdown. :(
>> Another thing Dmitry. You optimized apply calls that are used
>> for destruction of hashes. It would be much faster to change
>> them to hash_destroy since that doesn't fix the hash table
>> consistency after every member that gets destructed and reoved.
> What table do you mean?
> We don't destroy class/function/constant tables on request shutdown.
> Thanks. Dmitry.
>> regards
>> marcus
>>
>>
>> Monday, March 13, 2006, 11:18:05 PM, you wrote:
>>
>> > Hello Dmitry,
>>
>> > you didn't change the apply functions to respect the
>> > ZEND_HASH_APPLY_* consts, i did so now for HEAD. For 5.1 we have it
>> > inconsistent now. And changing it would mean a BC break.
>>
>> > As a consequence i can no longer use run-tests.php in HEAD but
>> > something else seems to be wrong, too.
>>
>> > A patch that fixed the hash apply functions is applied.
>>
>> > best regards
>> > marcus
>>
>> > Monday, March 13, 2006, 12:13:42 PM, you wrote:
>>
>> >> dmitry Mon Mar 13 11:13:42 2006 UTC
>>
>> >> Modified files: (Branch: PHP_5_1)
>> >> /ZendEngine2 zend_compile.h zend_constants.c
>> zend_execute_API.c
>> >> zend_opcode.c
>> >> Log:
>> >> Optimized cleanup loops on request shutdown
>> >>
>> >>
>> >>
>> http://cvs.php.net/viewcvs.cgi/ZendEngine2/zend_compile.h?r1=1.316.2.
>> >> 7&r2=1.316.2.8&diff_format=u
>> >> Index: ZendEngine2/zend_compile.h
>> >> diff -u ZendEngine2/zend_compile.h:1.316.2.7
>> ZendEngine2/zend_compile.h:1.316.2.8
>> >> --- ZendEngine2/zend_compile.h:1.316.2.7 Sat Feb 25
>> 18:25:44 2006
>> >> +++ ZendEngine2/zend_compile.h Mon Mar 13 11:13:42 2006
>> >> @@ -17,7 +17,7 @@
>> >>
>> >>
>> +-------------------------------------------------------------
>> ---------+
>> >> */
>> >>
>> >> -/* $Id: zend_compile.h,v 1.316.2.7 2006/02/25 18:25:44
>> helly Exp $
>> >> */
>> >> +/* $Id: zend_compile.h,v 1.316.2.8 2006/03/13 11:13:42
>> dmitry Exp $
>> >> +*/
>> >>
>> >> #ifndef ZEND_COMPILE_H
>> >> #define ZEND_COMPILE_H
>> >> @@ -517,6 +517,7 @@
>> >> ZEND_API void zend_file_handle_dtor(zend_file_handle
>> *fh); ZEND_API
>> >> int zend_cleanup_class_data(zend_class_entry **pce TSRMLS_DC);
>> >> ZEND_API int zend_cleanup_function_data(zend_function *function
>> >> TSRMLS_DC);
>> >> +ZEND_API int
>> zend_cleanup_function_data_full(zend_function *function
>> >> +TSRMLS_DC);
>> >>
>> >> ZEND_API void destroy_zend_function(zend_function *function
>> >> TSRMLS_DC); ZEND_API void zend_function_dtor(zend_function
>> >> *function);
>> >>
>> http://cvs.php.net/viewcvs.cgi/ZendEngine2/zend_constants.c?r1
>> =1.71.2.1&r2=1.71.2.2&diff_format=u
>> >> Index: ZendEngine2/zend_constants.c
>> >> diff -u ZendEngine2/zend_constants.c:1.71.2.1
>> >> ZendEngine2/zend_constants.c:1.71.2.2
>> >> --- ZendEngine2/zend_constants.c:1.71.2.1 Wed Jan 4
>> 23:53:04 2006
>> >> +++ ZendEngine2/zend_constants.c Mon Mar 13 11:13:42 2006
>> >> @@ -17,7 +17,7 @@
>> >>
>> >>
>> +-------------------------------------------------------------
>> ---------+
>> >> */
>> >>
>> >> -/* $Id: zend_constants.c,v 1.71.2.1 2006/01/04 23:53:04
>> andi Exp $
>> >> */
>> >> +/* $Id: zend_constants.c,v 1.71.2.2 2006/03/13 11:13:42
>> dmitry Exp $
>> >> +*/
>> >>
>> >> #include "zend.h"
>> >> #include "zend_constants.h"
>> >> @@ -55,11 +55,13 @@
>> >>
>> >> static int clean_non_persistent_constant(zend_constant *c
>> TSRMLS_DC)
>> >> {
>> >> - if (c->flags & CONST_PERSISTENT) {
>> >> - return EG(full_tables_cleanup) ? 0 :
>> ZEND_HASH_APPLY_STOP;
>> >> - } else {
>> >> - return EG(full_tables_cleanup) ? 1 :
>> ZEND_HASH_APPLY_REMOVE;
>> >> - }
>> >> + return (c->flags & CONST_PERSISTENT) ?
>> ZEND_HASH_APPLY_STOP :
>> >> + ZEND_HASH_APPLY_REMOVE;
>>
>> > I am quite sure stop is wrong here and eitehr way we don't
>> respect the
>> > consts in all apply functions yet.
>>
>> >> +}
>> >> +
>> >> +
>> >> +static int clean_non_persistent_constant_full(zend_constant *c
>> >> +TSRMLS_DC) {
>> >> + return (c->flags & CONST_PERSISTENT) ?
>> ZEND_HASH_APPLY_KEEP :
>> >> +ZEND_HASH_APPLY_REMOVE;
>> >> }
>> >>
>> >>
>> >> @@ -153,7 +155,7 @@
>> >> void clean_non_persistent_constants(TSRMLS_D)
>> >> {
>> >> if (EG(full_tables_cleanup)) {
>> >> - zend_hash_apply(EG(zend_constants), (apply_func_t)
>> >> clean_non_persistent_constant TSRMLS_CC);
>> >> + zend_hash_apply(EG(zend_constants), (apply_func_t)
>> >> clean_non_persistent_constant_full TSRMLS_CC);
>> >> } else {
>> >> zend_hash_reverse_apply(EG(zend_constants),
>> >> (apply_func_t) clean_non_persistent_constant TSRMLS_CC);
>> >> }
>> >>
>> http://cvs.php.net/viewcvs.cgi/ZendEngine2/zend_execute_API.c?r1=1.33
>> >> 1.2.16&r2=1.331.2.17&diff_format=u
>> >> Index: ZendEngine2/zend_execute_API.c
>> >> diff -u ZendEngine2/zend_execute_API.c:1.331.2.16
>> >> ZendEngine2/zend_execute_API.c:1.331.2.17
>> >> --- ZendEngine2/zend_execute_API.c:1.331.2.16 Fri Mar 3
>> 12:06:29 2006
>> >> +++ ZendEngine2/zend_execute_API.c Mon Mar 13 11:13:42 2006
>> >> @@ -17,7 +17,7 @@
>> >>
>> >>
>> +-------------------------------------------------------------
>> ---------+
>> >> */
>> >>
>> >> -/* $Id: zend_execute_API.c,v 1.331.2.16 2006/03/03
>> 12:06:29 dmitry
>> >> Exp $ */
>> >> +/* $Id: zend_execute_API.c,v 1.331.2.17 2006/03/13
>> 11:13:42 dmitry
>> >> +Exp $ */
>> >>
>> >> #include <stdio.h>
>> >> #include <signal.h>
>> >> @@ -95,23 +95,27 @@
>> >> }
>> >>
>> >>
>> >> -static int is_not_internal_function(zend_function *function
>> >> TSRMLS_DC)
>> >> +static int
>>
>> >> (zend_function *function TSRMLS_DC)
>> >> {
>> >> - if (function->type == ZEND_INTERNAL_FUNCTION) {
>> >> - return EG(full_tables_cleanup) ? 0 :
>> ZEND_HASH_APPLY_STOP;
>> >> - } else {
>> >> - return EG(full_tables_cleanup) ? 1 :
>> ZEND_HASH_APPLY_REMOVE;
>> >> - }
>> >> + return (function->type == ZEND_INTERNAL_FUNCTION) ?
>> >> ZEND_HASH_APPLY_STOP : ZEND_HASH_APPLY_REMOVE;
>>
>> > same here, stop is probably wrong
>>
>> >> }
>> >>
>> >>
>> >> -static int is_not_internal_class(zend_class_entry **ce TSRMLS_DC)
>> >> +static int clean_non_persistent_function_full(zend_function
>> >> +*function TSRMLS_DC)
>> >> {
>> >> - if ((*ce)->type == ZEND_INTERNAL_CLASS) {
>> >> - return EG(full_tables_cleanup) ? 0 :
>> ZEND_HASH_APPLY_STOP;
>> >> - } else {
>> >> - return EG(full_tables_cleanup) ? 1 :
>> ZEND_HASH_APPLY_REMOVE;
>> >> - }
>> >> + return (function->type == ZEND_INTERNAL_FUNCTION) ?
>> >> ZEND_HASH_APPLY_KEEP : ZEND_HASH_APPLY_REMOVE;
>> >> +}
>> >> +
>> >> +
>> >> +static int clean_non_persistent_class(zend_class_entry **ce
>> >> +TSRMLS_DC) {
>> >> + return ((*ce)->type == ZEND_INTERNAL_CLASS) ?
>> >> ZEND_HASH_APPLY_STOP : ZEND_HASH_APPLY_REMOVE;
>>
>> > Once more
>>
>> >> +}
>> >> +
>> >> +
>> >> +static int clean_non_persistent_class_full(zend_class_entry **ce
>> >> +TSRMLS_DC) {
>> >> + return ((*ce)->type == ZEND_INTERNAL_CLASS) ?
>> >> ZEND_HASH_APPLY_KEEP : ZEND_HASH_APPLY_REMOVE;
>> >> }
>> >>
>> >>
>> >> @@ -251,18 +255,22 @@
>> >> So we want first of all to clean up all
>> data and then move to tables destruction.
>> >> Note that only run-time accessed data
>> need to be cleaned up, pre-defined data can
>> >> not contain objects and thus are not
>> probelmatic */
>> >> - zend_hash_apply(EG(function_table), (apply_func_t)
>> >> zend_cleanup_function_data TSRMLS_CC);
>> >> + if (EG(full_tables_cleanup)) {
>> >> + zend_hash_apply(EG(function_table),
>> >> (apply_func_t) zend_cleanup_function_data_full TSRMLS_CC);
>> >> + } else {
>> >> + zend_hash_reverse_apply(EG(function_table),
>> >> (apply_func_t) zend_cleanup_function_data TSRMLS_CC);
>> >> + }
>> >> zend_hash_apply(EG(class_table), (apply_func_t)
>> >> zend_cleanup_class_data TSRMLS_CC);
>> >>
>> >> zend_ptr_stack_destroy(&EG(argument_stack));
>> >>
>> >> /* Destroy all op arrays */
>> >> if (EG(full_tables_cleanup)) {
>> >> - zend_hash_apply(EG(function_table),
>> >> (apply_func_t) is_not_internal_function TSRMLS_CC);
>> >> - zend_hash_apply(EG(class_table),
>> (apply_func_t) is_not_internal_class TSRMLS_CC);
>> >> + zend_hash_apply(EG(function_table),
>> >> (apply_func_t) clean_non_persistent_function_full TSRMLS_CC);
>> >> + zend_hash_apply(EG(class_table),
>> >> + (apply_func_t)
>> >> clean_non_persistent_class_full TSRMLS_CC);
>> >> } else {
>> >> - zend_hash_reverse_apply(EG(function_table),
>> >> (apply_func_t) is_not_internal_function TSRMLS_CC);
>> >> - zend_hash_reverse_apply(EG(class_table),
>> >> (apply_func_t) is_not_internal_class TSRMLS_CC);
>> >> + zend_hash_reverse_apply(EG(function_table),
>> >> (apply_func_t) clean_non_persistent_function TSRMLS_CC);
>> >> + zend_hash_reverse_apply(EG(class_table),
>> >> (apply_func_t) clean_non_persistent_class TSRMLS_CC);
>> >> }
>> >>
>> >> while
>> (EG(symtable_cache_ptr)>=EG(symtable_cache)) {
>> >>
>> http://cvs.php.net/viewcvs.cgi/ZendEngine2/zend_opcode.c?r1=1.110.2.3
>> >> &r2=1.110.2.4&diff_format=u
>> >> Index: ZendEngine2/zend_opcode.c
>> >> diff -u ZendEngine2/zend_opcode.c:1.110.2.3
>> ZendEngine2/zend_opcode.c:1.110.2.4
>> >> --- ZendEngine2/zend_opcode.c:1.110.2.3 Wed Jan 4
>> 23:53:04 2006
>> >> +++ ZendEngine2/zend_opcode.c Mon Mar 13 11:13:42 2006
>> >> @@ -17,7 +17,7 @@
>> >>
>> >>
>> +-------------------------------------------------------------
>> ---------+
>> >> */
>> >>
>> >> -/* $Id: zend_opcode.c,v 1.110.2.3 2006/01/04 23:53:04
>> andi Exp $ */
>> >> +/* $Id: zend_opcode.c,v 1.110.2.4 2006/03/13 11:13:42
>> dmitry Exp $
>> >> +*/
>> >>
>> >> #include <stdio.h>
>> >>
>> >> @@ -132,8 +132,18 @@
>> >> {
>> >> if (function->type == ZEND_USER_FUNCTION) {
>> >> zend_cleanup_op_array_data((zend_op_array
>> *) function);
>> >> - }
>> >> - return 0;
>> >> + return ZEND_HASH_APPLY_KEEP;
>> >> + } else {
>> >> + return ZEND_HASH_APPLY_STOP;
>> >> + }
>> >> +}
>> >> +
>> >> +ZEND_API int
>> zend_cleanup_function_data_full(zend_function *function
>> >> +TSRMLS_DC) {
>> >> + if (function->type == ZEND_USER_FUNCTION) {
>> >> + zend_cleanup_op_array_data((zend_op_array
>> *) function);
>> >> + }
>> >> + return ZEND_HASH_APPLY_KEEP;
>> >> }
>> >>
>> >> ZEND_API int zend_cleanup_class_data(zend_class_entry **pce
>> >> TSRMLS_DC) @@ -142,7 +152,7 @@
>> >> /* Clean all parts that can contain
>> run-time data */
>> >> /* Note that only run-time accessed data
>> need to be cleaned up, pre-defined data can
>> >> not contain objects and thus are not
>> probelmatic */
>> >> - zend_hash_apply(&(*pce)->function_table,
>> (apply_func_t)
>> >> zend_cleanup_function_data TSRMLS_CC);
>> >> + zend_hash_apply(&(*pce)->function_table,
>> >> + (apply_func_t)
>> >> zend_cleanup_function_data_full TSRMLS_CC);
>> >> (*pce)->static_members = NULL;
>> >> } else if (CE_STATIC_MEMBERS(*pce)) {
>> >> zend_hash_destroy(CE_STATIC_MEMBERS(*pce));
>>
>>
>> Best regards,
>> Marcus
>>
Best regards,
Marcus