RFR: 8221392: Reduce ConcurrentGCThreads spinning during start up

[Kim Barrett]

> On Mar 25, 2019, at 11:20 AM, Per Liden <per.liden at oracle.com> wrote:
> 
> On 03/25/2019 04:08 PM, Kim Barrett wrote:
>>> On Mar 25, 2019, at 4:36 AM, Per Liden <per.liden at oracle.com> wrote:
>>> 
>>> During startup, ConcurrentGCThreads end up in a loop sleeping for 1ms over and over (typically 30-50 times), until is_init_completed() is true. This is not optimal, and we should instead have them wait until they are signaled. This problem has been observed before, as comments in JDK-8136854 suggests.
>>> 
>>> This patch introduces wait_init_completed(), which will block until set_init_completed() is called. Note that the newly introduced InitCompleted_lock is only used to control the interaction between wait_init_completed() and set_init_completed(). is_init_completed() is still lock-free and was just cleaned up to use proper atomic load_acquire/release_store semantics.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8221392
>>> Webrev: http://cr.openjdk.java.net/~pliden/8221392/webrev.0
>>> 
>>> /Per
>> ------------------------------------------------------------------------------
>> src/hotspot/share/runtime/init.cpp
>>  203   OrderAccess::release_store(&_init_completed, true);
>> I don't think this needs a release_store.  If it did, then the access
>> in the wait_init_completed loop should also be barriered.  But since
>> both of those are under the lock...
> 
> The release_store here is to pair it with the load_acquire in is_init_completed(), which doesn't grab the lock.

Such pairing is sometimes needed, though not always.

> Without this, the store in set_init_completed() will only be atomic with correct ordering for readers who also grab the lock,

That's not true.  The lock context surrounding the store is sufficient
to provide any needed ordering for the predicate.

> like we do in wait_init_completed().

Right, the shared lock context removes any need for OrderAccess.
This is an example where load_acquire and release_store don’t need to be paired.

> Makes sense?

So no, that doesn't make sense to me; I don't know what theory of
usage leads to the release_store, since the locking done by the writer
also makes the pairing unnecessary.  I don't think the release does
anything useful, and I don't think we should be using OrderAccess
operations when they don't serve some purpose.

(FWIW, I think both the load in wait and the store probably ought to
be Atomic::load and Atomic::store (e.g. weak atomic accesses), because
of the atomic access by the predicate (e.g. the type of the variable
ought to be some Atomic<T> that we don't have); but that usage is
pretty rare in HotSpot code.)