From e6587545e90433d32834ac8d2c7657164deeb339 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 6 Jan 2010 18:58:16 +0000 Subject: [PATCH] Fixed bug #49560 (oci8: using LOBs causes slow PHP shutdown) - Improved descriptor refcounting to remove unneeded items sooner - Replaced n^2 list traversal during descriptor list destruction --- ext/oci8/oci8.c | 25 ++++----- ext/oci8/oci8_interface.c | 8 +-- ext/oci8/oci8_lob.c | 30 ++++++++++- ext/oci8/oci8_statement.c | 41 ++++++++++++++ ext/oci8/php_oci8_int.h | 7 ++- ext/oci8/tests/bug43497.phpt | 22 ++++---- ext/oci8/tests/lob_043.phpt | 101 +++++++++++++++++++++++++++++++++++ 7 files changed, 199 insertions(+), 35 deletions(-) create mode 100644 ext/oci8/tests/lob_043.phpt diff --git a/ext/oci8/oci8.c b/ext/oci8/oci8.c index e78ec00119..2b60f0a399 100644 --- a/ext/oci8/oci8.c +++ b/ext/oci8/oci8.c @@ -1368,24 +1368,21 @@ void php_oci_descriptor_flush_hash_dtor(void *data) } /* }}} */ -/* {{{ php_oci_descriptor_delete_from_hash() +/* }}} */ + +/* {{{ php_oci_connection_descriptors_free() * - * Delete descriptor from the hash + * Free descriptors for a connection */ -int php_oci_descriptor_delete_from_hash(void *data, void *id TSRMLS_DC) +void php_oci_connection_descriptors_free(php_oci_connection *connection TSRMLS_DC) { - php_oci_descriptor *descriptor = *(php_oci_descriptor **)data; - int *desc_id = (int *) id; - - if (descriptor && desc_id && descriptor->id == *desc_id) { - return 1; - } - return 0; + zend_hash_destroy(connection->descriptors); + efree(connection->descriptors); + connection->descriptors = NULL; + connection->descriptor_count = 0; } /* }}} */ -/* }}} */ - /* {{{ php_oci_error() * * Fetch & print out error message if we get an error @@ -2111,9 +2108,7 @@ int php_oci_connection_release(php_oci_connection *connection TSRMLS_DC) } if (connection->descriptors) { - zend_hash_destroy(connection->descriptors); - efree(connection->descriptors); - connection->descriptors = NULL; + php_oci_connection_descriptors_free(connection TSRMLS_CC); } if (connection->svc) { diff --git a/ext/oci8/oci8_interface.c b/ext/oci8/oci8_interface.c index 9cea077cb5..7d7963b6a1 100644 --- a/ext/oci8/oci8_interface.c +++ b/ext/oci8/oci8_interface.c @@ -1106,9 +1106,7 @@ PHP_FUNCTION(oci_rollback) PHP_OCI_ZVAL_TO_CONNECTION(z_connection, connection); if (connection->descriptors) { - zend_hash_destroy(connection->descriptors); - efree(connection->descriptors); - connection->descriptors = NULL; + php_oci_connection_descriptors_free(connection TSRMLS_CC); } if (php_oci_connection_rollback(connection TSRMLS_CC)) { @@ -1132,9 +1130,7 @@ PHP_FUNCTION(oci_commit) PHP_OCI_ZVAL_TO_CONNECTION(z_connection, connection); if (connection->descriptors) { - zend_hash_destroy(connection->descriptors); - efree(connection->descriptors); - connection->descriptors = NULL; + php_oci_connection_descriptors_free(connection TSRMLS_CC); } if (php_oci_connection_commit(connection TSRMLS_CC)) { diff --git a/ext/oci8/oci8_lob.c b/ext/oci8/oci8_lob.c index e210aae6be..540d6d9686 100644 --- a/ext/oci8/oci8_lob.c +++ b/ext/oci8/oci8_lob.c @@ -95,9 +95,17 @@ php_oci_descriptor *php_oci_lob_create (php_oci_connection *connection, long typ if (!connection->descriptors) { ALLOC_HASHTABLE(connection->descriptors); zend_hash_init(connection->descriptors, 0, NULL, php_oci_descriptor_flush_hash_dtor, 0); + connection->descriptor_count = 0; + } + + descriptor->index = (connection->descriptor_count)++; + if (connection->descriptor_count == LONG_MAX) { + php_error_docref(NULL TSRMLS_CC, E_WARNING, "Internal descriptor counter has reached limit"); + php_oci_connection_descriptors_free(connection TSRMLS_CC); + return NULL; } - zend_hash_next_index_insert(connection->descriptors,&descriptor,sizeof(php_oci_descriptor *),NULL); + zend_hash_index_update(connection->descriptors,descriptor->index,&descriptor,sizeof(php_oci_descriptor *),NULL); } return descriptor; @@ -672,7 +680,25 @@ void php_oci_lob_free (php_oci_descriptor *descriptor TSRMLS_DC) if (descriptor->connection->descriptors) { /* delete descriptor from the hash */ - zend_hash_apply_with_argument(descriptor->connection->descriptors, php_oci_descriptor_delete_from_hash, (void *)&descriptor->id TSRMLS_CC); + zend_hash_index_del(descriptor->connection->descriptors, descriptor->index); + if (zend_hash_num_elements(descriptor->connection->descriptors) == 0) { + descriptor->connection->descriptor_count = 0; + } else { + if (descriptor->index + 1 == descriptor->connection->descriptor_count) { + /* If the descriptor being freed is the end-most one + * allocated, then the descriptor_count is reduced so + * a future descriptor can reuse the hash table index. + * This can prevent the hash index range increasing in + * the common case that each descriptor is + * allocated/used/freed before another descriptor is + * needed. However it is possible that a script frees + * descriptors in arbitrary order which would prevent + * descriptor_count ever being reduced to zero until + * zend_hash_num_elements() returns 0. + */ + descriptor->connection->descriptor_count--; + } + } } /* flushing Lobs & Files with buffering enabled */ diff --git a/ext/oci8/oci8_statement.c b/ext/oci8/oci8_statement.c index b5ef31c451..04614fe406 100644 --- a/ext/oci8/oci8_statement.c +++ b/ext/oci8/oci8_statement.c @@ -95,6 +95,7 @@ php_oci_statement *php_oci_statement_create (php_oci_connection *connection, zst statement->connection = connection; statement->has_data = 0; + statement->has_descr = 0; statement->parent_stmtid = 0; zend_list_addref(statement->connection->rsrc_id); @@ -133,6 +134,40 @@ int php_oci_statement_set_prefetch(php_oci_statement *statement, long size TSRML } /* }}} */ +/* {{{ php_oci_cleanup_pre_fetch() + Helper function to cleanup ref-cursors and descriptors from the previous row */ +int php_oci_cleanup_pre_fetch(void *data TSRMLS_DC) +{ + php_oci_out_column *outcol = data; + + if (!outcol->is_descr && !outcol->is_cursor) + return ZEND_HASH_APPLY_KEEP; + + switch(outcol->data_type) { + case SQLT_CLOB: + case SQLT_BLOB: + case SQLT_RDD: + case SQLT_BFILE: + if (outcol->descid) { + zend_list_delete(outcol->descid); + outcol->descid = 0; + } + break; + case SQLT_RSET: + if (outcol->stmtid) { + zend_list_delete(outcol->stmtid); + outcol->stmtid = 0; + outcol->nested_statement = NULL; + } + break; + default: + break; + } + return ZEND_HASH_APPLY_KEEP; + +} /* }}} */ + + /* {{{ php_oci_statement_fetch() Fetch a row from the statement */ int php_oci_statement_fetch(php_oci_statement *statement, ub4 nrows TSRMLS_DC) @@ -145,6 +180,10 @@ int php_oci_statement_fetch(php_oci_statement *statement, ub4 nrows TSRMLS_DC) php_oci_out_column *column; zstr zstr_data; + if (statement->has_descr && statement->columns) { + zend_hash_apply(statement->columns, (apply_func_t) php_oci_cleanup_pre_fetch TSRMLS_CC); + } + PHP_OCI_CALL_RETURN(statement->errcode, OCIStmtFetch, (statement->stmt, statement->err, nrows, OCI_FETCH_NEXT, OCI_DEFAULT)); if ( statement->errcode == OCI_NO_DATA || nrows == 0 ) { @@ -577,6 +616,7 @@ int php_oci_statement_execute(php_oci_statement *statement, ub4 mode TSRMLS_DC) define_type = SQLT_RSET; outcol->is_cursor = 1; + outcol->statement->has_descr = 1; outcol->storage_size4 = -1; outcol->retlen = -1; dynamic = OCI_DYNAMIC_FETCH; @@ -590,6 +630,7 @@ int php_oci_statement_execute(php_oci_statement *statement, ub4 mode TSRMLS_DC) define_type = outcol->data_type; outcol->is_descr = 1; + outcol->statement->has_descr = 1; outcol->storage_size4 = -1; dynamic = OCI_DYNAMIC_FETCH; break; diff --git a/ext/oci8/php_oci8_int.h b/ext/oci8/php_oci8_int.h index 5b4c7efb0a..8c5045c9bd 100644 --- a/ext/oci8/php_oci8_int.h +++ b/ext/oci8/php_oci8_int.h @@ -136,6 +136,7 @@ typedef struct { /* php_oci_connection {{{ */ sword errcode; /* last errcode */ HashTable *descriptors; /* descriptors hash, used to flush all the LOBs using this connection on commit */ + ulong descriptor_count; /* used to index the descriptors hash table. Not an accurate count */ unsigned is_open:1; /* hels to determine if the connection is dead or not */ unsigned is_attached:1; /* hels to determine if we should detach from the server when closing/freeing the connection */ unsigned is_persistent:1; /* self-descriptive */ @@ -153,6 +154,7 @@ typedef struct { /* php_oci_connection {{{ */ typedef struct { /* php_oci_descriptor {{{ */ int id; + ulong index; /* descriptors hash table index */ php_oci_connection *connection; /* parent connection handle */ dvoid *descriptor; /* OCI descriptor handle */ ub4 type; /* descriptor type (FILE/LOB) */ @@ -206,6 +208,7 @@ typedef struct { /* php_oci_statement {{{ */ int ncolumns; /* number of columns in the result */ unsigned executed:1; /* statement executed flag */ unsigned has_data:1; /* statement has more data flag */ + unsigned has_descr:1; /* statement has at least one descriptor or cursor column */ ub2 stmttype; /* statement type */ } php_oci_statement; /* }}} */ @@ -377,7 +380,8 @@ void php_oci_column_hash_dtor (void *data); void php_oci_define_hash_dtor (void *data); void php_oci_bind_hash_dtor (void *data); void php_oci_descriptor_flush_hash_dtor (void *data); -int php_oci_descriptor_delete_from_hash(void *data, void *id TSRMLS_DC); + +void php_oci_connection_descriptors_free(php_oci_connection *connection TSRMLS_DC); sb4 php_oci_error (OCIError *, sword TSRMLS_DC); sb4 php_oci_fetch_errmsg(OCIError *, text ** TSRMLS_DC); @@ -463,6 +467,7 @@ int php_oci_bind_by_name(php_oci_statement *, zstr, int, zval*, long, ub2, zend_ sb4 php_oci_bind_in_callback(dvoid *, OCIBind *, ub4, ub4, dvoid **, ub4 *, ub1 *, dvoid **); sb4 php_oci_bind_out_callback(dvoid *, OCIBind *, ub4, ub4, dvoid **, ub4 **, ub1 *, dvoid **, ub2 **); php_oci_out_column *php_oci_statement_get_column_helper(INTERNAL_FUNCTION_PARAMETERS, int need_data); +int php_oci_cleanup_pre_fetch(void *data TSRMLS_DC); int php_oci_statement_get_type(php_oci_statement *, ub2 * TSRMLS_DC); int php_oci_statement_get_numrows(php_oci_statement *, ub4 * TSRMLS_DC); diff --git a/ext/oci8/tests/bug43497.phpt b/ext/oci8/tests/bug43497.phpt index 0fc6a97b35..1ea46b40d7 100644 --- a/ext/oci8/tests/bug43497.phpt +++ b/ext/oci8/tests/bug43497.phpt @@ -157,7 +157,7 @@ for ($i = 1; $i <= 10; $i++) { readxmltab_im($c); } -echo "\nExplicit LOB with no free (i.e. a temp lob leak)\n"; +echo "\nExplicit LOB with no free\n"; for ($i = 1; $i <= 10; $i++) { echo "\nRun = " . $i . "\n"; echo "Temporary LOBs = " . templobs($c, $sid) . "\n"; @@ -259,45 +259,45 @@ Run = 10 Temporary LOBs = 0 Loop count check = 100 -Explicit LOB with no free (i.e. a temp lob leak) +Explicit LOB with no free Run = 1 Temporary LOBs = 0 Loop count check = 100 Run = 2 -Temporary LOBs = 99 +Temporary LOBs = 0 Loop count check = 100 Run = 3 -Temporary LOBs = 198 +Temporary LOBs = 0 Loop count check = 100 Run = 4 -Temporary LOBs = 297 +Temporary LOBs = 0 Loop count check = 100 Run = 5 -Temporary LOBs = 396 +Temporary LOBs = 0 Loop count check = 100 Run = 6 -Temporary LOBs = 495 +Temporary LOBs = 0 Loop count check = 100 Run = 7 -Temporary LOBs = 594 +Temporary LOBs = 0 Loop count check = 100 Run = 8 -Temporary LOBs = 693 +Temporary LOBs = 0 Loop count check = 100 Run = 9 -Temporary LOBs = 792 +Temporary LOBs = 0 Loop count check = 100 Run = 10 -Temporary LOBs = 891 +Temporary LOBs = 0 Loop count check = 100 Done diff --git a/ext/oci8/tests/lob_043.phpt b/ext/oci8/tests/lob_043.phpt new file mode 100644 index 0000000000..5ae2d45ebc --- /dev/null +++ b/ext/oci8/tests/lob_043.phpt @@ -0,0 +1,101 @@ +--TEST-- +Bug #49560 (LOB resource destructor and refcount test) +--SKIPIF-- + +--FILE-- +load(), "\n"; + } +*/ + +// Clean up + +$stmtarray = array( + "drop table lob_043_tab" +); + +foreach ($stmtarray as $stmt) { + $s = oci_parse($c, $stmt); + oci_execute($s); +} + +oci_close($c); + +?> +===DONE=== + +--EXPECTF-- +Test 1 +f1 ended +Test 2 +f2 ended +===DONE=== -- 2.40.0