Skip to content
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

Reduce gc stack usage for strings (and resources) #17194

Closed
wants to merge 2 commits into from

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Dec 17, 2024

Adding strings to the worklist is useless, because they never contribute to cycles. The assembly size on x86_64 does not change. This significantly improves performance in this synthetic benchmark by 33%.

function test($a) {}

$a = new stdClass();
$a->self = $a;
$a->prop1 = str_repeat('a', 10);
$a->prop2 = str_repeat('a', 10);
$a->prop3 = str_repeat('a', 10);
$a->prop4 = str_repeat('a', 10);
$a->prop5 = str_repeat('a', 10);
$a->prop6 = str_repeat('a', 10);
$a->prop7 = str_repeat('a', 10);
$a->prop8 = str_repeat('a', 10);
$a->prop9 = str_repeat('a', 10);
$a->prop10 = str_repeat('a', 10);

for ($i = 0; $i < 10_000_000; $i++) {
    test($a);
    gc_collect_cycles();
}

This requires adding IS_TYPE_COLLECTABLE to IS_REFERENCE_EX to ensure these values continue to be pushed onto the stack. Luckily, IS_TYPE_COLLECTABLE is currently only used in gc_check_possible_root(), where the checked value cannot be a reference.

Note that this changes the output of gc_collect_cycles(). Non-cyclic, refcounted values no longer count towards the total reported values collected.

Also, there is some obvious overlap with GH-17130. This change should be good nonetheless, especially if we can remove the GC_COLLECTABLE(Z_COUNTED_P(zv)) condition in PHP 9 and rely on Z_COLLECTABLE_P() exclusively, given we can assume an object doesn't become cyclic at runtime anymore.

Adding strings to the worklist is useless, because they never contribute to
cycles. The assembly size on x86_64 does not change. This significantly improves
performance in this synthetic benchmark by 33%.

    function test($a) {}

    $a = new stdClass();
    $a->self = $a;
    $a->prop1 = str_repeat('a', 10);
    $a->prop2 = str_repeat('a', 10);
    $a->prop3 = str_repeat('a', 10);
    $a->prop4 = str_repeat('a', 10);
    $a->prop5 = str_repeat('a', 10);
    $a->prop6 = str_repeat('a', 10);
    $a->prop7 = str_repeat('a', 10);
    $a->prop8 = str_repeat('a', 10);
    $a->prop9 = str_repeat('a', 10);
    $a->prop10 = str_repeat('a', 10);

    for ($i = 0; $i < 10_000_000; $i++) {
        test($a);
        gc_collect_cycles();
    }
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Nice!

This looks good to me. @dstogov what do you think?

Zend/zend_gc.c Outdated Show resolved Hide resolved
Zend/zend_gc.c Outdated Show resolved Hide resolved
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

This looks great!
Only one question: does this work properly with non-COLLECTABLE objects?

@iluuu1994
Copy link
Member Author

iluuu1994 commented Dec 18, 2024

Only one question: does this work properly with non-COLLECTABLE objects?

Can you clarify? I believe for objects, IS_TYPE_COLLECTABLE flag is currently always set and GC_NOT_COLLECTABLE always not set. This would only change with GH-17130 if merged. However, the Z_COLLECTABLE_P() check will unfortunately not be enough. That PR marks classes as acyclic at compile time by looking at property types, and then set GC_NOT_COLLECTABLE on instantiation. Until PHP 9, instances of acyclic classes may still become cyclic at runtime through dynamic properties. When adding a dynamic property, GC_NOT_COLLECTABLE is re-added to the object. However, we cannot track this on zvals because the object may be referenced from multiple unknown locations. So, in that PR, we extend the GC with another check (GC_COLLECTABLE(Z_COUNTED_P(zv))). I have not yet checked how expensive that check is.

@dstogov
Copy link
Member

dstogov commented Dec 18, 2024

Can you clarify? I believe for objects, IS_TYPE_COLLECTABLE flag is currently always set and GC_NOT_COLLECTABLE always not set. This would only change with GH-17130 if merged.

Right. I ask, can't this make problems for "acyclic objects"?

Now GC collects all the garbage objects then call their destructors and then destroy them. With this patch "acyclic objects" are not going to be recognized as garbage (they will become "children" of the garbage). Therefore they are going to be destructed a bit differently.

@iluuu1994
Copy link
Member Author

@dstogov Ah, I see. Yes, you are correct that the destruction is potentially different if acyclic objects are not part of the GC buffer, in that the objects will be destroyed through zend_object_std_dtor() of the parent object. This should not cause any issues though, because by-definition, the child does not have a reference to any cyclic objects. Still, it might be reasonable to consider objects with destructors cyclic, just so that the destructor is called during the destructor phase in the GC. Anyway, I think that's something to discuss in the other PR.

@iluuu1994 iluuu1994 closed this in e69317b Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants