From d1d13ed6fa635452ce89155736b267948794e92b Mon Sep 17 00:00:00 2001 From: Go Kudo Date: Wed, 9 Mar 2022 07:35:48 +0900 Subject: [PATCH] [PHP] Remove unnecessary zval initialization (#9600) * [PHP] remove unneccesary zval * fix memory leak, warning, Makefile --- Makefile.am | 1 + php/ext/google/protobuf/def.c | 29 ++++++++----------- php/ext/google/protobuf/protobuf.c | 13 ++------- php/ext/google/protobuf/protobuf.h | 2 +- .../protobuf/tests/unnecessary_zval.phpt | 9 ++++++ php/tests/compile_extension.sh | 1 + 6 files changed, 26 insertions(+), 29 deletions(-) create mode 100644 php/ext/google/protobuf/tests/unnecessary_zval.phpt diff --git a/Makefile.am b/Makefile.am index c6787cc055..0b3c452b88 100644 --- a/Makefile.am +++ b/Makefile.am @@ -840,6 +840,7 @@ php_EXTRA_DIST= \ php/ext/google/protobuf/protobuf.c \ php/ext/google/protobuf/protobuf.h \ php/ext/google/protobuf/wkt.inc \ + php/ext/google/protobuf/tests/unnecessary_zval.phpt \ php/generate_descriptor_protos.sh \ php/generate_test_protos.sh \ php/release.sh \ diff --git a/php/ext/google/protobuf/def.c b/php/ext/google/protobuf/def.c index 9210026df2..dfb96f283f 100644 --- a/php/ext/google/protobuf/def.c +++ b/php/ext/google/protobuf/def.c @@ -732,20 +732,16 @@ void DescriptorPool_CreateWithSymbolTable(zval *zv, upb_DefPool *symtab) { } upb_DefPool *DescriptorPool_GetSymbolTable() { - DescriptorPool *intern = GetPool(get_generated_pool()); - return intern->symtab; + return get_global_symtab(); } - /* * DescriptorPool::getGeneratedPool() * * Returns the generated DescriptorPool. */ PHP_METHOD(DescriptorPool, getGeneratedPool) { - zval ret; - ZVAL_COPY(&ret, get_generated_pool()); - RETURN_COPY_VALUE(&ret); + DescriptorPool_CreateWithSymbolTable(return_value, get_global_symtab()); } /* @@ -880,14 +876,14 @@ static void add_name_mappings(const upb_FileDef *file) { } } -static void add_descriptor(DescriptorPool *pool, +static void add_descriptor(upb_DefPool *symtab, const google_protobuf_FileDescriptorProto *file) { upb_StringView name = google_protobuf_FileDescriptorProto_name(file); upb_Status status; const upb_FileDef *file_def; upb_Status_Clear(&status); - if (upb_DefPool_FindFileByNameWithSize(pool->symtab, name.data, name.size)) { + if (upb_DefPool_FindFileByNameWithSize(symtab, name.data, name.size)) { // Already added. // TODO(teboring): Re-enable this warning when aggregate metadata is // deprecated. @@ -902,10 +898,10 @@ static void add_descriptor(DescriptorPool *pool, // doesn't add it as a dependency even if the proto file actually does // depend on it. if (depends_on_descriptor(file)) { - google_protobuf_FileDescriptorProto_getmsgdef(pool->symtab); + google_protobuf_FileDescriptorProto_getmsgdef(symtab); } - file_def = upb_DefPool_AddFile(pool->symtab, file, &status); + file_def = upb_DefPool_AddFile(symtab, file, &status); CheckUpbStatus(&status, "Unable to load descriptor"); add_name_mappings(file_def); } @@ -915,7 +911,7 @@ static void add_descriptor(DescriptorPool *pool, * * Adds the given descriptor data to this DescriptorPool. */ -static void add_descriptor_set(DescriptorPool *pool, const char *data, +static void add_descriptor_set(upb_DefPool *symtab, const char *data, int data_len, upb_Arena *arena) { size_t i, n; google_protobuf_FileDescriptorSet *set; @@ -932,13 +928,12 @@ static void add_descriptor_set(DescriptorPool *pool, const char *data, for (i = 0; i < n; i++) { const google_protobuf_FileDescriptorProto* file = files[i]; - add_descriptor(pool, file); + add_descriptor(symtab, file); } } bool DescriptorPool_HasFile(const char *filename) { - DescriptorPool *intern = GetPool(get_generated_pool()); - return upb_DefPool_FindFileByName(intern->symtab, filename) != NULL; + return upb_DefPool_FindFileByName(get_global_symtab(), filename) != NULL; } void DescriptorPool_AddDescriptor(const char *filename, const char *data, @@ -952,7 +947,7 @@ void DescriptorPool_AddDescriptor(const char *filename, const char *data, return; } - add_descriptor(GetPool(get_generated_pool()), file); + add_descriptor(get_global_symtab(), file); upb_Arena_Free(arena); } @@ -974,7 +969,7 @@ PHP_METHOD(DescriptorPool, internalAddGeneratedFile) { } arena = upb_Arena_New(); - add_descriptor_set(intern, data, data_len, arena); + add_descriptor_set(intern->symtab, data, data_len, arena); upb_Arena_Free(arena); } @@ -1015,7 +1010,7 @@ zend_class_entry *InternalDescriptorPool_class_entry; * instance. */ PHP_METHOD(InternalDescriptorPool, getGeneratedPool) { - RETURN_COPY(get_generated_pool()); + DescriptorPool_CreateWithSymbolTable(return_value, get_global_symtab()); } static zend_function_entry InternalDescriptorPool_methods[] = { diff --git a/php/ext/google/protobuf/protobuf.c b/php/ext/google/protobuf/protobuf.c index 6f8a534f27..68c0ef00b1 100644 --- a/php/ext/google/protobuf/protobuf.c +++ b/php/ext/google/protobuf/protobuf.c @@ -56,12 +56,6 @@ ZEND_BEGIN_MODULE_GLOBALS(protobuf) // Set by the user to make the descriptor pool persist between requests. zend_bool keep_descriptor_pool_after_request; - // Currently we make the generated pool a "global", which means that if a user - // does explicitly create threads within their request, the other threads will - // get different results from DescriptorPool::getGeneratedPool(). We require - // that all descriptors are loaded from the main thread. - zval generated_pool; - // A upb_DefPool that we are saving for the next request so that we don't have // to rebuild it from scratch. When keep_descriptor_pool_after_request==true, // we steal the upb_DefPool from the global DescriptorPool object just before @@ -91,8 +85,8 @@ void free_protobuf_globals(zend_protobuf_globals *globals) { ZEND_DECLARE_MODULE_GLOBALS(protobuf) -const zval *get_generated_pool() { - return &PROTOBUF_G(generated_pool); +upb_DefPool *get_global_symtab() { + return PROTOBUF_G(global_symtab); } // This is a PHP extension (not a Zend extension). What follows is a summary of @@ -159,7 +153,6 @@ static PHP_GSHUTDOWN_FUNCTION(protobuf) { } static PHP_GINIT_FUNCTION(protobuf) { - ZVAL_NULL(&protobuf_globals->generated_pool); protobuf_globals->global_symtab = NULL; } @@ -177,7 +170,6 @@ static PHP_RINIT_FUNCTION(protobuf) { zend_hash_init(&PROTOBUF_G(name_msg_cache), 64, NULL, NULL, 0); zend_hash_init(&PROTOBUF_G(name_enum_cache), 64, NULL, NULL, 0); } - DescriptorPool_CreateWithSymbolTable(&PROTOBUF_G(generated_pool), symtab); zend_hash_init(&PROTOBUF_G(object_cache), 64, NULL, NULL, 0); zend_hash_init(&PROTOBUF_G(descriptors), 64, NULL, ZVAL_PTR_DTOR, 0); @@ -196,7 +188,6 @@ static PHP_RSHUTDOWN_FUNCTION(protobuf) { free_protobuf_globals(ZEND_MODULE_GLOBALS_BULK(protobuf)); } - zval_dtor(&PROTOBUF_G(generated_pool)); zend_hash_destroy(&PROTOBUF_G(object_cache)); zend_hash_destroy(&PROTOBUF_G(descriptors)); diff --git a/php/ext/google/protobuf/protobuf.h b/php/ext/google/protobuf/protobuf.h index c456c194b9..e008a6b50a 100644 --- a/php/ext/google/protobuf/protobuf.h +++ b/php/ext/google/protobuf/protobuf.h @@ -36,7 +36,7 @@ #include "php-upb.h" -const zval *get_generated_pool(); +upb_DefPool *get_global_symtab(); #if PHP_VERSION_ID < 70300 #define GC_ADDREF(h) ++GC_REFCOUNT(h) diff --git a/php/ext/google/protobuf/tests/unnecessary_zval.phpt b/php/ext/google/protobuf/tests/unnecessary_zval.phpt new file mode 100644 index 0000000000..9a169abf76 --- /dev/null +++ b/php/ext/google/protobuf/tests/unnecessary_zval.phpt @@ -0,0 +1,9 @@ +--TEST-- +unnecessary zval +--FILE-- + +--EXPECT-- +object(stdClass)#1 (0) { +} diff --git a/php/tests/compile_extension.sh b/php/tests/compile_extension.sh index d33458746c..326191738f 100755 --- a/php/tests/compile_extension.sh +++ b/php/tests/compile_extension.sh @@ -34,4 +34,5 @@ if [[ ! -f BUILD_STAMP ]] || [[ "$(cat BUILD_STAMP)" != "$FINGERPRINT" ]]; then fi make +TEST_PHP_ARGS="-q" make test popd > /dev/null