Add MarkState. Use MarkState for ImmortalSpace.

[qinsoon]

This PR introduces MarkState. It abstracts over the mark state and the mark bit, and can be used by various policies. This PR changes ImmortalSpace to use MarkState. This PR also duplicates the current ImmortalSpace as a VMSpace policy to make sure it works fine with JikesRVM (I shall provide a new implementation of VMSpace soon).

[wks]

LGTM.

The Fn I mentioned in a comment can be changed to FnMut to make it more generally applicable, but it is still correct for now to use Fn.

There is an opportunity to replace cmpxchg with fetch_update. We can do it in the future.

[k-sareen]

Could you run a performance evaluation of this change?

[qinsoon]

Could you run a performance evaluation of this change?

Yeah. The change in the PR only affects immortal space, and immortal space is not used by OpenJDK. We could evaluate this with JikesRVM. Let me know if you think it is worthwhile to evaluate this on JikesRVM. @k-sareen

Most of the changes in the PR is simply refactoring. The only thing that may bring any overhead is the loop in prepare that tries to reset the state for every allocated region.

[qinsoon]

Alternatively, I will do further PRs that use MarkState introduced in this PR for other policies (e.g. immix), and I can evaluate with those PRs.

[k-sareen]

Ah that makes sense. I was just concerned about the overhead involved with an indirection for marking (which is one of the hottest parts of our code).

[qinsoon]

Ah that makes sense. I was just concerned about the overhead involved with an indirection for marking (which is one of the hottest parts of our code).

If you mean this line,

if self.mark_state.test_and_mark::<VM>(object) {

there is no actual indirection. MarkState is embedded in the struct ImmortalSpace, and the u8 state is actually the same address as before. There is no extra load. The call overhead should be optimized away by the compiler or PGO.

[qinsoon]

This PR is not ready for merge. It seems the PR makes JikesRVM tests time out.

[k-sareen]

Might be beneficial to test its performance on JikesRVM then. I don't know how much the Rust compiler will optimize away.

[qinsoon]

Might be beneficial to test its performance on JikesRVM then. I don't know how much the Rust compiler will optimize away.

I am debugging on this. This does not seem to be a performance issue though. It got stuck in the mutator phase for the semispace debug build, which is very early in the tests.

[qinsoon]

The issue with JikesRVM is about the VM space. We use ImmortalSpace as vm_space, so changes in ImmortalSpace affects VM space. I suspect JikesRVM hard coded some assumptions about the space in the boot image generation. Our ImmortalSpace before this PR is exactly the same as the ImmortalSpace in Java MMTk/JikesRVM, so it worked fine. This PR changed the behavior of our immortal space, so it breaks JikesRVM. This only affects the VM space, not other immortal spaces. Here are some more detailed comments about the issue:

mmtk-core/src/policy/vmspace.rs

Line 18 in b0c71b5

// We used to use ImmortalSpace as vm space up to commit 43e8a92b507ce9b8f771f31d2dbef7eee93f3cc2, and only

To work around the problem, I duplicate the ImmortalSpace before this PR as VMSpace, and use it for VM space. So VM space will use the old ImmortalSpace which works fine for JikesRVM, and other immortal spaces use the new implementation. This should solve the issue for now. I will later provide a new implementation of the VMSpace (as it is needed for Julia).

[qinsoon]

@k-sareen @wks Please see the above comments, and my new commits. Let me know what you think.

[k-sareen]

I don't understand why JikesRVM is failing though. Should this not be a drop-in replacement for an ImmortalSpace as the PR mainly refactors code?

[k-sareen]

Oh. I see. The main difference is that the mark_state is 1 instead of 0. Hmm. I think we should discuss this with Steve first before we merge. I don't understand why the initial state should matter if all object metadata calls are going through the Rust MMTk framework.

[qinsoon]

Oh. I see. The main difference is that the mark_state is 1 instead of 0. Hmm. I think we should discuss this with Steve first before we merge. I don't understand why the initial state should matter if all object metadata calls are going through the Rust MMTk framework.

Based on my observation, clearly something is not going through Rust MMTk. I am not sure if they use Java MMTk or they hard code some assumptions about the space during boot image generation.

[k-sareen]

Quick sanity check (if you haven't already) I guess is changing the mark_state back to 0 to see if it fixes the issue? Perhaps we can just make the initial mark_state as a configurable parameter. I presume some VMs may want all 0s instead of all 1s. It might be cleaner than duplicating ImmortalSpace code.

EDIT: Though I don't know if exposing internal state (the initial mark_state) to the VM(-binding) is a good idea.

[qinsoon]

Quick sanity check (if you haven't already) I guess is changing the mark_state back to 0 to see if it fixes the issue? Perhaps we can just make the initial mark_state as a configurable parameter. I presume some VMs may want all 0s instead of all 1s. It might be cleaner than duplicating ImmortalSpace code.

EDIT: Though I don't know if exposing internal state (the initial mark_state) to the VM(-binding) is a good idea.

Simply changing mark_state does not fix the problem. There are other changes in this PR as well. If we start from master, and simply change the initial mark_state for vm_space, it immediately breaks JikesRVM with the same issue. Also if we change mark_state for all the spaces other than vm_space, there is no problem. So I concluded that JikesRVM has some assumptions about the VM space.

I duplicate ImmortalSpace as VMSpace based on the fact that we will need a VMSpace anyway. Using ImmortalSpace as vm_space was just a workaround. I was planning to introduce VMSpace (see my branch for Julia: https://github.com/qinsoon/mmtk-core/blob/julia-misc/src/policy/vmspace.rs), though I was planning to implement VMSpace by reusing ImmortalSpace.

Anyway, it seems to be an issue with our JikesRVM binding (and our JikesRVM fork). If that is the case, it is not a priority for me right now.

[qinsoon]

I reverted the change about duplicating ImmortalSpace. The problem is about incorrect mark bit, and about the VM space. But the problem is not in the JikesRVM side. I used to reset the mark bit by iterating through pr.for_allocated_regions(), and reset for each region. However, there is no allocation in the VM space, so for_allocated_regions does not return any thing and mark bit was not actually reset at all for VM space. I changed it: if we are doing prepare for VM space, we reset the address range for the space; otherwise, we reset allocated regions.

[k-sareen]

That makes sense 👍. Thanks for debugging it!