RFR: 8253779: Amalloc may be wasting space by overaligning [v2]

[Thomas Stuefe]

On Fri, 9 Jul 2021 01:25:15 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Thanks to @kimbarrett for noticing this. The alignment was changed to 64 bits for 32 bit platforms, but overalign for 64 bits platforms.  I changed this to BytesPerLong to cover both, since the long case is why it was changed on 32 bits in the first place in JDK-4526490.
>> I also removed Amalloc_D since I don't know what D stands for and it's the same as Amalloc_4.  That's not a great name either. I'm open to suggestions!
>> Tested with tier1-3.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improvements ala Kim

I like this fix and the suggested naming cleanups. LGTM.

Since you are talking about potential improvements: 

I was always a bit unhappy with this arena code. E.g. I disliked how, instead of letting the Arena itself deal with its chunk chain, it exposed the chain internals and let the Marks modify the chunks from outside. This always seemed wrong to me. 

There may be more potential improvements:
- _hwm and _max should be properties of the chunk, not the arena, and then arguably could be smaller typed offsets instead of pointers.
- I am not sure why we need to keep track of both _first and _current chunk in the arenas. I think one pointer would suffice: just holding the last added chunk, which would serve as starting point to traverse the chain. I may miss something here though, maybe traversal order matters somewhere.

I also disliked how an Arena would always create its first chunk right when constructed, instead of delaying chunk allocation to the first allocation. You always pay upfront even if you don't allocate from the Arena.

If one does all of the above, Arena could maybe shrink to just one member (the top chunk pointer), and then it could be embedded as a value object into Thread instead of having to dynamically create and destroy it. Would be slightly simpler and save one pointer dereferencing when accessing the resource area. You'd also save NMT registration of the Arenas themselves.

Cheers, Thomas

(I also had vague ideas of re-using Metaspace arena code for these hotspot arenas, which share many similarities. But I am not sure if or when I find the time to play with that idea.)

src/hotspot/share/memory/arena.hpp line 36:

> 34: 
> 35: // The byte alignment to be used by Arena::Amalloc.
> 36: #define ARENA_AMALLOC_ALIGNMENT BytesPerLong

We don't store types in here which need alignment larger than 64bit? eg long double?

.. I searched and found no cases where they lived inside an arena, so it's probably fine.

src/hotspot/share/memory/arena.hpp line 140:

> 138:   // on both 32 and 64 bit platforms. Required for atomic jlong operations on 32 bits.
> 139:   void* Amalloc(size_t x, AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM) {
> 140:     STATIC_ASSERT(is_power_of_2(ARENA_AMALLOC_ALIGNMENT));

Move this static assert up to the definition of ARENA_AMALLOC_ALIGNMENT?

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

Marked as reviewed by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4732