On Mon, 14 Sep 2020 20:45:29 GMT, Daniel D. Daugherty <dcu...@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
