GC optimization for primitive arrays

[iluuu1994]

PoC for GH-19608

[iluuu1994]

/cc @arnaud-lb, given you've worked on GC the most (apart from Dmitry),

[mvorisek]

empty array should be GC_NOT_COLLECTABLE from start

[mvorisek]

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

[iluuu1994]

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.

[mvorisek]

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.

[arnaud-lb]

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.

[mvorisek]

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

[iluuu1994]

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.

[arnaud-lb]

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

[dktapps]

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.

[iluuu1994]

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().

[dktapps]

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.

[arnaud-lb]

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

[iluuu1994]

Oh, very good catch.