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

[Daniel D.Daugherty]

On Mon, 14 Sep 2020 20:48:10 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

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

@fisk - please chime in here...

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

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