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

Fix issues in sanity GC #1079

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Fix issues in sanity GC #1079

merged 4 commits into from
Jan 31, 2024

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Jan 30, 2024

This PR fixes two issues in sanity GC.

  • It fixes Sanity GC is broken #1078 by removing the code to prepare/release mutator/collector, which seems unnecessary.
  • It replaces the use of ScanObjects for cached root nodes with ProcessRootNode. Otherwise, the assertion assert!(!self.roots()) will fail in ScanObjectsWork::do_work_common.

@qinsoon qinsoon requested a review from wks January 30, 2024 05:05
@wks
Copy link
Collaborator

wks commented Jan 30, 2024

ScanObjectsWork::do_work_common used to contain code for adding root nodes into the sanity checker. Since 61d20e2, the processing of root nodes (the objects pointed by root edges from outside the object graph, i.e. pointed by stack slots and global variables) is moved to ProcessRootNode, and ScanObjects never processes any "root nodes". Therefore, it is right to replace the use of ScanObjects with ProcessRootNode. (I wonder why 61d20e2 changed the code related to sanity GC but didn't change the actual SanityChecker.)

I also think it is safe to remove the roots() method from the ScanObjectsWork trait, and the roots field from both ScanObjects and PlanScanObjects. The roots() method was only used by the code related to sanity checking. The fact that it is asserted to be false in do_work_common means it is not used now.

@qinsoon
Copy link
Member Author

qinsoon commented Jan 30, 2024

I also think it is safe to remove the roots() method from the ScanObjectsWork trait, and the roots field from both ScanObjects and PlanScanObjects. The roots() method was only used by the code related to sanity checking. The fact that it is asserted to be false in do_work_common means it is not used now.

I removed roots() in the trait and the fields in its implementations.

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM

@wks
Copy link
Collaborator

wks commented Jan 30, 2024

There are some example code in the documentation that still uses the roots parameter.

@qinsoon
Copy link
Member Author

qinsoon commented Jan 31, 2024

binding-refs
OPENJDK_BINDING_REPO=qinsoon/mmtk-openjdk
OPENJDK_BINDING_REF=add-test-for-sanity-gc

@qinsoon qinsoon added the PR-extended-testing Run extended tests for the pull request label Jan 31, 2024
@qinsoon qinsoon added this pull request to the merge queue Jan 31, 2024
Merged via the queue into mmtk:master with commit 092b756 Jan 31, 2024
28 of 29 checks passed
@qinsoon qinsoon deleted the fix/sanity-gc-issues branch January 31, 2024 22:30
qinsoon added a commit to qinsoon/mmtk-core that referenced this pull request Feb 12, 2024
This PR fixes two issues in sanity GC.
* It fixes mmtk#1078 by removing the
code to prepare/release mutator/collector, which seems unnecessary.
* It replaces the use of `ScanObjects` for cached root nodes with
`ProcessRootNode`. Otherwise, the assertion `assert!(!self.roots())`
will fail in `ScanObjectsWork::do_work_common`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-extended-testing Run extended tests for the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sanity GC is broken
2 participants