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
>