Repository navigation
array_push(): Append to packed arrays in bulk #24157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
d26674c
9d1523c
d8ef5ef
f1c7c25
67c5978
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3476,19 +3476,56 @@ PHP_FUNCTION(array_push) | |
| Z_PARAM_VARIADIC('+', args, argc) | ||
| ZEND_PARSE_PARAMETERS_END(); | ||
|
|
||
| HashTable *ht = Z_ARRVAL_P(stack); | ||
|
|
||
| /* Pushing nothing must not change the array, not even initialize it. A | ||
| * single value is added to an uninitialized array more cheaply by | ||
| * zend_hash_next_index_insert(), which initializes it inline. */ | ||
| if (argc > 0 && (HT_IS_PACKED(ht) || (argc > 1 && (HT_FLAGS(ht) & HASH_FLAG_UNINITIALIZED)))) { | ||
| /* Like zend_hash_next_index_insert(), treat ZEND_LONG_MIN (no integer | ||
| * key used yet) as 0. */ | ||
|
LamentXU123 marked this conversation as resolved.
Outdated
|
||
| zend_long next_index = ht->nNextFreeElement == ZEND_LONG_MIN ? 0 : ht->nNextFreeElement; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This leaks details of zend_hash into ext/standard, this should be abstracted somehow
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved it into Zend/zend_hash.c as a new It also adds the references, like The call is out of line now, which costs a few points for 3 values compared with the inline version; single values still use |
||
| bool appends_after_last = next_index == (zend_long) ht->nNumUsed; | ||
| uint32_t room = ht->nTableSize - ht->nNumUsed; | ||
|
|
||
| /* The values can be stored after the last used slot. Growing an array | ||
| * with holes may turn it into a hash, so it must already have room. */ | ||
| 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)) { | ||
|
mehmetcansahin marked this conversation as resolved.
Outdated
|
||
| zend_hash_extend(ht, ht->nNumUsed + argc, true); | ||
| } else if (UNEXPECTED(HT_FLAGS(ht) & HASH_FLAG_UNINITIALIZED)) { | ||
|
mehmetcansahin marked this conversation as resolved.
Outdated
|
||
| zend_hash_real_init_packed(ht); | ||
| } | ||
|
|
||
| /* ZEND_HASH_FILL_END() resets the internal pointer, array_push() keeps it. */ | ||
| uint32_t pos = ht->nInternalPointer; | ||
| ZEND_HASH_FILL_PACKED(ht) { | ||
| for (uint32_t i = 0; i < argc; i++) { | ||
| Z_TRY_ADDREF(args[i]); | ||
| ZEND_HASH_FILL_ADD(&args[i]); | ||
| } | ||
| } ZEND_HASH_FILL_END(); | ||
| ht->nInternalPointer = pos; | ||
|
|
||
| RETURN_LONG(zend_hash_num_elements(ht)); | ||
| } | ||
| } | ||
|
|
||
| /* For each subsequent argument, make it a reference, increase refcount, and add it to the end of the array */ | ||
|
mehmetcansahin marked this conversation as resolved.
Outdated
|
||
| for (uint32_t i = 0; i < argc; i++) { | ||
| Z_TRY_ADDREF(args[i]); | ||
|
|
||
| if (zend_hash_next_index_insert(Z_ARRVAL_P(stack), &args[i]) == NULL) { | ||
| if (zend_hash_next_index_insert(ht, &args[i]) == NULL) { | ||
| Z_TRY_DELREF(args[i]); | ||
| zend_throw_error(NULL, "Cannot add element to the array as the next element is already occupied"); | ||
| RETURN_THROWS(); | ||
| } | ||
| } | ||
|
|
||
| /* Clean up and return the number of values in the stack */ | ||
| RETVAL_LONG(zend_hash_num_elements(Z_ARRVAL_P(stack))); | ||
| RETVAL_LONG(zend_hash_num_elements(ht)); | ||
| } | ||
| /* }}} */ | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| --TEST-- | ||
| array_push() keeps the internal pointer and the next index | ||
|
mehmetcansahin marked this conversation as resolved.
Outdated
|
||
| --FILE-- | ||
| <?php | ||
| // The internal pointer does not move | ||
| $a = [1, 2, 3]; | ||
| next($a); | ||
| array_push($a, 4, 5); | ||
| var_dump(key($a)); | ||
|
|
||
| // The next index left by a removed last element is used | ||
| $a = [0, 1, 2]; | ||
| unset($a[2]); | ||
| array_push($a, 3); | ||
| var_dump(array_keys($a)); | ||
|
|
||
| // Pushing nothing keeps the next index of an empty array | ||
| $a = []; | ||
| array_push($a); | ||
| $a[-5] = 1; | ||
| $a[] = 2; | ||
| var_dump(array_keys($a)); | ||
|
|
||
| // A negative next index of an empty copy is used | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't reach new code
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, it takes the one-by-one path: the copy is uninitialized with |
||
| $a = [-3 => 0]; | ||
| unset($a[-3]); | ||
| $copy = $a; | ||
| array_push($a, 1, 2); | ||
| var_dump(array_keys($a)); | ||
| ?> | ||
| --EXPECT-- | ||
| int(1) | ||
| array(3) { | ||
| [0]=> | ||
| int(0) | ||
| [1]=> | ||
| int(1) | ||
| [2]=> | ||
| int(3) | ||
| } | ||
| array(2) { | ||
| [0]=> | ||
| int(-5) | ||
| [1]=> | ||
| int(-4) | ||
| } | ||
| array(2) { | ||
| [0]=> | ||
| int(-2) | ||
| [1]=> | ||
| int(-1) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped the
argc > 1special case, it made no measurable difference. The one split left isargc == 1in array_push(): routing a single value through the new function cost ~5% onarray_push($list, $x).