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.
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.
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.331.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
Index: Zend/zend_hash.c
===================================================================
RCS file: /repository/ZendEngine2/zend_hash.c,v
retrieving revision 1.121.2.3
diff -u -p -d -r1.121.2.3 zend_hash.c
--- Zend/zend_hash.c 6 Feb 2006 20:37:11 -0000 1.121.2.3
+++ Zend/zend_hash.c 13 Mar 2006 22:28:03 -0000
@@ -657,13 +657,14 @@ ZEND_API void zend_hash_graceful_reverse
ZEND_API void zend_hash_apply(HashTable *ht, apply_func_t apply_func TSRMLS_DC)
{
Bucket *p;
+ int result = ZEND_HASH_APPLY_KEEP;
IS_CONSISTENT(ht);
HASH_PROTECT_RECURSION(ht);
p = ht->pListHead;
- while (p != NULL) {
- if (apply_func(p->pData TSRMLS_CC)) {
+ while (p != NULL && !(result & ZEND_HASH_APPLY_STOP)) {
+ if ((result = apply_func(p->pData TSRMLS_CC)) & ZEND_HASH_APPLY_REMOVE) {
p = zend_hash_apply_deleter(ht, p);
} else {
p = p->pListNext;
@@ -676,13 +677,14 @@ ZEND_API void zend_hash_apply(HashTable
ZEND_API void zend_hash_apply_with_argument(HashTable *ht, apply_func_arg_t apply_func, void *argument TSRMLS_DC)
{
Bucket *p;
+ int result = ZEND_HASH_APPLY_KEEP;
IS_CONSISTENT(ht);
HASH_PROTECT_RECURSION(ht);
p = ht->pListHead;
- while (p != NULL) {
- if (apply_func(p->pData, argument TSRMLS_CC)) {
+ while (p != NULL && !(result & ZEND_HASH_APPLY_STOP)) {
+ if ((result = apply_func(p->pData, argument TSRMLS_CC)) & ZEND_HASH_APPLY_REMOVE) {
p = zend_hash_apply_deleter(ht, p);
} else {
p = p->pListNext;
@@ -692,23 +694,24 @@ ZEND_API void zend_hash_apply_with_argum
}
-ZEND_API void zend_hash_apply_with_arguments(HashTable *ht, apply_func_args_t destruct, int num_args, ...)
+ZEND_API void zend_hash_apply_with_arguments(HashTable *ht, apply_func_args_t apply_func, int num_args, ...)
{
Bucket *p;
va_list args;
zend_hash_key hash_key;
+ int result = ZEND_HASH_APPLY_KEEP;
IS_CONSISTENT(ht);
HASH_PROTECT_RECURSION(ht);
p = ht->pListHead;
- while (p != NULL) {
+ while (p != NULL && !(result & ZEND_HASH_APPLY_STOP)) {
va_start(args, num_args);
hash_key.arKey = p->arKey;
hash_key.nKeyLength = p->nKeyLength;
hash_key.h = p->h;
- if (destruct(p->pData, num_args, args, &hash_key)) {
+ if ((result = apply_func(p->pData, num_args, args, &hash_key)) & ZEND_HASH_APPLY_REMOVE) {
p = zend_hash_apply_deleter(ht, p);
} else {
p = p->pListNext;