Repository navigation
array_push(): Append to packed arrays in bulk - #24157
mehmetcansahin wants to merge 3 commits into
Conversation
When the next index of a packed array is the end of its used slots, reserve the room once and copy the values instead of inserting them one by one. Arrays with holes only take this path when no growth is needed. A single value pushed onto an uninitialized array still goes through zend_hash_next_index_insert(), which initializes the array inline and is cheaper than a separate initialization.
Drop UNEXPECTED() on the uninitialized array branch, pushing several values onto an empty array is common. Fix the stale comment on the insertion loop and trim the comments that restate the code.
LamentXU123
left a comment
There was a problem hiding this comment.
LGTM /cc @arnaud-lb Thanks!
AWS x86_64 (c6id.metal)
Laravel 12.11.0 demo app - 50 iterations, 50 warmups, 100 requests (sec)
Symfony 2.8.0 demo app - 50 iterations, 50 warmups, 100 requests (sec)
Wordpress 6.9 main page - 50 iterations, 20 warmups, 20 requests (sec)
bench.php - 50 iterations, 20 warmups, 2 requests (sec)
|
|
I measured this on master c65321c (d8ef5ef merges cleanly) with GCC 13.3, GCC 14.2, clang 18.1 and clang 20.1 on an i7-13700H, built with the flags of the official Docker image. Change in CPU cycles per call with the PR, relative to master (negative = fewer cycles, faster); median of five alternating runs, each call in a loop, so the percentages are of the whole statement:
Pushing three or more values onto a list or The spread rows gain less here than in the PR description: 25–31% fewer cycles for 16 values onto a list and 35–38% for 1,000 values onto
Code: the fast path repeats Correctness: on 13,879 Tests: three of those mutations pass // needs one slot more than the table has: ASan catches both off-by-one mutations
$a = range(1, 7);
var_dump(array_push($a, 8, 9), $a === range(1, 9)); // int(9) bool(true)
$a = range(1, 8);
var_dump(array_push($a, ...range(9, 17)), $a === range(1, 17)); // int(17) bool(true)
// a full packed array with holes still takes the old loop: the internal pointer stays past the end
$a = range(0, 15);
for ($i = 0; $i < 14; $i++) unset($a[$i]);
end($a);
next($a);
var_dump(array_push($a, 16, 17), key($a)); // int(4) NULL; without the holes check: int(4) int(16)No correctness issues found. LGTM. Full tables, scripts and data: https://github.com/ArtUkrainskiy/php-src-bench/tree/main/reports/pr-24157-array-push |
| @@ -0,0 +1,52 @@ | |||
| --TEST-- | |||
| array_push() keeps the internal pointer and the next index | |||
There was a problem hiding this comment.
This test should test some of the edge cases (e.g. arrays close to max size, not enough room, ...)
| $a[] = 2; | ||
| var_dump(array_keys($a)); | ||
|
|
||
| // A negative next index of an empty copy is used |
| /* Pushing nothing must leave the array untouched. A single value is added | ||
| * to an uninitialized array more cheaply by zend_hash_next_index_insert(). */ | ||
| if (argc > 0 && (HT_IS_PACKED(ht) || (argc > 1 && (HT_FLAGS(ht) & HASH_FLAG_UNINITIALIZED)))) { | ||
| zend_long next_index = ht->nNextFreeElement == ZEND_LONG_MIN ? 0 : ht->nNextFreeElement; |
There was a problem hiding this comment.
This leaks details of zend_hash into ext/standard, this should be abstracted somehow
|
|
||
| /* Pushing nothing must leave the array untouched. A single value is added | ||
| * to an uninitialized array more cheaply by zend_hash_next_index_insert(). */ | ||
| if (argc > 0 && (HT_IS_PACKED(ht) || (argc > 1 && (HT_FLAGS(ht) & HASH_FLAG_UNINITIALIZED)))) { |
There was a problem hiding this comment.
This condition is quite complex. Maybe the single value is indeed an edge case that's handled faster by the old code, but does it really have such a large performance difference that this complexity is warranted?
| if (appends_after_last | ||
| && argc <= HT_MAX_SIZE - ht->nNumUsed /* nNumUsed + argc must not wrap */ | ||
| && (argc <= room || HT_IS_WITHOUT_HOLES(ht))) { | ||
| if (UNEXPECTED(argc > room)) { |
There was a problem hiding this comment.
I don't think this is UNEXPECTED
array_push()used to callzend_hash_next_index_insert()for each value. When the array is packed (or uninitialized, with 2+ values) and its next free index is right after the last used slot, this makes room once when needed and writes the values directly withZEND_HASH_FILL_*. This mainly helpsarray_push($a, ...$values).The old loop is still used for: hash arrays; packed arrays whose next free index is past the last used slot (e.g. after
unset()of the last element); packed arrays with holes that would need to grow (growth may turn them into a hash); a single value pushed onto an uninitialized empty array.Keys, the next free index, the internal pointer and table-growth rules stay the same.
Benchmark: script. Release build (
-O2 -DNDEBUG, NTS), OPcache/JIT off (php -n), 20,000–10,000,000 calls per repetition depending on the case, 15 alternating base/PR pairs (best of 5 repetitions per run), base421347dff42. Apple M1 (macOS) and Xeon Platinum 8370C (Linux VM, pinned to one CPU).Times are medians in ns/op. Change = PR median / base median - 1, calculated before rounding. Negative means faster.
Hash rows use the unchanged insertion loop; their measured changes are within ±2%. The
empty, 1 valuecase uses the old loop after the extra checks and measured about 1.5 ns/call slower; its timing includes$a = []and freeing the previous array.empty, 2 valuesis mixed (+2.0% M1, −8.2% Xeon): one possible explanation is that saving the second insert may be offset by the separatezend_hash_real_init_packed()call.The
bench-common.php verifyoutputs match for base and PR on both machines.