[intl] Size sortWithSortKeys buffers based on array size - #23504
Open
iliaal wants to merge 1 commit into
Open
Conversation
devnexen
reviewed
Aug 29, 2026
|
|
||
| collator_sort_key_index_t* sortKeyIndxBuf = NULL; /* buffer to store 'indexes' which will be passed to 'qsort' */ | ||
| uint32_t sortKeyIndxBufSize = DEF_SORT_KEYS_INDX_BUF_SIZE; | ||
| uint32_t sortKeyIndxBufSize = 0; |
Member
There was a problem hiding this comment.
--- a/ext/intl/collator/collator_sort.cpp
+++ b/ext/intl/collator/collator_sort.cpp
@@ -45,8 +45,8 @@
static const size_t DEF_SORT_KEYS_BUF_SIZE = 1048576;
static const size_t DEF_SORT_KEYS_BUF_INCREMENT = 1048576;
-static const size_t DEF_SORT_KEYS_INDX_BUF_SIZE = 1048576;
-static const size_t DEF_SORT_KEYS_INDX_BUF_INCREMENT = 1048576;
+static const size_t MIN_SORT_KEYS_BUF_SIZE = 4096;
+static const size_t SORT_KEY_LENGTH_ESTIMATE = 32;
static const size_t DEF_UTF16_BUF_SIZE = 1024;
@@ -427,17 +427,17 @@
zval* hashData = nullptr; /* currently processed item of input hash */
char* sortKeyBuf = nullptr; /* buffer to store sort keys */
- uint32_t sortKeyBufSize = DEF_SORT_KEYS_BUF_SIZE; /* buffer size */
+ uint32_t sortKeyBufSize = 0; /* buffer size */
ptrdiff_t sortKeyBufOffset = 0; /* pos in buffer to store sort key */
uint32_t sortKeyLen = 0; /* the length of currently processing key */
uint32_t bufLeft = 0;
uint32_t bufIncrement = 0;
collator_sort_key_index_t* sortKeyIndxBuf = nullptr; /* buffer to store 'indexes' which will be passed to 'qsort' */
- uint32_t sortKeyIndxBufSize = DEF_SORT_KEYS_INDX_BUF_SIZE;
uint32_t sortKeyIndxSize = sizeof( collator_sort_key_index_t );
uint32_t sortKeyCount = 0;
+ uint32_t numElements = 0;
uint32_t j = 0;
UChar* utf16_buf = nullptr; /* tmp buffer to hold current processing string in utf-16 */
@@ -472,9 +472,20 @@
if( !hash || zend_hash_num_elements( hash ) == 0 )
RETURN_TRUE;
+ numElements = zend_hash_num_elements( hash );
+
+ if( numElements > DEF_SORT_KEYS_BUF_SIZE / SORT_KEY_LENGTH_EST
+ sortKeyBufSize = DEF_SORT_KEYS_BUF_SIZE;
+ } else {
+ sortKeyBufSize = numElements * SORT_KEY_LENGTH_ESTIMATE;
+ }
+ if( sortKeyBufSize < MIN_SORT_KEYS_BUF_SIZE ) {
+ sortKeyBufSize = MIN_SORT_KEYS_BUF_SIZE;
+ }
+
/* Create buffers */
sortKeyBuf = reinterpret_cast<char *>(ecalloc( sortKeyBufS);
- sortKeyIndxBuf = reinterpret_cast<collator_sort_key_index_t *>(ecalloc( sortKeyIndxBufSize, sizeof( uint8_t ) ));
+ sortKeyIndxBuf = reinterpret_cast<collator_sort_key_index_t *>sortKeyIndxSize, 0 ));
utf16_buf = eumalloc( utf16_buf_size );
/* Iterate through input hash and create a sort key for each value. */
@@ -524,7 +535,13 @@
/* check for sortKeyBuf overflow, increasing its size of the buffer if needed */
if( sortKeyLen > bufLeft )
{
- bufIncrement = ( sortKeyLen > DEF_SORT_KEYS_BU DEF_SORT_KEYS_BUF_INCREMENT;
+ bufIncrement = sortKeyBufSize;
+
+ if( bufIncrement > DEF_SORT_KEYS_BUF_INCREMENT )
+ bufIncrement = DEF_SORT_KEYS_BUF_INCRE
+
+ if( bufIncrement < sortKeyLen )
+ bufIncrement = sortKeyLen;
sortKeyBufSize += bufIncrement;
bufLeft += bufIncrement;
@@ -534,16 +551,6 @@
sortKeyLen = ucol_getSortKey( co->ucoll, utf16ortKeyBuf + sortKeyBufOffset, bufLeft );
}
- /* check sortKeyIndxBuf overflow, increasing its size of the buffer if needed */
- if( ( sortKeyCount + 1 ) * sortKeyIndxSize > sortKeyIn
- {
- bufIncrement = ( sortKeyIndxSize > DEF_SORT_KErtKeyIndxSize : DEF_SORT_KEYS_INDX_BUF_INCREMENT;
-
- sortKeyIndxBufSize += bufIncrement;
-
- sortKeyIndxBuf = reinterpret_cast<collator_sorortKeyIndxBuf, sortKeyIndxBufSize ));
- }
-
sortKeyIndxBuf[sortKeyCount].key = (char*)sortKeyBufOffset; /* remember just offset, cause address */
may be changed due to realloc. */
sortKeyIndxBuf[sortKeyCount].zstr = hashData;as you can see it is diff for master, because your changes are improvements.
Member
Author
There was a problem hiding this comment.
Fair, admittedly memory reduction was a bit too ambitious for 8.4 😆
collator_sort_with_sort_keys() ecalloc'd sortKeyBuf and sortKeyIndxBuf at DEF_SORT_KEYS_BUF_SIZE (1MiB) each on every call regardless of array size. sortKeyBuf now starts from zend_hash_num_elements() * 32 bytes, clamped to a 4KiB minimum and the previous 1MiB cap, and grows geometrically up to DEF_SORT_KEYS_BUF_INCREMENT. sortKeyIndxBuf is allocated exactly for the element count, dropping the index-buffer growth path. Sibling audit: DEF_SORT_KEYS* constants have no other users and collator_sort()/asort()/get_sort_key() already scale allocations.
iliaal
force-pushed
the
fix/intl-sortkeys-84
branch
from
August 29, 2026 13:13
e5f02bf to
63712fc
Compare
devnexen
approved these changes
Aug 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
collator_sort_with_sort_keys() unconditionally ecalloc'd sortKeyBuf and sortKeyIndxBuf at 1MiB each, 2MiB per call regardless of array size, which dominates cost for small arrays. sortKeyBuf now starts from zend_hash_num_elements() * 32 bytes, clamped to a 4KiB minimum and the old 1MiB cap, and grows geometrically up to 1MiB increments. sortKeyIndxBuf is allocated exactly for the element count. Large inputs behave as before.