diff --git a/testdata/persist-roundtrip.php b/testdata/persist-roundtrip.php index f2bcec0627..51388a27e7 100644 --- a/testdata/persist-roundtrip.php +++ b/testdata/persist-roundtrip.php @@ -4,6 +4,8 @@ // FRANKENPHP_TEST_HOOKS is defined at build time). The script runs as a // plain non-worker request; the Go harness asserts the combined output. +const CONST_ARR = ['a' => 1, 'b' => [2, 3], 'c' => 'literal']; + enum Status: string { case Active = 'active'; case Paused = 'paused'; @@ -65,6 +67,14 @@ function same(mixed $actual, mixed $expected, string $label): void { 'array with enum', ); +// Compile-time constant array. Opcache marks these immutable and keeps them +// in shared memory, so persist must copy instead of sharing the pointer. +same($rt(CONST_ARR), CONST_ARR, 'const array'); + +// Literal keys and values are interned strings, which also live in opcache +// shared memory once the script is cached. +same($rt(['literal_key' => 'literal_value']), ['literal_key' => 'literal_value'], 'interned key and value'); + // Invalid inputs throw LogicException. try { $rt(new stdClass()); diff --git a/zval.h b/zval.h index cccdba31ad..c18706a7f6 100644 --- a/zval.h +++ b/zval.h @@ -5,9 +5,12 @@ * and enums. Everything else is rejected by persistent_zval_validate so * callers can fail fast before allocating. * - * Fast paths: - * - Interned strings: shared memory, no copy. - * - Opcache-immutable arrays: shared pointer, no copy, no free. + * Everything reachable from a persisted tree is allocated with pemalloc and + * owned by that tree. Nothing is borrowed from opcache. Interned strings and + * immutable arrays live in opcache's shared memory, which is rewound to a + * startup watermark on restart (out of memory, hash overflow, or + * opcache_reset()), so a tree that outlives a restart would be left pointing + * at reused memory. * * Included by frankenphp.c; not a standalone compilation unit. */ @@ -39,12 +42,6 @@ static bool persistent_zval_validate(zval *z) { case IS_OBJECT: return (Z_OBJCE_P(z)->ce_flags & ZEND_ACC_ENUM) != 0; case IS_ARRAY: { - /* Opcache-immutable arrays are compile-time constants in shared - * memory; their leaves are guaranteed scalars or further immutable - * arrays. The copy/free paths below already trust this flag, so a - * recursive walk here would just be cycles. */ - if ((GC_FLAGS(Z_ARRVAL_P(z)) & IS_ARRAY_IMMUTABLE) != 0) - return true; zval *val; ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(z), val) { if (!persistent_zval_validate(val)) @@ -78,40 +75,22 @@ static void persistent_zval_persist(zval *dst, zval *src) { case IS_DOUBLE: ZVAL_DOUBLE(dst, Z_DVAL_P(src)); break; - case IS_STRING: { - zend_string *s = Z_STR_P(src); - if (ZSTR_IS_INTERNED(s)) { - ZVAL_STR(dst, s); /* interned strings live process-wide */ - } else { - ZVAL_NEW_STR(dst, zend_string_init(ZSTR_VAL(s), ZSTR_LEN(s), 1)); - } + case IS_STRING: + ZVAL_NEW_STR(dst, zend_string_init(Z_STRVAL_P(src), Z_STRLEN_P(src), 1)); break; - } case IS_OBJECT: { /* Must be an enum (validated earlier in persistent_zval_validate). */ zend_class_entry *ce = Z_OBJCE_P(src); persistent_zval_enum_t *e = pemalloc(sizeof(*e), 1); - e->class_name = - ZSTR_IS_INTERNED(ce->name) - ? ce->name - : zend_string_init(ZSTR_VAL(ce->name), ZSTR_LEN(ce->name), 1); + e->class_name = zend_string_init(ZSTR_VAL(ce->name), ZSTR_LEN(ce->name), 1); zval *case_name_zval = zend_enum_fetch_case_name(Z_OBJ_P(src)); zend_string *case_str = Z_STR_P(case_name_zval); - e->case_name = - ZSTR_IS_INTERNED(case_str) - ? case_str - : zend_string_init(ZSTR_VAL(case_str), ZSTR_LEN(case_str), 1); + e->case_name = zend_string_init(ZSTR_VAL(case_str), ZSTR_LEN(case_str), 1); ZVAL_PTR(dst, e); break; } case IS_ARRAY: { HashTable *src_ht = Z_ARRVAL_P(src); - if ((GC_FLAGS(src_ht) & IS_ARRAY_IMMUTABLE) != 0) { - /* Opcache-immutable arrays live for the process lifetime and are - * safe to share across threads by pointer. Zero-copy, zero-free. */ - ZVAL_ARR(dst, src_ht); - break; - } HashTable *dst_ht = pemalloc(sizeof(HashTable), 1); zend_hash_init(dst_ht, zend_hash_num_elements(src_ht), NULL, NULL, 1); ZVAL_ARR(dst, dst_ht); @@ -123,16 +102,12 @@ static void persistent_zval_persist(zval *dst, zval *src) { zval pval; persistent_zval_persist(&pval, val); if (key) { - if (ZSTR_IS_INTERNED(key)) { - zend_hash_add_new(dst_ht, key, &pval); - } else { - zend_string *pkey = zend_string_init(ZSTR_VAL(key), ZSTR_LEN(key), 1); - /* Iteration guarantees the source key has its hash set. - * Propagating it lets zend_hash_add_new skip the re-hash. */ - ZSTR_H(pkey) = ZSTR_H(key); - zend_hash_add_new(dst_ht, pkey, &pval); - zend_string_release(pkey); - } + zend_string *pkey = zend_string_init(ZSTR_VAL(key), ZSTR_LEN(key), 1); + /* Iteration guarantees the source key has its hash set. + * Propagating it lets zend_hash_add_new skip the re-hash. */ + ZSTR_H(pkey) = ZSTR_H(key); + zend_hash_add_new(dst_ht, pkey, &pval); + zend_string_release(pkey); } else { zend_hash_index_add_new(dst_ht, idx, &pval); } @@ -146,33 +121,26 @@ static void persistent_zval_persist(zval *dst, zval *src) { } } -/* Deep-free a persistent zval tree. Idempotent on scalars. Skips - * interned strings and immutable arrays (they are borrowed, not owned). */ +/* Deep-free a persistent zval tree. Idempotent on scalars. */ static void persistent_zval_free(zval *z) { switch (Z_TYPE_P(z)) { case IS_STRING: - if (!ZSTR_IS_INTERNED(Z_STR_P(z))) { - zend_string_free(Z_STR_P(z)); - } + zend_string_free(Z_STR_P(z)); break; case IS_PTR: { persistent_zval_enum_t *e = (persistent_zval_enum_t *)Z_PTR_P(z); - if (!ZSTR_IS_INTERNED(e->class_name)) - zend_string_free(e->class_name); - if (!ZSTR_IS_INTERNED(e->case_name)) - zend_string_free(e->case_name); + zend_string_free(e->class_name); + zend_string_free(e->case_name); pefree(e, 1); break; } case IS_ARRAY: { HashTable *ht = Z_ARRVAL_P(z); - if ((GC_FLAGS(ht) & IS_ARRAY_IMMUTABLE) != 0) { - /* Borrowed from opcache, do not touch. */ - break; - } zval *val; ZEND_HASH_FOREACH_VAL(ht, val) { persistent_zval_free(val); } ZEND_HASH_FOREACH_END(); + /* zend_hash_destroy releases the string keys; the values have no + * destructor and were freed above. */ zend_hash_destroy(ht); pefree(ht, 1); break; @@ -200,11 +168,7 @@ static void persistent_zval_to_request(zval *dst, zval *src) { ZVAL_DOUBLE(dst, Z_DVAL_P(src)); break; case IS_STRING: - if (ZSTR_IS_INTERNED(Z_STR_P(src))) { - ZVAL_STR(dst, Z_STR_P(src)); - } else { - ZVAL_STRINGL(dst, Z_STRVAL_P(src), Z_STRLEN_P(src)); - } + ZVAL_STRINGL(dst, Z_STRVAL_P(src), Z_STRLEN_P(src)); break; case IS_PTR: { persistent_zval_enum_t *e = (persistent_zval_enum_t *)Z_PTR_P(src); @@ -234,11 +198,6 @@ static void persistent_zval_to_request(zval *dst, zval *src) { } case IS_ARRAY: { HashTable *src_ht = Z_ARRVAL_P(src); - if ((GC_FLAGS(src_ht) & IS_ARRAY_IMMUTABLE) != 0) { - /* Zero-copy: immutable arrays are safe to expose directly. */ - ZVAL_ARR(dst, src_ht); - break; - } array_init_size(dst, zend_hash_num_elements(src_ht)); HashTable *dst_ht = Z_ARRVAL_P(dst); @@ -253,14 +212,10 @@ static void persistent_zval_to_request(zval *dst, zval *src) { break; } if (key) { - if (ZSTR_IS_INTERNED(key)) { - zend_hash_add_new(dst_ht, key, &rval); - } else { - zend_string *rkey = zend_string_init(ZSTR_VAL(key), ZSTR_LEN(key), 0); - ZSTR_H(rkey) = ZSTR_H(key); - zend_hash_add_new(dst_ht, rkey, &rval); - zend_string_release(rkey); - } + zend_string *rkey = zend_string_init(ZSTR_VAL(key), ZSTR_LEN(key), 0); + ZSTR_H(rkey) = ZSTR_H(key); + zend_hash_add_new(dst_ht, rkey, &rval); + zend_string_release(rkey); } else { zend_hash_index_add_new(dst_ht, idx, &rval); } diff --git a/zval_test.go b/zval_test.go index 80aad7f16f..9fa05810c0 100644 --- a/zval_test.go +++ b/zval_test.go @@ -44,6 +44,8 @@ func TestPersistentZvalRoundtrip(t *testing.T) { require.NotContains(t, out, "FAIL", "persist-roundtrip.php reported a failure:\n"+out) require.Contains(t, out, "OK null") require.Contains(t, out, "OK enum active") + require.Contains(t, out, "OK const array") + require.Contains(t, out, "OK interned key and value") require.Contains(t, out, "OK stdClass rejected") require.Contains(t, out, "OK resource rejected") require.Contains(t, out, "OK nested stdClass rejected")