Repository navigation
array_push(): Append to packed arrays in bulk - #24157
mehmetcansahin wants to merge 5 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 |
| $a[] = 2; | ||
| var_dump(array_keys($a)); | ||
|
|
||
| // A negative next index of an empty copy is used |
There was a problem hiding this comment.
Right, it takes the one-by-one path: the copy is uninitialized with nNextFreeElement = -2. I kept it on purpose, it fails if the next-index check is dropped.
| /* 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
There was a problem hiding this comment.
Moved it into Zend/zend_hash.c as a new ZEND_API function, zend_hash_next_index_insert_copies() (noted in UPGRADING.INTERNALS), next to _zend_hash_index_add_or_update_i(), whose next-index and growth rules it follows. array_push() has no HashTable internals left. Since the function writes the slots itself instead of using ZEND_HASH_FILL_*, the internal pointer no longer needs saving and restoring.
It also adds the references, like ZVAL_COPY(): array_push() needs that, and a separate addref loop made 1000 spread values ~6% slower. SplFixedArray's get_properties_for() and __serialize() fill arrays the same way; switching them locally made (array) $fixed 53% and __serialize() 70% faster for 1000 elements, which I'd send as a follow-up.
The call is out of line now, which costs a few points for 3 values compared with the inline version; single values still use zend_hash_next_index_insert() directly. Happy to change the name or semantics if you'd prefer something else.
|
|
||
| /* 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?
There was a problem hiding this comment.
Dropped the argc > 1 special case, it made no measurable difference. The one split left is argc == 1 in array_push(): routing a single value through the new function cost ~5% on array_push($list, $x).
Add zend_hash_next_index_insert_copies(), which inserts several values like repeated zend_hash_next_index_insert() calls and adds a reference to each. Values appended right after the last used slot of a packed or uninitialized array are stored in one go. array_push() uses it for two or more values and no longer touches HashTable internals; a single value still goes through zend_hash_next_index_insert(). Rewrite the test to cover the room and growth boundaries, holes, the next index, refcounts and a partial insert.
|
Updated in f1c7c25: the bulk insertion now lives in Zend/zend_hash.c (see the review thread). The test covers the room and growth boundaries, holes and the next index, including the mutations @ArtUkrainskiy listed; a table near @ndossche could you take another look? |
array_push()used to callzend_hash_next_index_insert()for each value. With two or more values it now calls the newzend_hash_next_index_insert_copies(), which inserts all of them and adds a reference to each. When the array is packed or uninitialized and its next free index is right after the last used slot, that function makes room once if needed and stores the values directly. This mainly helpsarray_push($a, ...$values).A single value still goes through
zend_hash_next_index_insert(): passing it through the new function madearray_push($list, $x)about 5% slower.zend_hash_next_index_insert_copies()inserts the values one by one for: hash arrays; packed arrays whose next free index is past the last used slot (e.g. afterunset()of the last element); packed arrays with holes that would need to grow (growth may turn them into a hash); more values than fit belowHT_MAX_SIZE.Keys, the next free index, the internal pointer, table growth and the error for an occupied next index stay the same.
Benchmark: script. Release build (
-O2 -DNDEBUG, NTS), OPcache/JIT off (php -n), 15 alternating base/PR pairs per case (best of 5 repetitions per process), base6fc4017a940(the master merged into this branch). Apple M1 (macOS, Apple clang) measured PRf1c7c2542fd; AMD EPYC 9V74 (Linux VM, GCC 14.2, pinned to one CPU) measured67c59787d96, which only changes a comment: itszend_hash.oon M1 and its PHP binary on EPYC are byte-identical to those off1c7c2542fd.Times are medians in ns/op. Change = PR median / base median - 1, calculated before rounding. Negative means faster.
Base-against-base runs before and after these pairs stayed within ±0.32% on M1 for every case, and ranged from -5.3% to +4.1% on EPYC, where 750 of 900 processes saw steal time above 5%. On EPYC the rows with 3 or more values on a list or
[]are well outside that range; the single-value rows are within it, so they don't show a change either way.On M1 the two +0.9% rows (0.11 ns/call) are outside its range, but on the single-value path array_push() executes no more instructions than on master, and
zend_hash_next_index_insert()is unchanged, just placed 48 bytes later in the binary, so I attribute them to code layout.Hash arrays with several values also gain a little (-4.1% / -7.4%), most likely because the one-by-one path inside the new function inlines the insert instead of calling
zend_hash_next_index_insert()for each value.The
bench-common.php verifyoutputs match for base and PR on both machines.