Skip to content

Commit

Permalink
Fix bug in serializing php 8.2 lists, reduce memory usage.
Browse files Browse the repository at this point in the history
Starting in php 8.2, zvals are stored instead of Buckets, halving the
memory usage per extra element. Use the same memory allocation when
unserializing data and when storing data in shared memory in php 8.2.

This was previously copying too much data when there was an associative
array instead of a list, and apc_025.phpt would fail with valgrind.

Related to #323
  • Loading branch information
TysonAndre authored and nikic committed Nov 5, 2022
1 parent 98b2616 commit 11de4db
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 6 deletions.
18 changes: 15 additions & 3 deletions apc_persist.c
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,9 @@ static zend_bool apc_persist_calc_ht(apc_persist_context_t *ctxt, const HashTabl
}

/* TODO Too sparse hashtables could be compacted here */
ADD_SIZE(HT_USED_SIZE(ht));
#if PHP_VERSION_ID >= 80200
if (HT_IS_PACKED(ht)) {
ADD_SIZE(HT_PACKED_USED_SIZE(ht));
for (idx = 0; idx < ht->nNumUsed; idx++) {
zval *val = ht->arPacked + idx;
ZEND_ASSERT(Z_TYPE_P(val) != IS_INDIRECT && "INDIRECT in packed array?");
Expand All @@ -139,6 +139,7 @@ static zend_bool apc_persist_calc_ht(apc_persist_context_t *ctxt, const HashTabl
} else
#endif
{
ADD_SIZE(HT_USED_SIZE(ht));
for (idx = 0; idx < ht->nNumUsed; idx++) {
Bucket *p = ht->arData + idx;
if (Z_TYPE(p->val) == IS_UNDEF) continue;
Expand Down Expand Up @@ -334,9 +335,9 @@ static zend_array *apc_persist_copy_ht(apc_persist_context_t *ctxt, const HashTa

ht->nNextFreeElement = 0;
ht->nInternalPointer = HT_INVALID_IDX;
HT_SET_DATA_ADDR(ht, COPY(HT_GET_DATA_ADDR(ht), HT_USED_SIZE(ht)));
#if PHP_VERSION_ID >= 80200
if (HT_IS_PACKED(ht)) {
HT_SET_DATA_ADDR(ht, COPY(HT_GET_DATA_ADDR(ht), HT_PACKED_USED_SIZE(ht)));
for (idx = 0; idx < ht->nNumUsed; idx++) {
zval *val = ht->arPacked + idx;
if (Z_TYPE_P(val) == IS_UNDEF) continue;
Expand All @@ -354,6 +355,7 @@ static zend_array *apc_persist_copy_ht(apc_persist_context_t *ctxt, const HashTa
} else
#endif
{
HT_SET_DATA_ADDR(ht, COPY(HT_GET_DATA_ADDR(ht), HT_USED_SIZE(ht)));
for (idx = 0; idx < ht->nNumUsed; idx++) {
Bucket *p = ht->arData + idx;
if (Z_TYPE(p->val) == IS_UNDEF) continue;
Expand Down Expand Up @@ -563,6 +565,16 @@ static zend_reference *apc_unpersist_ref(
return ref;
}

/* Compute the size of the HashTable data to create when unpersisting. */
static zend_always_inline size_t apc_compute_ht_data_size(const HashTable *ht) {
#if PHP_VERSION_ID >= 80200
if (HT_IS_PACKED(ht)) {
return HT_PACKED_SIZE(ht);
}
#endif
return HT_SIZE(ht);
}

static zend_array *apc_unpersist_ht(
apc_unpersist_context_t *ctxt, const HashTable *orig_ht) {
HashTable *ht = emalloc(sizeof(HashTable));
Expand All @@ -580,7 +592,7 @@ static zend_array *apc_unpersist_ht(
}
#endif

HT_SET_DATA_ADDR(ht, emalloc(HT_SIZE(ht)));
HT_SET_DATA_ADDR(ht, emalloc(apc_compute_ht_data_size(ht)));
memcpy(HT_GET_DATA_ADDR(ht), HT_GET_DATA_ADDR(orig_ht), HT_HASH_SIZE(ht->nTableMask));

#if PHP_VERSION_ID >= 80200
Expand Down
33 changes: 33 additions & 0 deletions tests/apc_025.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
--TEST--
APC: apcu_fetch of packed array
--SKIPIF--
<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
--INI--
apc.enabled=1
apc.enable_cli=1
--FILE--
<?php
var_dump(apcu_store('foo', [1, 2, 3, 4, 5, 6, 7, 8]));
var_dump(apcu_fetch('foo'));

?>
--EXPECT--
bool(true)
array(8) {
[0]=>
int(1)
[1]=>
int(2)
[2]=>
int(3)
[3]=>
int(4)
[4]=>
int(5)
[5]=>
int(6)
[6]=>
int(7)
[7]=>
int(8)
}
7 changes: 4 additions & 3 deletions tests/server_test.inc
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,12 @@ function server_start_one($host, $port, $code = 'echo "Hello world";', $php_opts

$handle = proc_open($cmd, $descriptorspec, $pipes, $doc_root);
}

// note: even when server prints 'Listening on localhost:8964...Press Ctrl-C to quit.'
// it might not be listening yet...need to wait until fsockopen() call returns
// Let this wait for up to 10 seconds to avoid spurious failures with valgrind.
$i = 0;
while (($i++ < 10) && !connection_test($host, $port)) {
while (($i++ < 100) && !connection_test($host, $port)) {
usleep(100000);
}

Expand All @@ -78,7 +79,7 @@ function server_start($code = 'echo "Hello world";', $php_opts = array(), $no_ro
for ($i = 0; $i < $num_servers; $i++) {
$h = server_start_one(PHP_CLI_SERVER_HOSTNAME, PHP_CLI_SERVER_PORT+$i, $code, $php_opts, $no_router);
$handles[] = $h;
}
}

register_shutdown_function(
function($handles) use($router) {
Expand Down

0 comments on commit 11de4db

Please sign in to comment.