RFR: 8334223: Make Arena MEMFLAGs immutable [v2]

[David Holmes]

On Mon, 17 Jun 2024 15:33:34 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Arenas carry NMT flags. 
>> 
>> An arena should never change that flag. But it does: Arenas (as ResourceAreas), used by CompilerThread, are accounted toward mtCompiler. But since the RA is already created in the parent class constructor (as mtThread), we then have to awkwardly change the flag of an already existing RA in the CompilerThread constructor.
>> 
>> As a prerequisite for future NMT work I would like Arena MEMFLAGS to be immutable.
>> 
>> The patch does that:
>> - we hand in MEMFLAGS to the Thread constructor now (defaults to mtThread)
>> - CompilerThread hands in mtCompiler, all other threads rely on the default
>> - on creation, both ResourceArea and HandleArea are now accounted toward the flag handed in
>> - that allows us to make Arena::flags const, and to remove ResourceArea::bias_to which changed the flag in-flight for the arena
>> - it also allows us to make Arena::flags private
>> 
>> Other, unrelated cleanups:
>> - Made Arena::_size_in_bytes and Arena::_tag private
>> - Merged both Arena constructors into one by specifying a default value of `Chunk::init_size` for `init_size` argument. That makes it equivalent to the old `Arena(flag, tag)` constructor
>> - removed `JavaThread::JavaThread(bool)`. That constructor was used when creating threads that are getting attached. There was only a single use for that constructor, and I replaced it with functionally equivalent code.
>> 
>> Tests:
>> 
>> I manually verified that the NMT numbers printed don't change.
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - feedback johan
>  - Merge branch 'master' into arena-constify-memflags
>  - start

Changes requested by dholmes (Reviewer).

src/hotspot/share/runtime/javaThread.hpp line 1138:

> 1136:   bool is_attaching_via_jni() const { return _jni_attach_state == _attaching_via_jni; }
> 1137:   bool has_attached_via_jni() const { return is_attaching_via_jni() || _jni_attach_state == _attached_via_jni; }
> 1138:   inline void set_is_attaching_via_jni();

So the only thing I don't like about this aspect is that this has to be called at construction time, so having it be a stand-alone function call invites misuse. I'm tempted to add a factory method:

JavaThread* JavaThread::new_attaching_thread() {
  JavaThread* jt = new JavaThread();
  jt>set_is_attaching_via_jni();
}

and make `set_is_attaching_via_jni()` private.

What do you think?

src/hotspot/share/runtime/javaThread.inline.hpp line 201:

> 199: inline void JavaThread::set_is_attaching_via_jni() {
> 200:   _jni_attach_state = _attaching_via_jni;
> 201:   OrderAccess::fence();

No need for a fence here as this should be set before the thread has been "published" and is visible to anyone else. In contrast `set_done_attaching_via_jni` is set after the thread has been published.

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

PR Review: https://git.openjdk.org/jdk/pull/19693#pullrequestreview-2125019202
PR Review Comment: https://git.openjdk.org/jdk/pull/19693#discussion_r1644155425
PR Review Comment: https://git.openjdk.org/jdk/pull/19693#discussion_r1644156749