[Bug #20022] Fix GC.verify_compaction_references not moving every object #9041
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
KJTsanaktsidis
changed the title
KJTsanaktsidis
force-pushed
the
ktsanaktsidis/fix_verify_compaction
branch
2 times, most recently
from
34b9bd0
to
0b5af26
Compare
This works like objspace_each_obj, except instead of being called with the start & end address of each page, it's called with the page structure itself. [Bug #20022]
KJTsanaktsidis
force-pushed
the
ktsanaktsidis/fix_verify_compaction
branch
from
0b5af26
to
bea23b6
Compare
peterzhu2118
approved these changes
Dec 4, 2023
gc.c
Outdated
|
|
||
| heap_add_pages(objspace, size_pool, heap, pages_to_add); | ||
| } | ||
| } else if (RTEST(double_heap)) { |
There was a problem hiding this comment.
Nit: style.
Suggested change
| } else if (RTEST(double_heap)) { | |
| } | |
| else if (RTEST(double_heap)) { |
The intention of GC.verify_compaction_references is, I believe, to force every single movable object to be moved, so that it's possible to debug native extensions which not correctly updating their references to objects they mark as movable. To do this, it doubles the number of allocated pages for each size pool, and sorts the heap pages so that the free ones are swept first; thus, every object in an old page should be moved into a free slot in one of the new pages. This worked fine until movement of objects _between_ size pools during compaction was implemented. That causes some problems for verify_compaction_references: * We were doubling the number of pages in each size pool, but actually if some objects need to move into a _different_ pool, there's no guarantee that they'll be enough room in that one. * It's possible for the sweep & compact cursors to meet in one size pool before all the objects that want to move into that size pool from another are processed by the compaction. You can see these problems by changing some of the movement tests in test_gc_compact.rb to try and move e.g. 50,000 objects instead of 500; the test is not able to actually move all of the objects in a single compaction run. To fix this, we do two things in verify_compaction_references: * Firstly, we add enough pages to every size pool to make them the same size. This ensures that their compact cursors will all have space to move during compaction (even if that means empty pages are pointlessly compacted) * Then, we examine every object and determine where it _wants_ to be compacted into. We use this information to add additional pages to each size pool to handle all objects which should live there. With these two changes, we can move arbitrary amounts of objects into the correct size pool in a single call to verify_compaction_references. My _motivation_ for performing this work was to try and fix some test stability issues in test_gc_compact.rb. I now no longer think that we actually see this particular bug in rubyci.org, but I also think verify_compaction_references should do what it says on the tin, so it's worth keeping. [Bug #20022]
KJTsanaktsidis
force-pushed
the
ktsanaktsidis/fix_verify_compaction
branch
from
bea23b6
to
9981254
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The intention of
GC.verify_compaction_referencesis, I believe, to force every single movable object to be moved, so that it's possible to debug native extensions which not correctly updating their references to objects they mark as movable.To do this, it doubles the number of allocated pages for each size pool, and sorts the heap pages so that the free ones are swept first; thus, every object in an old page should be moved into a free slot in one of the new pages.
This worked fine until movement of objects between size pools during compaction was implemented. That causes some problems for
verify_compaction_references:You can see these problems by changing some of the movement tests in test_gc_compact.rb to try and move e.g. 50,000 objects instead of 500; the test is not able to actually move all of the objects in a single compaction run.
To fix this, we do two things in
verify_compaction_references:With these two changes, we can move arbitrary amounts of objects into the correct size pool in a single call to verify_compaction_references.
My motivation for performing this work was to try and fix some test stability issues in test_gc_compact.rb. I now no longer think that we actually see this particular bug in rubyci.org, but I also think
verify_compaction_referencesshould do what it says on the tin, so it's worth keeping.[Bug #20022]