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

[Kim Barrett]

On Thu, 17 Apr 2025 18:33: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:
> 
>   Move semantics would be nice.

Changes requested by kbarrett (Reviewer).

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

> 34: #else
> 35: #define LAUNDER(X) (X)
> 36: #endif

Does this really need to be a macro?  (Assuming we actually need it, which I'm not yet certain about.
Maybe a different approach will remove it.  And I suspect `-fno-strict-aliasing` obviates it.)

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

> 34: #else
> 35: #define LAUNDER(X) (X)
> 36: #endif

Have you investigated what Visual Studio has?  `std::launder` is supported in VS 2017 15.7, and the
minimum supported by JDK is well past that.  The MS folks have in some cases been adding
`__builtin_XXX` that are compatible with the gcc/clang equivalents, so maybe they did that here too?

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

> 39: // May be initialized exactly once.
> 40: template<typename T>
> 41: class StableValue {

Coming in late with more bike shedding on the name.  DeferredInit, DelayedInit, or something along those
lines?  The idea is to emphasize that the object's construction is deferred/delayed until the call to `initialize()`.

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

> 42:   union {
> 43:     T _t;
> 44:   };

All of this stuff about laundering and this union trick could go away if we're willing to add a pointer
to the StableValue class.  Something like this (not tested):

template<typename T>
class StableValue {
  alignas(T) char _data[sizeof(T)];
  T* _p;
public:
  StableValue() : _p(nullptr) {}
  ~StableValue() = default;
  NONCOPYABLE(StableValue);
  T* ptr() {
    assert(_p != nullptr, "uninitialized");
    return _p;
  }
  T* operator->() { return ptr(); }
  T& operator*() { return *ptr(); }
  template<typename... Args>
  void initialize(Args&&... args) {
    assert(_p == nullptr, "already initialized");
    _p = ::new (_data) T(std::forward<Args>(args)...);
  }
};

Regarding the destructor, quoting from the style guide: "HotSpot doesn't generally try to cleanup
on exit, and running destructors at exit can also lead to problems."

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

> 55: 
> 56:   T* ptr() {
> 57:     return LAUNDER(&this->_t);

Do we actually need a function that potentially returns the uninitialized data?  I'd prefer `ptr` and
`operator->()` were identical, and `initialize` just directly accessed `_t`.  And maybe drop `ptr`.
The one external use of `ptr` that I see in the PR would be better served by adding `operator*()`.
But our code tends to use pointers more than references, so maybe it's worth retaining `ptr`, so
long as it has the is-initialized assertion too.

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

> 66:   void initialize(Ts&... args) {
> 67:     DEBUG_ONLY(_initialized = true);
> 68:     new (ptr()) T(args...);

I think this should use perfect forwarding.  The style guide should also be updated to permit using
perfect forwarding.

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

PR Review: https://git.openjdk.org/jdk/pull/24689#pullrequestreview-2779943132
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2051494837
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2051495512
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2051463259
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2051659153
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2051457971
PR Review Comment: https://git.openjdk.org/jdk/pull/24689#discussion_r2051457210