RFR: JDK-8199735: Mark word updates need to use Access API

[Roman Kennke]

Hi Erik,

thanks for reviewing!

All: can I get one more reviewer for this? I shall probably mention that
this falls under the build-time switch for primitive heap accesses, and
therefore doesn't create any hazards for builds without Shenandoah (or
more generally GCs that require barriers on primitive accesses).


Thanks, Roman

> Hi Roman,
> 
> This looks good to me. Thank you for doing this.
> The only thing I am a bit puzzled by is why the oopDesc::set_mark member
> function takes a volatile markOop as parameter. That seems rather weird.
> But that is unrelated to your changes.
> 
> Thanks,
> /Erik
> 
> On 2018-03-25 23:03, Roman Kennke wrote:
>> Hi Erik,
>>
>> Thanks for having a look.
>>
>>> A few comments:
>>>
>>> 1) Please make sure all files referencing these accessors that have been
>>> moved to the oop.inline.hpp file now #include the oop.inline.hpp file. I
>>> found that there are quite a few missing by looking for references to
>>> oopDesc::mark() with rtags.
>>> 2) I think the mark_addr() function should now be called mark_addr_raw()
>>> for consistency.
>>> 3) I wonder if there should be _raw() accessors that the GCs use
>>> instead. It feels like the GC should not go through the Access API to
>>> acquire mark words.
>> You are of course right in all 3 points. Unfortunately, that causes
>> massive ripples through the code base ;-)
>>
>> So here is what I did: I added _raw() variants for stuff that is used by
>> GC. I temporarily renamed all relevant methods, e.g. mark() ->
>> mark_fluff() to make the compiler help me. Then for each occurance of
>> any such call decided whether it should call the usual method, or the
>> raw variant (as a general rule, everything under src/hotspot/share/gc
>> uses the _raw() variants). Also, I added #include "oop.inline.hpp" for
>> every such occurance. Then I renamed all _fluff() methods and calls back
>> to usual. Did this exercise with and without PCH, with and without zero.
>> Checked with grep if I might have missed some.
>>
>> Notes: bytecodeInterpreter.cpp needed some additional fixes to use
>> cas_set_mark() instead of the Atomic::cmpxchg().
>>
>> There are a bunch of methods in oop.hpp that are only used by GC, like
>> is_locked() etc. I haven't changed them to _raw().
>>
>> This passes tier1 tests release+fastdebug, and as mentioned above builds
>> with and without PCH and zero.
>>
>> Differential:
>> http://cr.openjdk.java.net/~rkennke/JDK-8199735/webrev.03.diff/
>> Full:
>> http://cr.openjdk.java.net/~rkennke/JDK-8199735/webrev.03/
>>
>> Better now?
>>
>> Best regards,
>> Roman
>>
>>
>>
>>> Thanks,
>>> /Erik
>>>
>>>
>>> On 2018-03-21 20:21, Roman Kennke wrote:
>>>> Am 19.03.2018 um 12:07 schrieb Roman Kennke:
>>>>> Am 19.03.2018 um 11:40 schrieb Roman Kennke:
>>>>>> Currently, the mark word is accessed directly in oopDesc::mark()
>>>>>> set_mark() and a bunch of other accessors. Those need to use the
>>>>>> Access
>>>>>> API instead because GC might want to employ barriers on those
>>>>>> accesses,
>>>>>> pretty much like every other field in oopDesc.
>>>>>>
>>>>>> Notice that this is *not* about accessing the bits and fields inside
>>>>>> the
>>>>>> markOop, but about accessing the header itself.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8199735/webrev.01/
>>>>>>
>>>>>> Testing: build fastdebug/release with and without PCH, passed tier1
>>>>>> fastdebug/release.
>>>>>>
>>>>>> Can I please get reviews?
>>>>>>
>>>>>> Thanks, Roman
>>>>>>
>>>>> Just when I sent it, I realized that this is dropping the volatile
>>>>> from
>>>>> the mark word access. Fixed here:
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8199735/webrev.02/
>>>>>
>>>> Ping?
>>>>
>>
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180330/f7e6a4f9/signature.asc>