-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PM managed GC #6554
Merged
Merged
PM managed GC #6554
+44
−3
Conversation
This file contains 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
the reasoning for this was described in #5628
dktapps
added
Category: Core
Related to internal functionality
Status: Insufficiently Tested
Type: Enhancement
Contributes features or other improvements to PocketMine-MP
Performance
labels
Dec 2, 2024
GC threshold recalculation is supposed to use the root count _after_ GC, not before (presumably so it doesn't count non-cyclic objects).
Related issues: php/php-src#17127 (Acyclic objects landing in GC root buffer) |
I theorized that the number of roots removed should be a better heuristic than cycles, but in practice it caused GC to run a lot more often. The cost of expensive objects like Server is parasitic, so the less we run GC, the more the cost is amortized. There are ways we can eliminate Server's cost from GC, but that's a task for another time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Category: Core
Related to internal functionality
Performance
Status: Insufficiently Tested
Type: Enhancement
Contributes features or other improvements to PocketMine-MP
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.
This is a first look at #5628.
In a nutshell: Server
MemoryManager
now controls when GC is called. The mechanism by which it's activated remains the same as PHP's own, but instead of being called in random code paths & causing huge lag spikes, it gets called at a predictable location & can therefore be tracked in timings reports.This required re-implementing PHP's garbage collection threshold algorithm, as it's not possible to reliably make PHP update its own internal threshold when GC isn't always enabled.
A couple of considerations I came across while working on this. It seems like PHP's GC algorithm is rather suboptimal for PM's use case.
Server
getting unref'd causes a hit to GC performance becauseServer
will land in the GC root buffer, andServer
is a gateway to everything.Server
will frequently end up taking up space in the GC root buffer (whenever their refcounts go down), and then the GC wastes a ton of CPU time trying to scan them for dependencies. There's no real solution for this beyond requesting PHP to implement generations in the GC (A way to stop GC wasting CPU time on long-lived objects (GC generations or something else) php/php-src#17131).Follow-up