RFR: 8233061: ZGC: Enforce memory ordering in segmented bit maps

[erik.osterlund at oracle.com]

Hi Stefan,

Thank you for the review. Let's revisit later if we want to polish 
passing in default memory orderings in our code or not.

Thanks,
/Erik

On 11/12/19 3:21 PM, Stefan Karlsson wrote:
> Hi Erik,
>
> This looks good and should be pushed.
>
> Some comments for further cleanups in this area.
>
> 1) I think it would be nice to be explicit about the memory order used 
> in is_segment_live, instead of relying on the default value:
>
>   inline bool ZLiveMap::is_segment_live(BitMap::idx_t segment) const {
>     return segment_live_bits().par_at(segment);
>   }
>
>   inline bool ZLiveMap::set_segment_live_atomic(BitMap::idx_t segment) {
>     return segment_live_bits().par_set_bit(segment, 
> memory_order_release);
>   }
>
> This way it's more apparent that the writer intentionally chose 
> memory_order_acquire and didn't forget to specify the memory order to 
> use.
>
> 2) I think would should be more explicit and use _bitmap.par_at(index, 
> memory_order_relaxed) here:
>
> inline bool ZLiveMap::get(size_t index) const {
>   BitMap::idx_t segment = index_to_segment(index);
>   return is_marked() &&              // Page is marked
>          is_segment_live(segment) && // Segment is marked
>          _bitmap.at(index);          // Object is marked
> }
>
> Thanks,
> StefanK
>
> On 2019-11-08 15:51, erik.osterlund at oracle.com wrote:
>> Hi Per,
>>
>> Thanks for the review.
>>
>> /Erik
>>
>> On 11/8/19 2:38 PM, Per Liden wrote:
>>>
>>> On 10/28/19 4:53 PM, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> In ZGC, bitmaps are lazily cleared in a segmented fashion. In this 
>>>> scheme, liveness is determined by looking at a counter, a segment 
>>>> bit map and finally the flat bit map structure. The accesses for 
>>>> the various stages need to be ordered properly. This patch 
>>>> sprinkles some OrderAccess calls to enforce this ordering.
>>>>
>>>> Out of curiosity, I disassembled libjvm.so with and without this 
>>>> patch to see if the reordering has bitten us in practice on x86_64. 
>>>> Fortunately, according to my analysis, it has not; we seem to have 
>>>> been lucky. But there is a lot of machine code, so I could have 
>>>> missed something. However, given that we now have an AArch64 port 
>>>> which is definitely affected by this problem, and compilers really 
>>>> are free to do whatever they want to in the future, it seems in 
>>>> order to enforce this explicitly.
>>>>
>>>> This patch depends on 
>>>> https://bugs.openjdk.java.net/browse/JDK-8233073 which exposes some 
>>>> memory ordering aware getters on BitMap. I did not want to just 
>>>> wrap the existing API in ZGC, so I split that out to a separate RFE.
>>>>
>>>> CR:
>>>> http://cr.openjdk.java.net/~eosterlund/8233061/webrev.00/
>>>
>>> The rebased webrev.01 looks good.
>>>
>>> /Per
>>>
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8233061
>>>>
>>>> Thanks,
>>>> /Erik
>>