Interned strings unification for TS/NTS #2390

weltling · 2017-02-17T12:15:36Z

The primary goal is to support real interned strings also in Ts builds. The unified approach implements string interning with two tables for permanent and per-request interned strings for both TS/NTS. More explanations to the patch here http://news.php.net/php.internals/98324

Thanks.

* php/master: (149 commits) Fixed bug #61471 Add #ifndef restrict Fix detection of isnan and isinf ReflectionGenerator now sends ReflectionException as expected use some dynamically generated NAN as well rework fd521a2 to simplify for master, see github php#2356 switch to smart str conversion routine to hide exact NAN type Revert "Fix detection of isnan and isinf" Fix detection of isnan and isinf Fix bug #73954 Implement Parameter Type Widening RFC Add UPGRADING notes for deprecations Deprecate each() Deprecate assert() with string argument Deprecate mbstring.func_overload Deprecate track_errors / $php_errormsg Deprecate mb_parse_str() without second argument Deprecate parse_str() without second argument Deprecate (unset) Deprecate __autoload() ...

- implemented separate hash tables for permanent and per request interned strings - strings interned during the startup are permanent - afterwards, any interned string has life time per request only In the thread safe build this means, every thread maintains its own hash table for the non permanent interned strings. The permanent strings are common to all the threads and won't be free'd until the process shutdown.

* php/master: (44 commits) [ci skip] update NEWS [ci skip] update NEWS fix typo skip test Fixed bug #74090 stream_get_contents maxlength>-1 returns empty string Fixed test in travis Make it slower Fixed test Add a test for hard_timeout(bug #74093) Workaround to fix bug #74093 (Maximum execution time of n+2 seconds exceed not written in error_log) Update to SQLite 3.17.0 Update NEWS Fixed bug #73989 (PHP 7.1 Segfaults within Symfony test suite) update NEWS update UPGRADING.INTERNALS The d_name member of struct dirent should be a pointer Revert "Fixed bug #74035" Upgrade timelib to 2017.01 remove loop fix loop ...

ensure clean alloc/dealloc

dstogov · 2017-02-17T12:37:54Z

main/streams/streams.c

@@ -1706,7 +1706,7 @@ PHPAPI int php_register_url_stream_wrapper(const char *protocol, php_stream_wrap
 		return FAILURE;
 	}

-	return zend_hash_str_add_ptr(&url_stream_wrappers_hash, protocol, protocol_len, wrapper) ? SUCCESS : FAILURE;
+	return zend_hash_add_ptr(&url_stream_wrappers_hash, zend_string_init_interned(protocol, protocol_len, 1), wrapper) ? SUCCESS : FAILURE;


This place may be reached at run-time through stream_wrapper_register(), but we don't create interned strings at run-time.

I was basing it on the fact url_stream_wrappers_hash is used, which is global. Otheriwse, stream wrappers created on runtime seem to be not thread safe in general. Gonna investigate and rework this.

Seems this part is ok. User space wrappers runs through php_register_url_stream_wrapper_volatile() and save into FG(stream_wrappers). So far i couldn't find a code path where php_register_url_stream_wrapper() is called outside the startup phase. Thus names of those saved into url_stream_wrappers_hash need to be interned, as they can interact with the runtime code.

laruence · 2017-02-18T12:19:39Z

Zend/zend_string.c

-#endif
+	empty_string = zend_new_interned_string_permanent_int(str);
+#ifndef ZTS
+	CG(empty_string) = empty_string;


since empty_string is a true global variable, seems we don't need to store it in CG .

Indeed, it would then even replace the current CG reference in ZSTR_EMPTY_ALLOC(), just need to extern it. I'll do this in the scope of this patch. Would have to check with Opcache yet, missed it in the initial patch.

laruence · 2017-02-19T07:45:44Z

TSRM/TSRM.c

+		/* ensure singleton */
+		return;
+	}
+
 	if (tsrm_tls_table) {


as we already have a in_main_thread guarder, this check should also not that necessary, right?

I'll check this, thanks for the hint. Thought also to export this as a function, so it's usable in any places with startup/shutdown, maybe improving to explicitly checking thread ids.

This seems ok, i've reverted for now as it's not related to interned strings directly. Will re-apply once the actual interned strings stuff is finished.

this part is shm protected

This avoids shutdown crashes with Opcache, basically because function/class/etc. table is already cleaned up by the Opcache, so the global pointers need to match when zend_shutdown tries to free stuff.

* php/master: fix test for libzip 1.2.0 initialize valid_symbol_table, important for the main thread ftp_mlsd(): Parse the MLSD response Fixed #74099 - Memory leak with openssl_encrypt() Fixed bug #74105 Unused var Disable RTLD_DEEPBIND when compiling with AddressSanitizer (-fsanitize=address). Fix memory errors in url rewriter Fix autoload_func_info destruction Make the ftp and stream tests more reliable. Add json dep to test

dstogov · 2017-02-21T08:45:27Z

TSRM/TSRM.c

@@ -10,7 +10,7 @@
   +----------------------------------------------------------------------+
 */

-#include "TSRM.h"
+#include "zend.h"


TSRM didn't depend on Zend by design. Can we avoid this?

We could introduce more callbacks in TSRM, currently there are only a couple of callbacks at thread creation. Some callbacks could be introduced for the tsrm startup/shutdown and thread shutdown. I'll do the callbacks for now, for cleaner TSRM API.

dstogov · 2017-02-21T08:53:48Z

TSRM/TSRM.c

+	if (!in_main_thread) {
+		/* ensure singleton */
+		return;
+	}


Changes/fix related to "tsrm_tls_table" and "in_main_thread" look unrelated to interned strings. Lets commit this part separately.

ack, will revert for now.

dstogov · 2017-02-21T08:56:19Z

Zend/zend.c

@@ -623,7 +619,7 @@ static void executor_globals_ctor(zend_executor_globals *executor_globals) /* {{
 	ZEND_TSRMLS_CACHE_UPDATE();

 	zend_startup_constants();
-	zend_copy_constants(EG(zend_constants), GLOBAL_CONSTANTS_TABLE);
+	zend_copy_constants(executor_globals->zend_constants, GLOBAL_CONSTANTS_TABLE);


This change is unrelated. Commit it separately.

dstogov · 2017-02-21T09:01:10Z

Zend/zend.c

 	compiler_globals->auto_globals = GLOBAL_AUTO_GLOBALS_TABLE;

 	zend_hash_destroy(executor_globals->zend_constants);
-	*executor_globals->zend_constants = *GLOBAL_CONSTANTS_TABLE;
+	executor_globals->zend_constants = GLOBAL_CONSTANTS_TABLE;


I'm not sure about all these zend_startup() changes above.

This reverts commit 32d72b3.

This reverts commit 5f03243.

…ld logic"" This reverts commit b2b7a56.

* php/master: (22 commits) do not try to handle signals, when globals are inconsistent Zip: add support for encrypted archive PHP bug #74004 Fixed tests after tzdb removed abbreviations Make sure we anchor this test to February Add test-case from bug #55157 Fix bug73858.phpt to work in months without 31 days Update NEWS Update NEWs Fixed bug #54379 (PDO_OCI: UTF-8 output gets truncated) Remove dead code related to error constants Remove PHP5-specific code Updated to version 2017.1 (2017a) Updated to version 2017.1 (2017a) Updated to version 2017.1 (2017a) make type consistent with glob_t.gl_pathc Fix potential crash when setting invalid declare value make test slower again prepare 7.1.4 prepare next ...

nikic · 2017-03-02T21:22:23Z

Zend/zend_API.c

@@ -2019,7 +2019,7 @@ ZEND_API zend_module_entry* zend_register_module_ex(zend_module_entry *module) /
 	lcname = zend_string_alloc(name_len, 1);
 	zend_str_tolower_copy(ZSTR_VAL(lcname), module->name, name_len);

-	if ((module_ptr = zend_hash_add_mem(&module_registry, lcname, module, sizeof(zend_module_entry))) == NULL) {
+	if ((module_ptr = zend_hash_add_mem(&module_registry, zend_new_interned_string(lcname), module, sizeof(zend_module_entry))) == NULL) {


This should be lcname = zend_new_interned_string(lcname) on a separate line. The semantics of zend_new_interned_string are similar to realloc. The argument is released, you just sometimes don't see it (if the string was not interned yet).

nikic · 2017-03-02T21:23:41Z

Zend/zend_builtin_functions.c

@@ -763,7 +763,7 @@ ZEND_FUNCTION(each)
 		if (Z_REFCOUNTED_P(entry)) Z_ADDREF_P(entry);
 	}
 	zend_hash_index_add_new(Z_ARRVAL_P(return_value), 1, entry);
-	zend_hash_add_new(Z_ARRVAL_P(return_value), CG(known_strings)[ZEND_STR_VALUE], entry);
+	zend_hash_add_new(Z_ARRVAL_P(return_value), ZSTR_KNOWN(ZEND_STR_VALUE), entry);


Is it possible to apply these changes changes independently? (Only if it's not too entangled.)

CG(known_strings) is now removed, instead it's a real global var zend_known_strings. Same is with zend_empty_string and zend_one_char_string. So we would reference a global var directly, which is worse than having an acessor macro. But in general, it is a a change that belongs to the patch, so either way it were not possible to keep the exact current code.

nikic · 2017-03-02T21:28:25Z

Zend/zend_string.c

+	interned_strings->nTableMask = -interned_strings->nTableSize;
+	HT_SET_DATA_ADDR(interned_strings, pemalloc(HT_SIZE(interned_strings), permanent));
+	HT_HASH_RESET(interned_strings);
+	interned_strings->u.flags |= HASH_FLAG_INITIALIZED;


Isn't this code just zend_hash_real_init(interned_strings, 0);?

nikic · 2017-03-02T21:32:43Z

Zend/zend_string.c

+			memcpy(interned_strings->arData, old_buckets, sizeof(Bucket) * interned_strings->nNumUsed);
+			pefree(old_data, interned_strings->u.flags & HASH_FLAG_PERSISTENT);
+			zend_hash_rehash(interned_strings);
+		} else {


With pemalloc this branch can never be taken.

nikic · 2017-03-02T21:33:50Z

Zend/zend_string.c

+
+static zend_never_inline void zend_string_table_grow(HashTable *interned_strings)
+{
+	if (EXPECTED(interned_strings->nTableSize < HT_MAX_SIZE)) {	/* Let's double the table size */


I know this is existing code, but the implementation as-is cannot actually handle this condition gracefully. Either it should error as the HT would usually do, or it should not intern in that case.

Probably it should fail hard in this situation, as not interning might cause more hard weight issues, at least with TS. Alternatively, we could increase the value, if possible.

nikic · 2017-03-02T21:35:53Z

Zend/zend_string.c

-				CG(interned_strings).nTableSize = CG(interned_strings).nTableSize >> 1;
-				CG(interned_strings).nTableMask = -CG(interned_strings).nTableSize;
-			}
+	return NULL;


As the interned string is stored as both key and value of the HT (https://github.com/weltling/php-src/blob/97882476c602a20411c33d0d5c555818ba5e5834/Zend/zend_string.c#L176) this function should be a thin wrapper around zend_hash_find(), instead of reimplementing HT logic.

nikic · 2017-03-02T21:40:56Z

Zend/zend_string.c

-	HT_HASH(&CG(interned_strings), nIndex) = HT_IDX_TO_HASH(idx);
+	nIndex = h | interned_strings->nTableMask;
+	Z_NEXT(p->val) = HT_HASH(interned_strings, nIndex);
+	HT_HASH(interned_strings, nIndex) = HT_IDX_TO_HASH(idx);


This could use zend_hash_append(). Or going one step further, this could just use zend_hash_add_new(), in which case we can discard the duplicated table growing logic as well.

It looks like the only not addressed points regard to the interned strings ht management. I would address them after the patch was merged. It would be indeed make sense to reuse the existing API, for now we're fine reusing the existing implementation.

Sounds good. Patch LGTM, very nice work!

* php/master: sync NEWS for bug #74159 improve signal globals consistency check for TS

nikic · 2017-03-02T21:45:31Z

ext/opcache/ZendAccelerator.c

 /* Copy PHP interned strings from PHP process memory into the shared memory */
-static void accel_use_shm_interned_strings(void)
+static void accel_copy_permanent_strings(zend_new_interned_string_func_t new_interned_string)


Just to clarify: Copying to SHM here is just an optimization right? For cross-process purposes it's only important that strings referenced from SHM are interned, not that permanent strings are all in SHM. If something is missed here, it should not cause issues. Is that correct?

On ASLR enabled systems, like Windows by default or hardened Linux, pointers to interned strings are likely differ. So in first place, by copying we ensure the pointers are same in all the processes. If something is missed here, the interning will happen on demand in zend_persist.c and zend_persist_calc.c parts on demand. See ADD_INTERNED_STRING, zend_accel_store_interned_string and so on there.

It might be, that on some systems the pointers to the local interned strings could stay same, But as we can't guarantee it, it's played safe. While it were possible to add a configure option and modify the code for not saving to SHM, i'd not go for that, as it is really not a reliable behavior.

nikic · 2017-03-02T21:46:04Z

ext/opcache/ZendAccelerator.c

+			p->key = new_interned_string(p->key);
+		}
+		c = (zend_constant*)Z_PTR(p->val);
+		if (c->name) {


Are there constants without names?

Probably not a usual case. Some tricks i recall were like giving a name with a \0-byte in the front, to save some thing unusual into the constant. Such code is for sure still somewhere in APCu and earlier in APC, probably in some other exts as well.

nikic · 2017-03-02T21:47:11Z

ext/opcache/ZendAccelerator.c

+		}
+	}
+
+	for (idx = 0; idx < module_registry.nNumUsed; idx++) {


All of this code should be using ZEND_HASH_FOREACH_BUCKET... but not related to this change.

dstogov · 2017-03-03T07:07:12Z

We have to copy permanent interned strings into SHM, because on Windows different processes may put the same strings at different addresses.

________________________________ From: Nikita Popov <notifications@github.com> Sent: Friday, March 3, 2017 12:45:54 AM To: php/php-src Cc: Dmitry Stogov; Comment Subject: Re: [php/php-src] Interned strings unification for TS/NTS (#2390) @nikic commented on this pull request.

________________________________ In ext/opcache/ZendAccelerator.c<#2390 (comment)>:

/* Copy PHP interned strings from PHP process memory into the shared memory */

-static void accel_use_shm_interned_strings(void) +static void accel_copy_permanent_strings(zend_new_interned_string_func_t new_interned_string) Just to clarify: Copying to SHM here is just an optimization right? For cross-process purposes it's only important that strings referenced from SHM are interned, not that permanent strings are all in SHM. If something is missed here, it should not cause issues. Is that correct? — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#2390 (review)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACZM0nEuHBvRQBpb_VfqTT7WZ_k2Su5Oks5rhziSgaJpZM4MEM_p>.

weltling · 2017-03-04T11:04:03Z

Merged per c698299. The fololwups are going to be handled either directly in dev or per a separate PR.

Thanks!

weltling added 12 commits February 8, 2017 12:25

stream wrapper names need to be permanent

38555f1

improve naming, add comment

872a92a

provide a way to override the persistent interned string storage

77b8510

fix typo

9ee5afb

change comment

15bb3fa

don't pass zend_string from outside, the ini item name should be used to

65b36aa

ensure clean alloc/dealloc

replace the function pointer instead of using a flag

1aaede8

basic approach for proper permanent strings init/dtor

5540225

remove commented out obsolete line

5e85b81

dstogov reviewed Feb 17, 2017

View reviewed changes

weltling added 2 commits February 17, 2017 20:30

remove ZTS wraps

68288c0

remove more ZTS wrappers

e34b7b5

laruence reviewed Feb 18, 2017

View reviewed changes

laruence reviewed Feb 19, 2017

View reviewed changes

weltling added 4 commits February 19, 2017 23:19

enable one more zts wrapping part

34890e3

this part is shm protected

Refactor globals startup/shutdown in the ts main thread

205b241

This avoids shutdown crashes with Opcache, basically because function/class/etc. table is already cleaned up by the Opcache, so the global pointers need to match when zend_shutdown tries to free stuff.

yet more rework for up/down with Opcache

7df7a0b

do not do blanket destroy, only replace tables of interest

261cc1b

krakjoe added the Feature label Feb 20, 2017

weltling added 4 commits February 20, 2017 15:54

integrate new interned strings handling with phpdbg

166ef86

ermove CG(empty_sting), make empty_string available directly

673802e

remove unnecessary check

0f61127

dstogov reviewed Feb 21, 2017

View reviewed changes

weltling and others added 14 commits February 28, 2017 16:30

init Opcache interned string handlers on reattach

8003f07

add asserts, accel interned strings should be always permanent

5f03243

Keep IS_STR_PERMANENT for SHM interned strings.

75451a3

extend SHM string set on demand in reattached process

32d72b3

Revert "extend SHM string set on demand in reattached process"

fe03d82

This reverts commit 32d72b3.

Revert "add asserts, accel interned strings should be always permanent"

27e6d22

This reverts commit 5f03243.

Revert "Revert "fix persist size calculation, basically restore the o…

a5b6f3d

…ld logic"" This reverts commit b2b7a56.

remove the assert, continue to use the process permanent string

aabddc0

reset ZCG(counter) after the SHM lookups are finished

bba494a

accel intern the module names yet, as they're interned locally

b5fb88d

Fixed file cache

dd1bd4d

Always inline hash lookup

b470521

Don't intern string at run-time

2d560f5

nikic reviewed Mar 2, 2017

View reviewed changes

Merge remote-tracking branch 'php/master' into ts_interned_strings

05a6576

* php/master: sync NEWS for bug #74159 improve signal globals consistency check for TS

nikic reviewed Mar 2, 2017

View reviewed changes

weltling added 2 commits March 2, 2017 22:49

fix semantics for zend_new_interned_string call

4e30022

remove void condition

7217fb0

weltling closed this Mar 4, 2017

Interned strings unification for TS/NTS #2390

Interned strings unification for TS/NTS #2390

Uh oh!

Conversation

weltling commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltling Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstogov commented Mar 3, 2017 via email

Uh oh!

weltling commented Mar 4, 2017

Uh oh!

Uh oh!

weltling commented Feb 17, 2017 •

edited

Loading

weltling Mar 2, 2017 •

edited

Loading