RFR: 8302354: InstanceKlass init state/thread should be atomic

[David Holmes]

On Mon, 13 Feb 2023 18:56:19 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

> Hi, please consider.
> 
> These states are read 'lock-less' should thus be atomic.
> Also the assert setting init thread state can be stricter.
> 
> Passes t1-4

src/hotspot/share/oops/instanceKlass.hpp line 237:

> 235: 
> 236:   Monitor*             _init_monitor;           // mutual exclusion to _init_state and _init_thread.
> 237:   JavaThread* volatile _init_thread;            // Pointer to current thread doing initialization (to handle recursive initialization)

`volatile` is not strictly needed here - see discussion above - but flags this as a "lock-free mutable field". (Though a special type - again see above.)

src/hotspot/share/oops/instanceKlass.hpp line 237:

> 235: 
> 236:   Monitor*             _init_monitor;           // mutual exclusion to _init_state and _init_thread.
> 237:   JavaThread* volatile _init_thread;            // Pointer to current thread doing initialization (to handle recursive initialization)

Nit: please re-align comments with preceding lines. Thanks

src/hotspot/share/oops/instanceKlass.hpp line 514:

> 512:   bool is_being_initialized() const        { return init_state() == being_initialized; }
> 513:   bool is_in_error_state() const           { return init_state() == initialization_error; }
> 514:   bool is_init_thread(JavaThread *thread)  { return thread == Atomic::load(&_init_thread); }

Just to comment ... `is_init_thread` only ever makes sense to be called by passing the current thread, in which case there is no need for an Atomic::load because a thread's own writes are always visible to itself, and no other thread will ever write a different thread's address into the field. So you could simplify this by doing:

bool is_init_thread(JavaThread *current { 
  assert(current == JavaThread::current(), "invariant");
  return thread == _init_thread;
}

src/hotspot/share/oops/instanceKlass.hpp line 1083:

> 1081:     assert((thread == JavaThread::current() && _init_thread == nullptr) ||
> 1082:            (thread == nullptr && _init_thread == JavaThread::current()), "Only one thread is allowed to own initialization");
> 1083:     Atomic::store(&_init_thread, thread);

As a corollary to the above comment, Atomic::store is not needed here when only the current thread can set the value to itself, or change the value from itself to nullptr.

-------------

PR: https://git.openjdk.org/jdk/pull/12542