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 outdated fields in PlanConstraints #588

Open
qinsoon opened this issue May 9, 2022 · 2 comments
Open

Remove outdated fields in PlanConstraints #588

qinsoon opened this issue May 9, 2022 · 2 comments
Labels
A-plan Area: Plan C-cleanup Category: Cleanup F-good-first-issue Call For Participation: Suitable issues for first-time contributors P-normal Priority: Normal.

Comments

@qinsoon
Copy link
Member

qinsoon commented May 9, 2022

PlanConstraints are constant for each plan. But some of its fields are unused, or superseded by other mechanisms.

moves_objects

pub moves_objects: bool,

moves_objects is superseded by may_move_objects() in PlanTraceObject, which is a constant value based on the trace kind. It is helpful for plans like Immix, for which, some traces (defrag) move objects and other traces (fast) do not move objects.

gc_header_bits and gc_header_words

pub gc_header_bits: usize,
pub gc_header_words: usize,

We now have metadata specs, like mark bit, log bit. We no longer need a plan to specify the GC bits/words they need.

num_specialized_scans

pub num_specialized_scans: usize,

This is unused. This seems related with object scanning, which is now in the bindings.

max_non_los_copy_bytes

pub max_non_los_copy_bytes: usize,

This is currently unused. We need to decide whether we need to check object size before copying (currently we assume we won't). The difference is that in one case, we could copy nursery->mature or nursery->LOS, and in the other case, we only copy nursery->mature (large objects are directly allocated into LOS' logical nursery). A related issue: #452.

needs_linear_scan

pub needs_linear_scan: bool,

Java MMTk uses this in object model. I am not sure how it works. We need to further check this. However, for MMTk core, I assume we will allow linear scan in some policies and when global_alloc_bit is enabled. So we may need to make sure this value is set properly.

needs_concurrent_workers

pub needs_concurrent_workers: bool,

Currently unused.

generate_gc_trace

pub generate_gc_trace: bool,

This should be removed. It is related to GCTrace in Java MMTk, and we do not plan to port it.

@qinsoon qinsoon added C-cleanup Category: Cleanup A-plan Area: Plan labels May 9, 2022
@k-sareen
Copy link
Collaborator

k-sareen commented May 10, 2022

I think, to me at least, part of why these values are useful is if I just want to compare two different plans. Comparing MarkCompact to SemiSpace, I can immediately see that oh MarkCompact uses 1 extra header word and also requires an extra scan in comparison to SemiSpace, instead of having to read its source code.

Though I'll note that we should fix how gc_header_words is used. This is something that I encountered when I was trying to add +1 extra word to header for SemiSpace. Simply increasing the gc_header_words in the plan constraint didn't work since currently the fastpath and everything is hardcoded only for MarkCompact. I will be submitting an issue for this for both mmtk-core and mmtk-openjdk.

@qinsoon qinsoon added the F-good-first-issue Call For Participation: Suitable issues for first-time contributors label May 14, 2023
@udesou udesou added the P-normal Priority: Normal. label Nov 15, 2023
@qinsoon
Copy link
Member Author

qinsoon commented Nov 23, 2023

#1028 removed the following fields:

  • gc_header_bits/words
  • num_specialized_scans
  • generate_gc_trace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-plan Area: Plan C-cleanup Category: Cleanup F-good-first-issue Call For Participation: Suitable issues for first-time contributors P-normal Priority: Normal.
Projects
None yet
Development

No branches or pull requests

3 participants