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

Fixing sticky immix #121

Merged
merged 14 commits into from
Jan 29, 2024
Merged

Fixing sticky immix #121

merged 14 commits into from
Jan 29, 2024

Conversation

udesou
Copy link
Contributor

@udesou udesou commented Jan 24, 2024

This PR should be merged with mmtk/julia#32.

Copying the description here for simplicity:

At least one of the issues with sticky immix back ported to v1.9.2+RAI is that bindings may occur in the remset. However, bindings are currently skipped when scanned by MMTk since they are allocated as buffers. Therefore, we need a way to specify which bindings should actually be scanned since they were added via write barrier.

Possible sources of problems that haven't been verified yet:
(i) can the bits.gc value change after being set by the write barrier?
(ii) should we reset the value after the object has been scanned?

This PR adds code for scanning buffers that are Julia bindings that were added in the write barrier.

@udesou udesou requested a review from qinsoon January 24, 2024 05:38
obj.as_usize() - std::mem::size_of::<crate::julia_scanning::mmtk_jl_taggedvalue_t>();
let t_header = Address::from_usize(as_tagged_value).load::<Address>();
let tag = t_header.as_usize() & 3;
if tag == 2 {
Copy link
Member

Choose a reason for hiding this comment

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

I can see from the Julia code, that the write barrier for jl_binding_t sets the bits to 2. It means if jl_binding_t gets modified, the write barrier will set the bits to 2, and we will trace the fields in the jl_binding_t. You probably explained to me earlier why we need to do this, and why we don't need to trace fields in jl_binding_t if it is not in the remset. But I forgot the actual reason.

Can you write it into the comments why we need this code?

Copy link
Member

Choose a reason for hiding this comment

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

Is the bits cleared in any way? If not, that means once we sets the bits for a jl_binding_t object, the bits stay as 2, and we will trace its fields in every subsequent GCs.

Copy link
Member

Choose a reason for hiding this comment

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

I can see from the Julia code, that the write barrier for jl_binding_t sets the bits to 2. It means if jl_binding_t gets modified, the write barrier will set the bits to 2, and we will trace the fields in the jl_binding_t. You probably explained to me earlier why we need to do this, and why we don't need to trace fields in jl_binding_t if it is not in the remset. But I forgot the actual reason.

Can you write it into the comments why we need this code?

Oh I see. We don't scan buffers, as they will be scanned as a part of its parent object. But when a jl_binding_t buffer is inserted into remset, they have be to scanned. The gc bits tells us if the buffer is in the remset. Can you put this into comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the bits cleared in any way? If not, that means once we sets the bits for a jl_binding_t object, the bits stay as 2, and we will trace its fields in every subsequent GCs.

I've added a FIXME for now, but that's true. We should reset the bits after the jl_binding_t object has been scanned.

Copy link
Member

@qinsoon qinsoon Jan 28, 2024

Choose a reason for hiding this comment

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

Why not just clear the bits after the process_edge calls? Are you worrying that they will cause other issues? If they do (for example, if those bits are used by others for other purposes), it means the implementation is not sound.

@udesou udesou requested a review from qinsoon January 25, 2024 09:30
@qinsoon
Copy link
Member

qinsoon commented Jan 29, 2024

This PR only affects v1.9.2, not master.

Eduardo: they don't use buffers to store bindings in master. And IIRC they don't have the special write barrier either
Eduardo: I think they store them in a plain jl_array_t

@qinsoon qinsoon merged commit 7978ecf into mmtk:v1.9.2+RAI Jan 29, 2024
9 checks passed
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