RFR: 8247281: migrate ObjectMonitor::_object to OopStorage [v4]

[Daniel D.Daugherty]

On Mon, 14 Sep 2020 20:34:13 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Sorry, no. Maybe it's too late here and I shall think about it tomorrow morning instead ;-) Or maybe you can explain it
>> again in the context of that change. How's the assert even relevant when moved in the else-branch?
>
> Sorry, I confused myself switching between this review and a
> preliminary review thread.
> 
> Here's the original code:
> 
> 165   while (cur_obj < scan_limit) {
> 166     assert(!space->scanned_block_is_obj(cur_obj) ||
> 167            oop(cur_obj)->mark_raw().is_marked() || oop(cur_obj)->mark_raw().is_unlocked() ||
> 168            oop(cur_obj)->mark_raw().has_bias_pattern(),
> 169            "these are the only valid states during a mark sweep");
> 170     if (space->scanned_block_is_obj(cur_obj) && oop(cur_obj)->is_gc_marked()) {
> 
> and here's the code after it was moved and rewritten:
> 
> 173     } else {
> 174       assert(!space->scanned_block_is_obj(cur_obj) || oop(cur_obj)->mark_raw().is_unlocked() ||
> 175              oop(cur_obj)->mark_raw().has_bias_pattern() || oop(cur_obj)->mark_raw().has_monitor(),
> 176              "these are the only valid states during a mark sweep");
> 
> 
> Where the assert() was worked fine when the ObjectMonitor had a regular oop,
> but after it was changed into a weak handle, that location became exposed to
> the fact that the object reference could be GC'ed. The original code assumes
> that the ObjectMonitor oop reference is stable and unchanging and that's no
> longer the case with the weak handle.

I found my original preliminary code review comment and Erik's reply:

> src/hotspot/share/gc/shared/space.inline.hpp
>     old L166:     assert(!space->scanned_block_is_obj(cur_obj) ||
>     old L167:            oop(cur_obj)->mark_raw().is_marked() || oop(cur_obj)->mark_raw().is_unlocked() ||
>     old L168: oop(cur_obj)->mark_raw().has_bias_pattern(),
>     old L169:            "these are the only valid states during a mark sweep");
>         This assert was before the if-statement at the top of the while-loop.
>
>     new L174: assert(!space->scanned_block_is_obj(cur_obj) || oop(cur_obj)->mark_raw().is_unlocked() ||
>     new L175: oop(cur_obj)->mark_raw().has_bias_pattern() || oop(cur_obj)->mark_raw().has_monitor(),
>     new L176:              "these are the only valid states during a mark sweep");
>         The assert is now in the else branch of the following if-statement:
>
>            L166     if (space->scanned_block_is_obj(cur_obj) && oop(cur_obj)->is_gc_marked()) {
>
>         The new assert() drops this check:
>
>             oop(cur_obj)->mark_raw().is_marked()
>
>         and adds this check:
>
>             oop(cur_obj)->mark_raw().has_monitor()
>
>         Dropping the "is_marked()" makes sense since the new location
>         of the assert is in the else branch of "oop(cur_obj)->is_gc_marked()".
>         The addition of the "has_monitor()" check is puzzling. Why was this
>         added and why wasn't it needed in the old assert()?
>
>         In fact, I'm not sure why this change is here at all.

This is an artifact of the monitor now being weak. Since there was previously always a strong root
to all inflated monitors, there were never any dead objects in the heap, that still had pointers in the
mark word to the monitor. The change to weak now implies that we suddenly have dead objects
in the heap, that in the markWord point out their monitor. GC code that iterates through consecutive
objects one by one, will see these now dead objects with monitors. The assert changes reflect that.
Before it was unexpected and would assert on that. Now I moved the assertion to the case when the
object is alive instead. We have no business asserting what should be in the markWord of dead objects.

I hope it makes sense now!

-------------

PR: https://git.openjdk.java.net/jdk/pull/135