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

Rename "alloc bit" to "valid-object bit" (VO bit), the second attempt. #791

Merged
merged 11 commits into from
Apr 16, 2023

Conversation

wks
Copy link
Collaborator

@wks wks commented Apr 13, 2023

My previous PR #680 tried to do too many things in one PR, and I didn't feel I could finish all the work in the short term.

This PR only renames "alloc bit" to "VO bit", and is intended to quickly remove the notion of "alloc bit" from mmtk-core. Cargo features, identifier, strings and comments are renamed. But this PR does not change how we use (or misuse) the VO bit metadata.

Things that are not done in this PR include:

  • Clearing VO bit: This feature is useful for marking an object dead before transitive closure, and prevent MMTk from scanning it. It is useful for VMs like Ruby which destroys the type metadata when finalising objects.
  • Removing the is_mmtk_object feature: The is_mmtk_object method should be available as long as the VO bit metadata is present.
  • Only allow accessing VO bit through ObjectReference: Because VO bit is set on the address of the ObjectReference, we should refactor the API to emphasise this and prevent misuse.
  • Move MarkCompact away from VO bit: MarkCompact needs a local (specific to the MarkCompact space) metadata to record where allocated objects (and buffers if we implement disjoint objects, too). It shouldn't use the global VO bit metadata.

Those changes will be left to other PRs.

@wks wks changed the title Rename "alloc bit" to "VO bit" Rename "alloc bit" to "valid-object bit" (VO bit), the second attempt. Apr 13, 2023
@wks wks requested a review from qinsoon April 13, 2023 16:58
Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

Generally good. But I think we should use valid object instead of VO in the comments, keep mentioning VO without explaining it could be confusing.

We also need to update the bindings (at least the OpenJDK binding). I think it uses the feature, as long with the base address for the side metadata (ALLOC_SIDE_METADATA_ADDR/VO_BIT_SIDE_METADATA_ADDR).

Cargo.toml Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
src/util/metadata/vo_bit.rs Outdated Show resolved Hide resolved
Ensure the full name is used when first mentioned in public interfaces.
@@ -5,6 +5,9 @@ use crate::vm::ObjectModel;
use crate::vm::VMBinding;
use std::marker::PhantomData;

// FIXME: This linear scanning algorithm is only used by MarkCompact. It should use a local
Copy link
Member

Choose a reason for hiding this comment

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

Currently linear scan can be used by any plan or by the binding (as a utility/debug tool) as long as vo bit is turned on. I don't know if it will be useful or not. Maybe we can engineer the linear scan code to take a side metadata spec as an argument (or const generic).

But you are right -- mark compact probably should use its own local metadata rather than the VO bit.

Malloc mark sweep also uses the vo bit. But I remember that we have some difficulties of using local metadata for it. We have to use the global vo/alloc bit in malloc mark sweep.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rephrased this comment a little bit.

Yes. Malloc mark sweep and MarkCompact are the two cases which I think are misuses of the VO bit metadata. As for malloc space, it needs a global metadata that has different semantics than the VO bit. For example, if the user explicitly clears the VO bit to indicate that "the object's type metadata is destroyed and the GC shouldn't scan it or it will crash", malloc still needs a way to know it has an allocation at that address so that it can free it. We may consider adding another per-16-bit (granularity of malloc) global metadata, and we may consider making the VO bit a tri-state metadata where the third state means "there is an allocation, but is no longer valid".

@wks
Copy link
Collaborator Author

wks commented Apr 14, 2023

binding-refs
OPENJDK_BINDING_REPO=wks/mmtk-openjdk
OPENJDK_BINDING_REF=rename-to-vo-bit2

@wks wks added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Apr 14, 2023
@wks
Copy link
Collaborator Author

wks commented Apr 14, 2023

The OpenJDK binding test passed. The V8 binding failed, but the failure doesn't appear to be related to this change.

@wks wks merged commit df146b7 into mmtk:master Apr 16, 2023
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants