On Mon, 14 Sep 2020 20:48:10 GMT, Coleen Phillimore <[email protected]> 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
