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