RFR: 8253779: Amalloc may be wasting space by overaligning

[Kim Barrett]

On Thu, 8 Jul 2021 19:56:33 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.

Changes requested by kbarrett (Reviewer).

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

> 136: 
> 137:   // Fast allocate in the arena.  Common case aligns to the size of long which is 64 bits
> 138:   // on both 32 and 64 bit platforms. Required for atomic long operations on 32 bits.

s/long/jlong/ because C++ long is not 64 bits on some platforms (Windows!).

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

> 138:   // on both 32 and 64 bit platforms. Required for atomic long operations on 32 bits.
> 139:   void* Amalloc(size_t x, AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM) {
> 140:     assert(is_power_of_2(ARENA_AMALLOC_ALIGNMENT) , "should be a power of 2");

[pre-existing] I think this could now be a static_assert.

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

> 153: 
> 154:   // Allocate in the arena, assuming the size has been aligned to size of pointer, which
> 155:   // is 4 bytes on 32 bits, hence the name.

`Amalloc_4` is a horrible name for this, because this is allocating pointer-aligned and sized values, and even enforces that the size is appropriately pointer aligned.  Maybe better would be something like `AmallocP`?  Though it can also be used to allocate `size_t` and the like that are non-pointers.

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

> 155:   // is 4 bytes on 32 bits, hence the name.
> 156:   void *Amalloc_4(size_t x, AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM) {
> 157:     assert((x & (sizeof(char*)-1)) == 0, "misaligned size");

This ought to be using `is_aligned`.

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

> 157:     assert((x & (sizeof(char*)-1)) == 0, "misaligned size");
> 158:     debug_only(if (UseMallocOnly) return malloc(x);)
> 159:     if (!check_for_overflow(x, "Arena::Amalloc_4", alloc_failmode))

[pre-existing] Missing brackets for the multiline `if` is contrary to the style guide.  Similarly in Amalloc.

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

> 169:   // Allocate with 'double' alignment. It is 8 bytes on sparc.
> 170:   // In other cases Amalloc_D() should be the same as Amalloc_4().
> 171:   void* Amalloc_D(size_t x, AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM) {

I'm happy to see this go.  That description is about as clear as mud; I have no idea what the intended semantics are for this name.  Oh, I see. Before SPARC removal (JDK-8244224), this had some seriously stale conditional code for 32bit SPARC.  I still don't understand what that was about, but don't really care either.  Yay code deletion.

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

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