mmtk / mmtk-core

Memory Management ToolKit
https://www.mmtk.io
Other
379 stars 69 forks source link

Only reset unlog bits of objects in modbuf for nursery GCs #1169

Open k-sareen opened 4 months ago

k-sareen commented 4 months ago

This fixes a bug wherein we were erroneously resetting the unlog bits for objects in the modbuf for full-heap GCs. If we reset the unlog bits in a full-heap GC, we may end up setting the unlog bit for an object that actually died which may cause issues for future allocations.

We now set the unlog bits for mature objects when tracing for VM space (if compiled with set_unlog_bits_vm_space), immortal space, and large object space.

This PR also adds debug assertions checking that any object that has been added to the modbuf is considered "live" by MMTk, or in other words, it is a mature object.

k-sareen commented 4 months ago

@wks Looks like OpenJDK actually triggers the assertion 😆 . I think the assertion is sound so this is actually an OpenJDK bug. The assertion is not triggered for ART.

k-sareen commented 3 months ago

Just fixed it like we discussed in a previous meeting

k-sareen commented 3 months ago

@wks Can you re-review this?

wks commented 3 months ago

If we reset the unlog bits in a full-heap GC, we may end up setting the unlog bit for an object that actually died which may cause issues for future allocations.

I assume the "issues for future allocations" means newly allocated (young) objects may have unlog bit set, and may be logged when written into, and we will retain more objects than necessary.

But I don't think it is sufficient to ensure this by skipping setting unlog bits in mature GCs. There may be dead mature objects that are not written to (i.e. unlogged). When those objects die, their unlog bits will remain set. If future allocations reuse the memory space of those objects, some young objects will also have unlog bits set.

If you want to ensure nursery objects never get logged, we may need to (bulk) clear unlog bits for reclaimed regions, like how we clear VO bits. I think this only affects StickyImmix because we never set the unlog bits for objects in the GenCopy or GenImmix nursery in the first place.

k-sareen commented 3 months ago

Hence why we do a bulk clear?

k-sareen commented 3 months ago

Oh. How odd. This PR doesn't do the bulk clear. I must have messed up when cherry-picking from my branch

wks commented 3 months ago

Instead of bulk clearing, it is also possible to copy the mark bits over to the unlog bits (and remove ImmixSpaceArgs::unlog_object_when_traced and ImmixSpaceArgs::reset_log_bit_in_major_gc). I think that's a bit like VO bits where there are two different strategies, and we found that it is faster to just copy over mark bits. We may refactor the code to unify the handling of unlog bits and VO bits.

p.s. I think the so-called "unlog bit" should really be named "is candidate for logging". Obviously nursery objects are not such candidates.

k-sareen commented 3 months ago

What that suggests is we need to be more systematic with how we deal with stale metadata values