Skip to content

Remove the immix_stress_copying feature. Replace with separate options.#1324

Merged
qinsoon merged 4 commits intommtk:masterfrom
qinsoon:refactor-immix-stress-defrag-features
May 21, 2025
Merged

Remove the immix_stress_copying feature. Replace with separate options.#1324
qinsoon merged 4 commits intommtk:masterfrom
qinsoon:refactor-immix-stress-defrag-features

Conversation

@qinsoon
Copy link
Member

@qinsoon qinsoon commented May 20, 2025

immix_stress_copying may cause confusion with the stress GC and the stress factor, as immix_stress_copying does not imply stress GCs. This PR removes immix_stress_copying, and replaces with separate options to control the specific behavior.

@qinsoon qinsoon force-pushed the refactor-immix-stress-defrag-features branch from dbf67eb to a2682e4 Compare May 20, 2025 06:38
@qinsoon qinsoon force-pushed the refactor-immix-stress-defrag-features branch from a2682e4 to 66eef3b Compare May 20, 2025 06:40
@qinsoon qinsoon marked this pull request as ready for review May 20, 2025 07:23
@qinsoon qinsoon requested a review from wks May 20, 2025 07:23
@qinsoon qinsoon changed the title Remove immix_stress_copying. Replace with immix_stress_defrag and immix_defrag_every_block Remove the immix_stress_copying feature. Replace with separate options. May 20, 2025
Comment on lines +6 to +7
One way to make copying more deterministic is to use the following options to
change the copying behavior of Immix.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the reader is new to MMTk, they may wonder how to set options. Providing a link to mmtk::util::options::Options should be helpful. We can also add some examples that set the options by setting environment variables.

Comment on lines +907 to +912
/// Mark every allocated block as defragmentation source before GC. (for debugging)
immix_defrag_every_block: bool [always_valid] = false,
/// Percentage of heap size reserved for defragmentation.
/// According to [this paper](https://doi.org/10.1145/1375581.1375586), Immix works well with
/// headroom between 1% to 3% of the heap size.
immix_defrag_headroom_percent: usize [|v: &usize| *v <= 50] = 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Merely setting immix_defrag_every_block = true will make it run out of headroom before copying every live object. We should mention, in either immix_defrag_every_block or immix_defrag_headroom_percent or both, that we need to set immix_defrag_headroom_percent = 50 if we want to stress copying. We can also provide a link to the porting guide.

Copy link
Member Author

@qinsoon qinsoon May 21, 2025

Choose a reason for hiding this comment

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

This is a good point. So setting immix_defrag_every_block to true alone does not make sense.

What if we set the defrag headroom to 50% but we do not set immix_defrag_every_block? I assume the defrag also works, as it will still defrag enough blocks (which should be all the blocks) to use the headroom.

I am wondering if we really need an option for immix_defrag_every_block.

@qinsoon
Copy link
Member Author

qinsoon commented May 21, 2025

I have made changes to address the reviews. Can you take a look again? @wks

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.

The mmtk-ruby binding currently has a feature named immix_stress_copying which is just a wrapper of the mmtk-core feature fo the same name. I'll remove it from mmtk-ruby once this PR is merged.

@qinsoon qinsoon enabled auto-merge May 21, 2025 05:42
@qinsoon qinsoon added this pull request to the merge queue May 21, 2025
Merged via the queue into mmtk:master with commit d538987 May 21, 2025
31 of 32 checks passed
@qinsoon qinsoon deleted the refactor-immix-stress-defrag-features branch May 21, 2025 07:34
wks added a commit to wks/mmtk-ruby that referenced this pull request May 21, 2025
The feature of the same name has been removed in mmtk-core.

See: mmtk/mmtk-core#1324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants