Skip to content

Allow configuring each Immix space to be non moving #1305

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Apr 17, 2025

We plan to use non-moving Immix as a non-moving space. In this case, we will have two Immix spaces, one allows moving, and the other does not. We currently use global constants to control whether Immix moves objects or not, and we cannot have two Immix spaces that are configured differently.

This PR removes those constants, and adds a flag never_move_objects in ImmixSpaceArgs to indicate whether this Immix space disallows moving.

@qinsoon qinsoon force-pushed the immix-nonmoving-instance branch from 61d26db to a965bb8 Compare April 22, 2025 04:08
@qinsoon qinsoon force-pushed the immix-nonmoving-instance branch from a965bb8 to 530d80c Compare April 22, 2025 04:21
@qinsoon qinsoon marked this pull request as ready for review April 23, 2025 00:07
@qinsoon qinsoon requested a review from wks April 23, 2025 00:07
@@ -139,6 +140,7 @@ impl<VM: VMBinding> Immix<VM> {
unlog_object_when_traced: false,
#[cfg(feature = "vo_bit")]
mixed_age: false,
never_move_objects: cfg!(feature = "immix_non_moving"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line never_move_objects: cfg!(feature = "immix_non_moving") is repeated three times for Immix, GenImmix and StickyImmix. But the semantics of the Cargo feature "immix_non_moving" is disallowing any ImmixSpace instance to move objects. Repeating cfg!(feature = "immix_non_moving") imposes a danger that in the future, as we add more spaces or plans, we may miss one or two of this, and accidentally assign true to never_move_objects when the "immix_non_moving" feature is enabled.

I suggest we allow plans to set never_move_objects according to the requirements in the "normal" scenario (i.e. the main ImmixSpace is movable, but the dedicated non-moving space is non-movable), and let ImmixSpace::new override its value according to cfg!(feature = "immix_non_moving"). In this way, we guarantee never_move_objects == false if the "immix_non_moving" feature is enabled.

Suggestion:

Suggested change
never_move_objects: cfg!(feature = "immix_non_moving"),
never_move_objects: true,

But override it in ImmixSpace::new.

args.name,
Block::LOG_BYTES
);
if space_args.never_move_objects {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can make space_args mutable

-space_args: ImmixSpaceArgs,
+mut space_args: ImmixSpaceArgs,

and override never_move_objects here if cfg!(feature = "immix_non_moving") is true. We can also print a log here, telling the user that this is overridden.

Comment on lines 284 to 288
info!(
"Creating non-moving ImmixSpace: {}. Block size: 2^{}",
args.name,
Block::LOG_BYTES
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This existing info! log here was intended to warn the user that the user is forcing the ImmxSpace not to move objects, which may hurt the performance. We can still print this log if the user overrides never_move_objects with cfg!(feature = "immix_non_moving"). Maybe raise it to the warn! level.

But if the current ImmixSpace is the dedicated non-moving space, it may not be worth logging this at the info! level because it is a normal thing.

@qinsoon qinsoon force-pushed the immix-nonmoving-instance branch from 804f43b to cf726b4 Compare April 24, 2025 05:25
@qinsoon qinsoon force-pushed the immix-nonmoving-instance branch from cf726b4 to 2ecb7a3 Compare April 24, 2025 06:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants