RFR: JDK-8270308: Amalloc aligns size but not return value (take 2) [v5]

[Thomas Stuefe]

On Mon, 26 Jul 2021 13:23:16 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> Are Chunk::size, medium_size, init_size and tiny size all 64 bit aligned? I see that they're aligned here again but it seems like those sizes being invariantly aligned to 64 bit would prevent future bugs (if they're not already). Also maybe needing a static_assert ?

They are all aligned now - and you are right, I can add static asserts to assert that. I do not align them here, I just assert that they are aligned (via assert(is_aligned(length)).

The only thing actively I align here is the header size aka sizeof(Chunk). I tried to force the compiler to generate Chunk as 64-bit aligned structure but gave up after fiddling with pragmas for half an hour.

> src/hotspot/share/memory/arena.cpp line 208:
> 
>> 206:        vm_exit_out_of_memory(bytes, OOM_MALLOC_ERROR, "Chunk::new");
>> 207:      }
>> 208:      assert(is_aligned(p, BytesPerLong), "Chunk start address not malloc aligned?");
> 
> Should this be ARENA_AMALLOC_ALIGNMENT, in case we have to change it back to 128 bits for some reason?

Yes, I can do that too. I should try to set it to 128bits and test it. 

With 128 bit we may run into alignment problems with 32-bit platforms, since we rely on ARENA_AMALLOC_ALIGNMENT being <= malloc alignment, and on 32-bit I believe malloc alignment is just 64 bit. Not a showstopper, but may require a bit more care.

> test/hotspot/gtest/memory/test_arena.cpp line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
> 
> Did you want an SAP copyright?

I could add one. Probably should, thanks for noticing.

> test/hotspot/gtest/memory/test_arena.cpp line 30:
> 
>> 28: #ifndef LP64
>> 29: // These tests below are about alignment issues when mixing Amalloc and AmallocWords.
>> 30: // Since on 64-bit these APIs offer the same alignment, they only matter for 32-bit.
> 
> Thank you so much for testing this on 32 bit platforms.

Sure. I run Ubuntu 20.4, with Debian multi-arch it's beautifully easy to have 32-bit and 64-bit compilers side by side.

> test/hotspot/gtest/memory/test_arena.cpp line 39:
> 
>> 37:   void* p2 = ar.Amalloc(BytesPerLong);
>> 38:   ASSERT_TRUE(is_aligned(p1, BytesPerWord));
>> 39:   ASSERT_TRUE(is_aligned(p2, BytesPerLong));
> 
> Should BytesPerLong in this test be ARENA_AMALLOC_ALIGNMENT?

Yes, I can do that.

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

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