Region GC#105
Conversation
|
For now, I only checked the last commit (based) on the description. I plan to look at the rest later. The fact that this PR is this small is super impressive and a testament for how well you understood the whole thing and wired it up! I'm looking forward to reviewing it :D |
xFrednet
left a comment
xFrednet
left a comment
There was a problem hiding this comment.
Excellent, the code is super readable and concise. I'm pretty confident that I can modify it or that we can give this to someone else to test different scheduling mechanics. Very well done, thank you for all the work you put into this!
I have some comments, but mainly some clarifying questions. I didn't find any logic bugs :D
| PyThreadState *tstate = PyThreadState_Get(); | ||
| if (!_PyGC_CanRunRegionGC(tstate)) { |
There was a problem hiding this comment.
The docs of PyThreadState_Get(); state that it can return NULL. But this code doesn't null check. Does it expect _PyGC_CanRunRegionGC to do the NULL check? If so, can you add a comment about this?
There was a problem hiding this comment.
Where did you find that?
These docs say the opposite.
There was a problem hiding this comment.
Sorry, I mixed this up with PyInterpreterState_Get. You're right that this should be safe :)
| /* Separate child regions from contained objects. | ||
| * Finalizers need them to be in the GC list, at the start of the list. | ||
| */ | ||
| PyGC_Head contained; | ||
| gc_list_init(&contained); | ||
| gc_region_list_split(&data->gc_list, &contained); | ||
|
|
There was a problem hiding this comment.
With all of these additional splits and joins I see why you asked if we can split the contained object and child-region lists. For now, I want to keep this structure since it works, but this should definitely be on the "benchmark this" list for when the implementation is complete-ish.
The implementation thus far is super readable, so this change should be easy to in cooperate into your implementation.
There was a problem hiding this comment.
On the other hand, the GC iterates over all children anyway when traversing the tree. So this just means we iterate twice instead of once. That might be a good tradeoff for 16 bytes saved in the region data.
There was a problem hiding this comment.
This will be worth benchmarking at one point :)
| */ | ||
| gcstate->region_budget += 3; | ||
| if (gcstate->region_budget > gcstate->young.threshold) { | ||
| gcstate->region_budget = gcstate->young.threshold; |
There was a problem hiding this comment.
I assume this is one of the places we can modify to adjust the scheduling?
For a steady state heap, the amount of work to do is three times
Where does the "three times" come from, is this a different comment in this file or some benchmarking?
There was a problem hiding this comment.
This is taken from the comment in assess_work_to_do. I came to the conclusion that it is correct.
| // TODO(regions): If callback moves op into a region, | ||
| // this function would start iterating objects in the region. |
There was a problem hiding this comment.
I assume this TODO is if op is local?
We can later mark the object as unmovable, when we support this on a per-instance level :)
There was a problem hiding this comment.
This is related to PyUnstable_GC_VisitObjects, which is an unstable API function that calls a given callback for every object tracked by the local GC.
So yes, op is local. I guess you could mark it unmovable, do the callback, and then restore movability?
There was a problem hiding this comment.
Yep, that was the thing I was considering. For immutability, we have a way to temporary set an object unfreezable and then reset the value again
xFrednet
left a comment
xFrednet
left a comment
There was a problem hiding this comment.
Thank you for answering all my questions and updating the minor nits. This is excellent!
Let get this merged and on to regions-main! I'm really happy how this turned out! :D
2 participants
Please check the last commit in particular, I still don't understand the write barrier enough 😅