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

[Stefan Karlsson]

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
>