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

[Coleen Phillimore]

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

>> 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!

This code seems like something that doesn't belong here anymore.  This code assumed synchronous scanning of oops in
ObjectMonitor and scanning memory regions, and that's no longer the case with OopStorage. I think this assert should be
removed.  It exports some implementation detail of now completely unrelated code in order to do a very specific check.

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

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