From 7d97c97d4c7f79abe0950b4ef3e814753fb955d9 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 26 Sep 2015 18:58:12 +0200 Subject: [PATCH 1/4] Fix multi-threading, initialize V8 only once --- Makefile.frag | 5 ++++ php_v8js_macros.h | 30 +++++++++++++++++-- tests/pthreads_001.phpt | 65 +++++++++++++++++++++++++++++++++++++++++ v8js.cc | 19 +++++++----- v8js_v8.cc | 25 ++++++++++++---- 5 files changed, 128 insertions(+), 16 deletions(-) create mode 100644 tests/pthreads_001.phpt diff --git a/Makefile.frag b/Makefile.frag index 77ac51d..93abcdc 100644 --- a/Makefile.frag +++ b/Makefile.frag @@ -3,6 +3,11 @@ ifneq (,$(realpath $(EXTENSION_DIR)/json.so)) PHP_TEST_SHARED_EXTENSIONS+=-d extension=$(EXTENSION_DIR)/json.so endif +# add pthreads extension, if available +ifneq (,$(realpath $(EXTENSION_DIR)/pthreads.so)) +PHP_TEST_SHARED_EXTENSIONS+=-d extension=$(EXTENSION_DIR)/pthreads.so +endif + testv8: all $(PHP_EXECUTABLE) -n -d extension_dir=./modules -d extension=v8js.so test.php diff --git a/php_v8js_macros.h b/php_v8js_macros.h index 8fb5536..2f8c2a5 100644 --- a/php_v8js_macros.h +++ b/php_v8js_macros.h @@ -111,10 +111,8 @@ void v8js_register_accessors(std::vector *accessor_list, v8: /* Module globals */ ZEND_BEGIN_MODULE_GLOBALS(v8js) + // Thread-local cache whether V8 has been initialized so far int v8_initialized; -#if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036 - v8::Platform *v8_platform; -#endif HashTable *extensions; /* Ini globals */ @@ -142,6 +140,32 @@ ZEND_EXTERN_MODULE_GLOBALS(v8js) # define V8JSG(v) (v8js_globals.v) #endif +/* + * Process-wide globals + * + * The zend_v8js_globals structure declared above is created once per thread + * (in a ZTS environment). If a multi-threaded PHP process uses V8 there is + * some stuff shared among all of these threads + * + * - whether V8 has been initialized at all + * - the V8 backend platform + * + * In a ZTS-enabled environment access to all of these variables must happen + * while holding a mutex lock. + */ +struct _v8js_process_globals { +#ifdef ZTS + int v8_initialized; + std::mutex lock; +#endif + +#if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036 + v8::Platform *v8_platform; +#endif +}; + +extern struct _v8js_process_globals v8js_process_globals; + /* Register builtin methods into passed object */ void v8js_register_methods(v8::Handle, v8js_ctx *c); diff --git a/tests/pthreads_001.phpt b/tests/pthreads_001.phpt new file mode 100644 index 0000000..b177fbf --- /dev/null +++ b/tests/pthreads_001.phpt @@ -0,0 +1,65 @@ +--TEST-- +Test V8::executeString() : Pthreads test #1 +--SKIPIF-- + +--FILE-- +val = $val; + } + + public function run() + { + $v8 = new V8Js(); + $v8->val = $this->val; + $v8->sync_var_dump = function($value) { + $this->synchronized(function($thread) use ($value) { + while(!$thread->readyToPrint) { + $thread->wait(); + } + var_dump($value); + $thread->notify(); + }, $this); + }; + + $v8->executeString('PHP.sync_var_dump(PHP.val);'); + } +} + +$foo = new Workhorse('foo'); +$bar = new Workhorse('bar'); + +$foo->start(); +$bar->start(); + +$bar->synchronized(function($thread) { + $thread->readyToPrint = true; + $thread->notify(); + $thread->wait(); +}, $bar); + +$foo->synchronized(function($thread) { + $thread->readyToPrint = true; + $thread->notify(); + $thread->wait(); +}, $foo); + +$foo->join(); +$bar->join(); +?> +===EOF=== +--EXPECT-- +string(3) "bar" +string(3) "foo" +===EOF=== \ No newline at end of file diff --git a/v8js.cc b/v8js.cc index cdda22f..25499e0 100755 --- a/v8js.cc +++ b/v8js.cc @@ -29,6 +29,7 @@ extern "C" { #include "v8js_v8object_class.h" ZEND_DECLARE_MODULE_GLOBALS(v8js) +struct _v8js_process_globals v8js_process_globals; /* {{{ INI Settings */ @@ -118,6 +119,16 @@ PHP_MINIT_FUNCTION(v8js) static PHP_MSHUTDOWN_FUNCTION(v8js) { UNREGISTER_INI_ENTRIES(); + + if(v8js_process_globals.v8_initialized) { + v8::V8::Dispose(); +#if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036 + v8::V8::ShutdownPlatform(); + // @fixme call virtual destructor somehow + //delete v8js_process_globals.v8_platform; +#endif + } + return SUCCESS; } /* }}} */ @@ -202,14 +213,6 @@ static PHP_GSHUTDOWN_FUNCTION(v8js) v8js_globals->timer_stack.~deque(); v8js_globals->timer_mutex.~mutex(); #endif - - if (v8js_globals->v8_initialized) { - v8::V8::Dispose(); -#if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036 - v8::V8::ShutdownPlatform(); - delete v8js_globals->v8_platform; -#endif - } } /* }}} */ diff --git a/v8js_v8.cc b/v8js_v8.cc index a72376c..7206d20 100644 --- a/v8js_v8.cc +++ b/v8js_v8.cc @@ -37,14 +37,27 @@ extern "C" { void v8js_v8_init(TSRMLS_D) /* {{{ */ { - /* Run only once */ + /* Run only once; thread-local test first */ if (V8JSG(v8_initialized)) { return; } + /* Set thread-local flag, that V8 was initialized. */ + V8JSG(v8_initialized) = 1; + +#ifdef ZTS + v8js_process_globals.lock.lock(); + + if(v8js_process_globals.v8_initialized) { + /* V8 already has been initialized by another thread */ + v8js_process_globals.lock.unlock(); + return; + } +#endif + #if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036 - V8JSG(v8_platform) = v8::platform::CreateDefaultPlatform(); - v8::V8::InitializePlatform(V8JSG(v8_platform)); + v8js_process_globals.v8_platform = v8::platform::CreateDefaultPlatform(); + v8::V8::InitializePlatform(v8js_process_globals.v8_platform); #endif /* Set V8 command line flags (must be done before V8::Initialize()!) */ @@ -55,8 +68,10 @@ void v8js_v8_init(TSRMLS_D) /* {{{ */ /* Initialize V8 */ v8::V8::Initialize(); - /* Run only once */ - V8JSG(v8_initialized) = 1; +#ifdef ZTS + v8js_process_globals.v8_initialized = 1; + v8js_process_globals.lock.unlock(); +#endif } /* }}} */ From 74440ed9f743d69e2358cbd222d3e942e7acb68c Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 26 Sep 2015 21:39:26 +0200 Subject: [PATCH 2/4] Move V8JSG extensions and v8_flags to process globals --- php_v8js_macros.h | 9 ++++++-- v8js.cc | 52 +++++++++++++++++++++++++++++++---------------- v8js_class.cc | 47 ++++++++++++++++++++++++++++++++---------- v8js_v8.cc | 5 +++-- 4 files changed, 81 insertions(+), 32 deletions(-) diff --git a/php_v8js_macros.h b/php_v8js_macros.h index 2f8c2a5..9b8b980 100644 --- a/php_v8js_macros.h +++ b/php_v8js_macros.h @@ -113,10 +113,8 @@ void v8js_register_accessors(std::vector *accessor_list, v8: ZEND_BEGIN_MODULE_GLOBALS(v8js) // Thread-local cache whether V8 has been initialized so far int v8_initialized; - HashTable *extensions; /* Ini globals */ - char *v8_flags; /* V8 command line flags */ bool use_date; /* Generate JS Date objects instead of PHP DateTime */ bool use_array_access; /* Convert ArrayAccess, Countable objects to array-like objects */ bool compat_php_exceptions; /* Don't stop JS execution on PHP exception */ @@ -149,6 +147,8 @@ ZEND_EXTERN_MODULE_GLOBALS(v8js) * * - whether V8 has been initialized at all * - the V8 backend platform + * - loaded extensions + * - V8 "command line" flags * * In a ZTS-enabled environment access to all of these variables must happen * while holding a mutex lock. @@ -159,6 +159,11 @@ struct _v8js_process_globals { std::mutex lock; #endif + HashTable *extensions; + + /* V8 command line flags */ + char *v8_flags; + #if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036 v8::Platform *v8_platform; #endif diff --git a/v8js.cc b/v8js.cc index 25499e0..9e37de7 100755 --- a/v8js.cc +++ b/v8js.cc @@ -35,15 +35,35 @@ struct _v8js_process_globals v8js_process_globals; static ZEND_INI_MH(v8js_OnUpdateV8Flags) /* {{{ */ { + bool immutable = false; + +#ifdef ZTS + v8js_process_globals.lock.lock(); + + if(v8js_process_globals.v8_initialized) { + v8js_process_globals.lock.unlock(); + immutable = true; + } + + v8js_process_globals.lock.unlock(); +#else + immutable = V8JSG(v8_initialized); +#endif + + if(immutable) { + /* V8 already has been initialized -> cannot be changed anymore */ + return FAILURE; + } + if (new_value) { - if (V8JSG(v8_flags)) { - free(V8JSG(v8_flags)); - V8JSG(v8_flags) = NULL; + if (v8js_process_globals.v8_flags) { + free(v8js_process_globals.v8_flags); + v8js_process_globals.v8_flags = NULL; } if (!new_value[0]) { return FAILURE; } - V8JSG(v8_flags) = zend_strndup(new_value, new_value_length); + v8js_process_globals.v8_flags = zend_strndup(new_value, new_value_length); } return SUCCESS; @@ -129,6 +149,17 @@ static PHP_MSHUTDOWN_FUNCTION(v8js) #endif } + if (v8js_process_globals.v8_flags) { + free(v8js_process_globals.v8_flags); + v8js_process_globals.v8_flags = NULL; + } + + if (v8js_process_globals.extensions) { + zend_hash_destroy(v8js_process_globals.extensions); + free(v8js_process_globals.extensions); + v8js_process_globals.extensions = NULL; + } + return SUCCESS; } /* }}} */ @@ -180,9 +211,7 @@ static PHP_GINIT_FUNCTION(v8js) run the destructors manually. */ #ifdef ZTS - v8js_globals->extensions = NULL; v8js_globals->v8_initialized = 0; - v8js_globals->v8_flags = NULL; v8js_globals->timer_thread = NULL; v8js_globals->timer_stop = false; @@ -198,17 +227,6 @@ static PHP_GINIT_FUNCTION(v8js) */ static PHP_GSHUTDOWN_FUNCTION(v8js) { - if (v8js_globals->extensions) { - zend_hash_destroy(v8js_globals->extensions); - free(v8js_globals->extensions); - v8js_globals->extensions = NULL; - } - - if (v8js_globals->v8_flags) { - free(v8js_globals->v8_flags); - v8js_globals->v8_flags = NULL; - } - #ifdef ZTS v8js_globals->timer_stack.~deque(); v8js_globals->timer_mutex.~mutex(); diff --git a/v8js_class.cc b/v8js_class.cc index 7eaccbd..96c7bed 100644 --- a/v8js_class.cc +++ b/v8js_class.cc @@ -827,10 +827,17 @@ static int v8js_register_extension(char *name, uint name_len, char *source, uint { v8js_jsext *jsext = NULL; - if (!V8JSG(extensions)) { - V8JSG(extensions) = (HashTable *) malloc(sizeof(HashTable)); - zend_hash_init(V8JSG(extensions), 1, NULL, (dtor_func_t) v8js_jsext_dtor, 1); - } else if (zend_hash_exists(V8JSG(extensions), name, name_len + 1)) { +#ifdef ZTS + v8js_process_globals.lock.lock(); +#endif + + if (!v8js_process_globals.extensions) { + v8js_process_globals.extensions = (HashTable *) malloc(sizeof(HashTable)); + zend_hash_init(v8js_process_globals.extensions, 1, NULL, (dtor_func_t) v8js_jsext_dtor, 1); + } else if (zend_hash_exists(v8js_process_globals.extensions, name, name_len + 1)) { +#ifdef ZTS + v8js_process_globals.lock.unlock(); +#endif return FAILURE; } @@ -843,6 +850,9 @@ static int v8js_register_extension(char *name, uint name_len, char *source, uint php_error_docref(NULL TSRMLS_CC, E_WARNING, "Invalid dependency array passed"); v8js_jsext_dtor(jsext); free(jsext); +#ifdef ZTS + v8js_process_globals.lock.unlock(); +#endif return FAILURE; } } @@ -859,17 +869,23 @@ static int v8js_register_extension(char *name, uint name_len, char *source, uint jsext->extension = new v8::Extension(jsext->name, jsext->source, jsext->deps_count, jsext->deps); - if (zend_hash_add(V8JSG(extensions), name, name_len + 1, jsext, sizeof(v8js_jsext), NULL) == FAILURE) { + if (zend_hash_add(v8js_process_globals.extensions, name, name_len + 1, jsext, sizeof(v8js_jsext), NULL) == FAILURE) { v8js_jsext_dtor(jsext); free(jsext); +#ifdef ZTS + v8js_process_globals.lock.unlock(); +#endif return FAILURE; } +#ifdef ZTS + v8js_process_globals.lock.unlock(); +#endif + jsext->extension->set_auto_enable(auto_enable ? true : false); v8::RegisterExtension(jsext->extension); free(jsext); - return SUCCESS; } /* }}} */ @@ -916,10 +932,15 @@ static PHP_METHOD(V8Js, getExtensions) } array_init(return_value); - if (V8JSG(extensions)) { - zend_hash_internal_pointer_reset_ex(V8JSG(extensions), &pos); - while (zend_hash_get_current_data_ex(V8JSG(extensions), (void **) &jsext, &pos) == SUCCESS) { - if (zend_hash_get_current_key_ex(V8JSG(extensions), &key, &key_len, &index, 0, &pos) == HASH_KEY_IS_STRING) { + +#ifdef ZTS + v8js_process_globals.lock.lock(); +#endif + + if (v8js_process_globals.extensions) { + zend_hash_internal_pointer_reset_ex(v8js_process_globals.extensions, &pos); + while (zend_hash_get_current_data_ex(v8js_process_globals.extensions, (void **) &jsext, &pos) == SUCCESS) { + if (zend_hash_get_current_key_ex(v8js_process_globals.extensions, &key, &key_len, &index, 0, &pos) == HASH_KEY_IS_STRING) { MAKE_STD_ZVAL(ext) array_init(ext); add_assoc_bool_ex(ext, ZEND_STRS("auto_enable"), jsext->auto_enable); @@ -931,9 +952,13 @@ static PHP_METHOD(V8Js, getExtensions) } add_assoc_zval_ex(return_value, key, key_len, ext); } - zend_hash_move_forward_ex(V8JSG(extensions), &pos); + zend_hash_move_forward_ex(v8js_process_globals.extensions, &pos); } } + +#ifdef ZTS + v8js_process_globals.lock.unlock(); +#endif } /* }}} */ diff --git a/v8js_v8.cc b/v8js_v8.cc index 7206d20..bc1f3e5 100644 --- a/v8js_v8.cc +++ b/v8js_v8.cc @@ -61,8 +61,9 @@ void v8js_v8_init(TSRMLS_D) /* {{{ */ #endif /* Set V8 command line flags (must be done before V8::Initialize()!) */ - if (V8JSG(v8_flags)) { - v8::V8::SetFlagsFromString(V8JSG(v8_flags), strlen(V8JSG(v8_flags))); + if (v8js_process_globals.v8_flags) { + v8::V8::SetFlagsFromString(v8js_process_globals.v8_flags, + strlen(v8js_process_globals.v8_flags)); } /* Initialize V8 */ From 34ca8500b11fc775ab19f0213be4c0cb99ed06a7 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 26 Sep 2015 21:45:12 +0200 Subject: [PATCH 3/4] Remove 'not tested' note regarding ZTS --- README.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/README.md b/README.md index 3514abe..566ce8d 100644 --- a/README.md +++ b/README.md @@ -22,8 +22,7 @@ Minimum requirements - PHP 5.3.3+ - This embedded implementation of the V8 engine uses thread locking so it should work with ZTS enabled. - However, this has not been tested yet. + This embedded implementation of the V8 engine uses thread locking so it works with ZTS enabled. Compiling latest version From b292715c75ddc413cb2ff6596559378c64db4000 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sun, 27 Sep 2015 09:25:58 +0200 Subject: [PATCH 4/4] Fix non-ZTS build. --- v8js.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/v8js.cc b/v8js.cc index 9e37de7..272ce87 100755 --- a/v8js.cc +++ b/v8js.cc @@ -140,7 +140,15 @@ static PHP_MSHUTDOWN_FUNCTION(v8js) { UNREGISTER_INI_ENTRIES(); - if(v8js_process_globals.v8_initialized) { + bool v8_initialized; + +#ifdef ZTS + v8_initialized = v8js_process_globals.v8_initialized; +#else + v8_initialized = V8JSG(v8_initialized); +#endif + + if(v8_initialized) { v8::V8::Dispose(); #if !defined(_WIN32) && PHP_V8_API_VERSION >= 3029036 v8::V8::ShutdownPlatform();