RFR: 8354954: Typed static memory for late initialization of static class members in Hotspot [v7]

[Kim Barrett]

On Tue, 22 Apr 2025 11:09:36 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Hi,
>> 
>> This PR introduces a `StableValue<T>` which is sized and aligned identically to a `T`, with the difference that a `StableValue<T>` needs to be explicitly instantiated.
>> 
>> Dynamic static initalization in C++ leads to unpredictable bugs as there is no defined order in which objects will be initialized, to the degree that 'static initialization fiasco' is a term used. In the code I've worked on in Hotspot we resolve this by having an initialization function, and instead of having static members of `T` we have `T*` instead and use `malloc` in order to gain the memory for the objects. This is workable, but is unnecessary.
>> 
>> That's why I'd like to have `StableValue<T>`. It let's you avoid the whole `malloc` thing, and we overload `->` to make it behave as if it is actually a `T`. We add in a simple checker in debug mode that checks whether the memory has been initialized before using it.
>> 
>> In the code I've switched two members to be of `StableValue` instead. One is the malloc case above, the second (MemBaseline) is one where I got a bug while developing. The bug occurred because I  changed the initializer of `MemBaseline` without knowing that it was dynamic-static-allocated, and the exact change I made caused weird crashes (because of initialization order issues).
>> 
>> This solution is quite practical to me, but I wanted to know what others think.
>
> Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Use the pointer, of course

Changes requested by kbarrett (Reviewer).

src/hotspot/share/nmt/memTracker.hpp line 264:

> 262:   // Stored baseline
> 263:   static inline MemBaseline& get_baseline() {
> 264:     return *_baseline.ptr();

I think StableValue should provide `operator*()` and it should be used here.

src/hotspot/share/utilities/stableValue.hpp line 42:

> 40: public:
> 41:   StableValue() {
> 42:     DEBUG_ONLY(_initialized = false);

Prefer ctor-initializer initialization to assignment in the body.

src/hotspot/share/utilities/stableValue.hpp line 45:

> 43:   }
> 44: 
> 45:   ~StableValue() {

Comment the ctor/dtor as intentionally not initializing `_t` and not destroying it either.

src/hotspot/share/utilities/stableValue.hpp line 47:

> 45:   ~StableValue() {
> 46:   }
> 47: 

This class should be noncopyable.

src/hotspot/share/utilities/stableValue.hpp line 50:

> 48:   T* ptr() {
> 49:     assert(_initialized, "must be initialized before access");
> 50:     return &this->_t;

Why `&this->_t` rather than `&_t`?

src/hotspot/share/utilities/stableValue.hpp line 55:

> 53:   T* operator->() {
> 54:     assert(_initialized, "must be initialized before access");
> 55:     return ptr();

Just call `ptr()`, which does the assert.

src/hotspot/share/utilities/stableValue.hpp line 62:

> 60:     DEBUG_ONLY(_initialized = true);
> 61:     // If T has const and volatile, get rid of them and tack on a pointer *.
> 62:     using NCVP = std::add_pointer_t<std::remove_cv_t<T>>;

I think the comment is grammatically confusing as written.  I also think the comment doesn't add any
information, and we'd be better off just removing that comment.

src/hotspot/share/utilities/stableValue.hpp line 64:

> 62:     using NCVP = std::add_pointer_t<std::remove_cv_t<T>>;
> 63:     // Make _t into NCVP temporarily so that we can placement-new it.
> 64:     new (const_cast<NCVP>(ptr())) T(args...);

s/new/::new/ to avoid looking up the allocator in the scope of `T`.  All of the uses in this PR are
for "value classes" rather than (say) `CHeapObj`, so don't provide `operator new` &etc.

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

PR Review: https://git.openjdk.org/jdk/pull/24689#pullrequestreview-2784531545
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2054403881
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2054368859
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2054386699
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2054373731
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2054372300
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2054371389
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2054381942
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2054400656