[master] RFR: Simplify stable_mark() routine [v2]

[Thomas Stuefe]

On Mon, 11 Apr 2022 19:32:14 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> The code in ObjectSynchronizer::stable_mark() is a bit over-complicated: it has a 'fast' path (which really isn't a fast-path, because the really-fast path is already in the various oopDesc::klass() methods), and for the slow-path loop dives into ObjectSynchronizer::safe_load_mark(). This change proposes to remove the stable_mark() method, and rename safe_load_mark() to stable_mark() instead.
>> 
>> I also propose a small improvement: we can use read_stable_mark() to deal with INFLATING mark properly, and handle the remaining cases in the loop: monitor and stack-locked are the most common - deal with them first. Stack-locked can safely access the mark when current thread is owning thread of the lock - avoid CASing INFLATING into the header in this case. Neutral and Marked cases go last, because they should not be very common (neutral is already caught in oopDesc::klass() variants, marked should only happen during full GCs by GC internal code).
>> 
>> Testing:
>>  - [x] tier1
>>  - [x] tier2
>>  - [x] tier3
>>  - [x] tier4
>
> Roman Kennke has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge branch 'master' into simplify-safemark
>  - Simplify stable_mark() routine

Hi,

looked through this and also the old version (I had missed that) and it looks fine. Interesting trick of temporary set the mark to "installing". I guess there is no code being surprised by the new state change (installing->locked)? Or could that happen before too?

Small remarks inline, but LGTM.

src/hotspot/share/oops/markWord.hpp line 192:

> 190:   }
> 191:   bool has_monitor() const {
> 192:     return ((value() & monitor_value) != 0);

So, the old version returned true for "monitor" (10) and for "marked" (11) both? Seems like a bug. Would it be worth fixing it independently, maybe upstream?

src/hotspot/share/runtime/synchronizer.cpp line 852:

> 850:       object->release_set_mark(mark);
> 851: 
> 852:       return dmw;

Question, why is above guarantee a guarantee, not an assert? You want to be super sure here also in release builds? Do you plan to drop down to assert() eventually?

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

Marked as reviewed by stuefe (Committer).

PR: https://git.openjdk.java.net/lilliput/pull/46