RFR: 8241920: G1: Lazily initialize OtherRegionsTable::_coarse_map

[Erik Österlund]

Hi Claes,

Looks good. Not sure why _n_coarse_entries isn't simply a boolean 
instead though. Nobody seems to care
about its number. Only about whether the bit map is initialized or not. 
But that could be a refactoring
for another day.

Thanks,
/Erik

On 2020-04-02 11:58, Claes Redestad wrote:
> Hi,
>
> after some off-list discussion we've simplified the changes
> we think necessary for ensuring proper initialization:
>
> http://cr.openjdk.java.net/~redestad/8241920/open.03/
>
> Testing: tier1-3 (ongoing), gc/stress/TestStressRSetCoarsening.java
> locally
>
> Thanks!
>
> /Claes
>
> On 2020-04-01 20:37, Claes Redestad wrote:
>> Hi,
>>
>> On 2020-04-01 19:06, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On 01.04.20 18:46, Claes Redestad wrote:
>>>> Hi,
>>>>
>>>> initializing OtherRegionsTable::_coarse_map lazily allows for a small
>>>> startup and footprint improvement, with no measurable cost to GC
>>>> collection times.
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~redestad/8241920/open.00/
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8241920
>>>>
>>>> Thanks!
>>>
>>>    at first glance, I think the code misses a potential racy read of 
>>> the bitmap data, i.e. the pointer to the bitmap data already 
>>> published in delete_region_data() after the assignment (which is 
>>> under a mutex) but a concurrent thread could see the still not 
>>> properly initialized bitmap data values as they are being cleared?
>>
>> Hmm, I was mislead by the comment that says they are all protected by
>> the mutex:
>>
>>    // These are protected by "_m".
>>    CHeapBitMap _coarse_map;
>>    size_t      _n_coarse_entries;
>>    static jint _n_coarsenings;
>>
>> Since we need to guarantee any access to _coarse_bitmap happens after
>> it's been properly initialized, we can use _n_coarse_entries as a guard
>> with proper acquire/release semantics:
>>
>> http://cr.openjdk.java.net/~redestad/8241920/open.01/
>>
>> I think a release_store_fence on the store that moves us from 0 to 1 the
>> first time is sufficient, but my understanding of the Atomic/OrderAccess
>> API is very limited.
>>
>> /Claes
>>
>>>
>>> Thanks,
>>>    Thomas
>>>