RFR: 8291714: Implement a Multi-Reader Single-Writer mutex for Hotspot [v2]

[Stefan Karlsson]

On Fri, 19 Aug 2022 14:50:11 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> I think the pattern of passing a nullptr argument to Lockers is a pattern that we reach for quite often. The benefit is that it allows for a conditional scope, without messing up the surrounding code.
>> 
>> Compare
>> 
>> Locker locker(should_lock ? &_lock : nullptr);
>> doit1();
>> doit2();
>> doit3();
>> 
>> vs
>> 
>> if (should_lock) {
>>   Locker locker(&_lock);
>>   doit1();
>>   doit2();
>>   doit3();
>> } else {
>>   doit1();
>>   doit2();
>>   doit3();
>> }
>> 
>> or, having to go extra miles to pull out the code into helper functions:
>> 
>> void MyClass::doall() {
>>   doit1();
>>   doit2();
>>   doit3();
>> }
>> ...
>> if (should_lock) {
>>   Locker locker(&_lock);
>>   doall();
>> } else {
>>   doall();
>> }
>> 
>> 
>> The first version is much easier to read, IMHO. If you want to ban nullptrs in Lockers, I'd like to see an alternative that retains the crispness of the code.
>
> You can have a blah_locked() version in these cases.  Having code called in multiple situations (some paths locked and some not) might be not great.  Some higher level of code should have done the locking or not.
> That said, the sweeper has multiple of these with the Compile_lock so this can't be attempted to be squashed until the sweeper is gone.

I've gotten more push-back on things called blah_locked(). The motivation was that it's not clear if it is the caller or callee who should take the lock, so that name convention isn't really helping the reader. I'm a but sympathetic to that point-of-view.

I still don't think this is something we should be squashing. Maybe it would help sway me if someone tried to convert ZActivatedArray and show what the code would look like if we couldn't use this pattern:
https://github.com/openjdk/zgc/blob/zgc_generational/src/hotspot/share/gc/z/zArray.inline.hpp

Alternatively, we could have two versions of lockers to appease both sides? One that never accepts nullptr, which would be the locker that most should be using, and one for those cases where we do need to run with and without the lock held.

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

PR: https://git.openjdk.org/jdk/pull/9838