Repository navigation
ext/spl: Share arrays copy-on-write in ArrayObject and ArrayIterator - #23945
nicolas-grekas wants to merge 3 commits into
Conversation
|
I don't think we should try to be fixing or optimizing this until we have removed support for objects as a backing array. As all the complexity is due to the hashtable sometimes being a property HT. The moment that "feature" is removed dealing with a single HashTable becomes straightforward. |
|
The object-backed mode isn't touched here, it keeps copying as before. What this deals with is sharing a plain array: separating it before writes and moving iterator positions when that happens, which stays the same once object backing is gone. Its removal can only land in 9.0, while |
866a245 to
0b60d9d
Compare
|
PR updated: immutable arrays (opcache literals, |
3322ea1 to
298ec30
Compare
298ec30 to
f138a57
Compare
|
Quoted below: written with Claude, an LLM, as CONTRIBUTING.md asks us to mark. The builds and test runs behind it are on my machine; the commands to repeat them are under the fold.
|
ArrayObject and ArrayIterator assumed they were the only owner of their storage, but __serialize(), __debugInfo(), var_export(), json_encode() and the sort methods hand it out or borrow it without copying it. Writing to the object then modified the array that was handed out, and failed the refcount assertion of zend_hash.c in debug builds. The storage is now separated before any write. When this happens while it is iterated, iterators move to the copy of their position that zend_array_dup() registers on the new table. When the storage is replaced, they keep their position and get registered on the new table. An error handler can now separate the storage by writing to the object, so the table is fetched after the code that can call one: get_hash_key() on read paths, and argument parsing in ArrayIterator::seek(). This also fixes use-after-frees when a handler calls exchangeArray() there, or while an element is being created.
ArrayObject and ArrayIterator duplicated the array they were given when its refcount was above 1, and the arrays they hand out through getArrayCopy(), exchangeArray(), clone and array casts. They now share these arrays, and rely on the separation of their storage before writes. Arrays holding references are still duplicated, since zend_array_dup() unwraps references with refcount 1. Immutable arrays are still duplicated too, since iterators cannot be registered on them. exchangeArray() still returns a duplicate when iterators are registered on the storage, as they would keep following the returned array. While an iterator has a position, it is registered on the table shared with the caller's array.
Immutable arrays, that is opcache literals and the empty array, were still duplicated. They are now stored as is. Since iterators cannot be registered on them, the storage is separated when a position is first needed in it, unless it is empty: positions in an empty array need no iterator. The sort methods separate it first too, as the sorted array replaces the storage. spl_array_method() no longer writes the sorted array through a pointer kept across the call. When sorting through an iterator, the callback can replace the storage of the underlying ArrayObject by an immutable array, and writing the HashTable alone would leave the type flags of that array on a refcounted one.
f138a57 to
d01b76f
Compare
|
Thanks for testing @anun333. Rebased. I reviewed this PR again and found three cases that I missed. An error handler that writes to the object during a read led to a use-after-free. A I also split the PR. The separation before writes is now #25432, as the fix for GH-17935. This PR is on top of #25432 and only shares the arrays. Sharing has a cost that I didn't see before, see the updated description. An iterator is now registered on the table shared with the caller's array, so copies of that array cost more while the iterator is alive. It also needs #23401 first. Status: needs work |
On top of #25432, which is the first commit here.
ArrayObject and ArrayIterator duplicate the array they're given as soon as its refcount is above 1. That's the common case for
return new \ArrayIterator($this->items);in getIterator(), so every foreach over such a collection copies it. getArrayCopy(), exchangeArray(), clone and (array) duplicate the storage too.The second commit shares these arrays instead, as the TODOs in spl_array.c suggested. It relies on #25432 to separate the storage before any write. Arrays holding references are still duplicated since zend_array_dup() unwraps references with refcount 1. The third commit shares immutable arrays too. They're separated when a position is first needed in them since they can't hold iterators, except empty ones which need no iterator.
Instructions per operation (callgrind, 101 elements with string keys): getIterator() + foreach -11%, new ArrayObject() -81%, getArrayCopy() -89%, clone -84%, exchangeArray() -88%, (array) -41%.
foreachover an aggregate holding[]: -11%. With packed integers, the copies are 48% to 64% cheaper, (array) 33%, and getIterator() + foreach doesn't change. foreach over an ArrayObject costs 3% more. That's the cost of #25432.While an iterator has a position, it's now registered on the table that is shared with the caller's array. Each copy-on-write duplication of that array then adds an iterator slot until the iterator is destroyed. GH-23332 also becomes reachable from a plain array, so this needs #23401 first.
It conflicts with #22921, which separates the storage on each access and would undo the sharing. /cc @alexandre-daubois @iliaal