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

Remove or refactor GcStatus #1061

Open
wks opened this issue Jan 2, 2024 · 1 comment
Open

Remove or refactor GcStatus #1061

wks opened this issue Jan 2, 2024 · 1 comment
Labels
P-normal Priority: Normal.

Comments

@wks
Copy link
Collaborator

wks commented Jan 2, 2024

The GcStatus type is defined as following:

pub enum GcStatus {
    NotInGC,
    GcPrepare,
    GcProper,
}

It is inherited from JikesRVM. The status is set during different phases of a GC.

  • It is set to GCPrepare
    • in ScheduleCollection in Rust MMTk, and
    • in the INITIATE phase in JikesRVM.
  • It is set to GCProper
    • when all mutator stacks have been scanned in Rust MMTk, and
    • in the end of the STACK_ROOTS and ROOTS phases in JikesRVM. Also set to GC_PROPER in processPhaseStack
  • It is set to NotInGC
    • right before calling resume_mutators in Rust MMTk, and
    • in the COMPLETE phase in JikesRVM. Also set to NOT_IN_GC in processPhaseStack

But its value is never read in the Rust MMTK. Neither MMTK::gc_in_progress() nor MMTk::gc_in_progress_proper() is called anywhere in the Rust MMTk or any of its bindings.

In JikesRVM,

  • gcInProgress() is only used in assertions, especially for concurrent plans.
  • gcInProgressProper() is only called in modifyCheck which no longer exists in the Rust MMTk.

It looks like the GC status mainly helps in concurrent GC algorithms, but the lxr branch of Rust MMTk does not read the gc_status field, either.

So I think GcStatus can be safely removed from MMTK without any harm.

Repurposing

When I was trying to remove the coordinator thread (See #1053), I realized that with the coordinator thread removed, we need a state machine to record the GC status, in a more fine-grained way than the current GcStatus and the current fields of GCReqeuster, and the states will affect the behavior of several things, including triggering GC, and the behavior of the last parked GC worker (to open new buckets or to wait for the GC to be triggered). I think we can re-purpose the GcStatus type for this.

(Update: #1067 which fixes #1053 now uses its own state for that purpose, and will not affect this issue.)

@wks
Copy link
Collaborator Author

wks commented Jan 12, 2024

Actually it is not "unused". Its most important use is the function MMTK::set_gc_status itself. This function marks the start and end of each GC, and is used to distinguish between mutator time and GC time in statistics. Such statistics is part of the output of harness_end.

@qinsoon qinsoon added the P-normal Priority: Normal. label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-normal Priority: Normal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants