Re: [ZEND-ENGINE-CVS] cvs: ZendEngine2(PHP_5_1) / zend_compile.h zend_constants.c zend_execute_API.c zend_opcode.c

From: Date: Wed, 15 Mar 2006 01:17:54 +0000
Subject: Re: [ZEND-ENGINE-CVS] cvs: ZendEngine2(PHP_5_1) / zend_compile.h zend_constants.c zend_execute_API.c zend_opcode.c
References: 1 2  Groups: php.zend-engine.cvs 
Request: Send a blank email to [email protected] to get a copy of this message
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


Thread (6 messages)

« previous php.zend-engine.cvs (#4727) next »