Skip to content

GC optimization for primitive arrays #19626

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.

Sign up for GitHub

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
+33 −1
Draft

GC optimization for primitive arrays #19626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3db5c6d
GC optimization for primitive arrays
iluuu1994 Aug 28, 2025
1ae410e
Simplify
iluuu1994 Aug 28, 2025
123dcc5
Simplify more
iluuu1994 Aug 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion Zend/zend_gc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,9 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack)
handle_ht:
n = ht->nNumUsed;
zv = ht->arPacked;
if (GC_FLAGS(ht) & GC_NOT_COLLECTABLE) {
goto next;
}
if (HT_IS_PACKED(ht)) {
goto handle_zvals;
}
Expand Down Expand Up @@ -1041,7 +1044,7 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack)
* counts and mark visited nodes grey. See MarkGray() in Bacon & Rajan. */
static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack)
{
HashTable *ht;
HashTable *ht, *primitive_ht = NULL;
Bucket *p;
zval *zv;
uint32_t n;
Expand Down Expand Up @@ -1132,6 +1135,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack)
handle_zvals:
for (; n != 0; n--) {
if (Z_COLLECTABLE_P(zv)) {
primitive_ht = NULL;
ref = Z_COUNTED_P(zv);
GC_DELREF(ref);
if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) {
Expand All @@ -1153,12 +1157,20 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack)
}
zv++;
}
if (primitive_ht) {
GC_ADD_FLAGS(primitive_ht, GC_NOT_COLLECTABLE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty array should be GC_NOT_COLLECTABLE from start

primitive_ht = NULL;
}
}
} else if (GC_TYPE(ref) == IS_ARRAY) {
ZEND_ASSERT(((zend_array*)ref) != &EG(symbol_table));
ht = (zend_array*)ref;
handle_ht:
n = ht->nNumUsed;
if (GC_FLAGS(ht) & GC_NOT_COLLECTABLE) {
goto next;
}
primitive_ht = ht;
Comment on lines +1170 to +1173
Copy link
Member

@arnaud-lb arnaud-lb Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved above handle_ht, otherwise this logic is applied to hash tables returned by get_gc handlers. I think it wouldn't be safe, as the ht may have INDIRECT elements, whose content can be changed without touching the array. For objects with custom get_gc handlers, the ht may be updated in unexpected ways too.

#[AllowDynamicProperties]
class C {
    public $c;
}

$c = new C();
$c->c = 1;
$c->d = 2; // dynamic prop

$x = $c;
unset($x); // add to roots

gc_collect_cycles(); // marks C->properties as not collectable

$c->c = $c; // should be collectable now, but isn't

unset($c);

gc_collect_cycles(); // ignores C->properties

Otherwise, this looks right to me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, very good catch.

if (HT_IS_PACKED(ht)) {
zv = ht->arPacked;
goto handle_zvals;
Expand All @@ -1171,6 +1183,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack)
zv = Z_INDIRECT_P(zv);
}
if (Z_COLLECTABLE_P(zv)) {
primitive_ht = NULL;
ref = Z_COUNTED_P(zv);
GC_DELREF(ref);
if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) {
Expand All @@ -1196,6 +1209,10 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack)
}
p++;
}
if (primitive_ht) {
GC_ADD_FLAGS(ht, GC_NOT_COLLECTABLE);
primitive_ht = NULL;
}
} else if (GC_TYPE(ref) == IS_REFERENCE) {
if (Z_COLLECTABLE(((zend_reference*)ref)->val)) {
ref = Z_COUNTED(((zend_reference*)ref)->val);
Expand Down Expand Up @@ -1378,6 +1395,9 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack)

handle_ht:
n = ht->nNumUsed;
if (GC_FLAGS(ht) & GC_NOT_COLLECTABLE) {
goto next;
}
if (HT_IS_PACKED(ht)) {
zv = ht->arPacked;
goto handle_zvals;
Expand Down Expand Up @@ -1623,6 +1643,9 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta

handle_ht:
n = ht->nNumUsed;
if (GC_FLAGS(ht) & GC_NOT_COLLECTABLE) {
goto next;
}
if (HT_IS_PACKED(ht)) {
zv = ht->arPacked;
goto handle_zvals;
Expand Down Expand Up @@ -1811,6 +1834,9 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe

handle_ht:
n = ht->nNumUsed;
if (GC_FLAGS(ht) & GC_NOT_COLLECTABLE) {
goto next;
}
if (HT_IS_PACKED(ht)) {
zv = ht->arPacked;
goto handle_zvals;
Expand Down
3 changes: 3 additions & 0 deletions Zend/zend_hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,7 @@ static zend_always_inline zval *_zend_hash_add_or_update_i(HashTable *ht, zend_s
nIndex = h | ht->nTableMask;
Z_NEXT(p->val) = HT_HASH_EX(arData, nIndex);
HT_HASH_EX(arData, nIndex) = HT_IDX_TO_HASH(idx);
GC_DEL_FLAGS(ht, GC_NOT_COLLECTABLE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I right this will delete the GC_NOT_COLLECTABLE flag even when a pririmite is added?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a trade-off. The patch is already adding 0.05% instructions to Wordpress. A branch with a potential for a branch miss may not be worth it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's have the impact tested. GC on large array is really costly and appending to a large array should ideally imply no single GC run when not really needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be difficult to track that precisely due to the way arrays are updated.

For example, the statement $a[1][2] = 3; is executed by fetching an INDIRECT reference to $a[1] and updating the reference in a separate opcode. This inserts NULL with the zend_hash API, but then the INDIRECT ref is modified without going through the zend_hash API. Possibly we could hint the array about what we are going to place in the reference, at least in this case.

Could you tell more about the use-case @mvorisek @dktapps? One alternative would be something like JS TypedArrays: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray. This would be GC friendly, space efficient, JIT friendly, and would solve some use-cases better than arrays or strings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use PHP for CLI heavily, very often we cache large datasets (database data, CSV data, ...) with memory frequently over 5 GB.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given you also created GH-17131, this PR will not be sufficient either. It's unlikely that there's a silver bullet to this problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current optimization would probably already help a lot for workflows in which the GC spends a lot of a time scanning scalar arrays that are usually not updated between two GC runs. @dktapps @mvorisek would you be able to measure the impact on your code? gc_status() can be used to find how much time is spent in GC.

In addition to that, a simple generational GC scheme would help as well, for use-cases with long lived large data sets:

  • Nodes that were scanned but not collected are marked as OLD
  • During the next GC runs we ignore OLD nodes
  • We run a full GC from time to time

I will try to implement the generational GC idea.

IntArray (or PrimitiveArray storing just zend_value and require the same zval type for every element) is useful either way.

Arrays with a size different than int would also be useful for binary data processing

Copy link
Contributor

@dktapps dktapps Aug 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given you also created GH-17131, this PR will not be sufficient either. It's unlikely that there's a silver bullet to this problem.

Well, when digging into the problem in my specific case, I found that blacklisting objects containing large arrays dropped my average GC from 22ms to 4ms, so that's a pretty big W in my book. (Also worth noting, my application runs on a 20Hz tick cycle, so this makes a major improvement to game tick lag.) I feel like this is a much easier win than generational GC.

My tests used this extension: https://github.com/dktapps/ext-gcignore (with some local changes) that flags vars as GC_NOT_COLLECTABLE, which is basically what this PR is doing. So it's a fairly good analog.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, when digging into the problem in my specific case, I found that blacklisting objects containing large arrays dropped my average GC from 22ms to 4ms, so that's a pretty big W in my book.

I see you responded to a different message than I thought. I thought you were responding to the explicit opt-out of GC for a single value, e.g. gc_mark_as_uncyclic().

Copy link
Contributor

@dktapps dktapps Aug 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you responded to a different message than I thought. I thought you were responding to the explicit opt-out of GC for a single value, e.g. gc_mark_as_uncyclic().

@iluuu1994 I already explored explicit opt-out ability with my extension and found it to be clunky. It works, but having to splash gc_ignore() everywhere wasn't great, and it'd be too easy to have hidden performance issues sneak back in the future due to developers who weren't aware of the GC problems with large arrays. So IMO it's not a great option, although I'd take it if that was the only option available.

The more sustainable solutions to the arrays issue would be a new GC (generational seems to be the favourite, there might be other options that I'm not aware of), or this PR. While this PR wouldn't generally solve the issue of long-lived objects being repeatedly scanned, it would (at least in my case) dramatically reduce the scale of the performance impact, with what seems like a relatively easy change.

if (flag & HASH_LOOKUP) {
ZVAL_NULL(&p->val);
} else {
Expand Down Expand Up @@ -970,6 +971,7 @@ static zend_always_inline zval *_zend_hash_str_add_or_update_i(HashTable *ht, co
nIndex = h | ht->nTableMask;
Z_NEXT(p->val) = HT_HASH(ht, nIndex);
HT_HASH(ht, nIndex) = HT_IDX_TO_HASH(idx);
GC_DEL_FLAGS(ht, GC_NOT_COLLECTABLE);

return &p->val;
}
Expand Down Expand Up @@ -1172,6 +1174,7 @@ static zend_always_inline zval *_zend_hash_index_add_or_update_i(HashTable *ht,
p = ht->arData + idx;
Z_NEXT(p->val) = HT_HASH(ht, nIndex);
HT_HASH(ht, nIndex) = HT_IDX_TO_HASH(idx);
GC_DEL_FLAGS(ht, GC_NOT_COLLECTABLE);
if ((zend_long)h >= ht->nNextFreeElement) {
ht->nNextFreeElement = (zend_long)h < ZEND_LONG_MAX ? h + 1 : ZEND_LONG_MAX;
}
Expand Down
3 changes: 3 additions & 0 deletions Zend/zend_hash.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1638,6 +1638,7 @@ static zend_always_inline zval *_zend_hash_append_ex(HashTable *ht, zend_string
nIndex = (uint32_t)p->h | ht->nTableMask;
Z_NEXT(p->val) = HT_HASH(ht, nIndex);
HT_HASH(ht, nIndex) = HT_IDX_TO_HASH(idx);
GC_DEL_FLAGS(ht, GC_NOT_COLLECTABLE);
ht->nNumOfElements++;
return &p->val;
}
Expand All @@ -1664,6 +1665,7 @@ static zend_always_inline zval *_zend_hash_append_ptr_ex(HashTable *ht, zend_str
nIndex = (uint32_t)p->h | ht->nTableMask;
Z_NEXT(p->val) = HT_HASH(ht, nIndex);
HT_HASH(ht, nIndex) = HT_IDX_TO_HASH(idx);
GC_DEL_FLAGS(ht, GC_NOT_COLLECTABLE);
ht->nNumOfElements++;
return &p->val;
}
Expand All @@ -1690,6 +1692,7 @@ static zend_always_inline void _zend_hash_append_ind(HashTable *ht, zend_string
nIndex = (uint32_t)p->h | ht->nTableMask;
Z_NEXT(p->val) = HT_HASH(ht, nIndex);
HT_HASH(ht, nIndex) = HT_IDX_TO_HASH(idx);
GC_DEL_FLAGS(ht, GC_NOT_COLLECTABLE);
ht->nNumOfElements++;
}

Expand Down