[RFR] (M) 8143608: Don't 64-bit align start of InstanceKlass vtable, itable, and nonstatic_oopmap on 32-bit systems

[Chris Plummer]

On 12/6/15 2:15 PM, David Holmes wrote:
> Hi Chris,
>
> On 5/12/2015 8:02 AM, Chris Plummer wrote:
>> Hello,
>>
>> Please review the following:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8143608
>> http://cr.openjdk.java.net/~cjplummer/8143608/webrev.00/webrev.hotspot/
>>
>> A bit of background would help. The InstanceKlass object has a number of
>> variable length fields that are laid out after the declared fields. When
>> an InstanceKlass object is allocated, extra memory is allocated for it
>> to leave room for these fields. The first three of these fields are
>> vtable, itable, and nonstatic_oopmap. They are all arrays of HeapWord
>> sized values, which means void* size, which means they only need 32-bit
>> alignment on 32-bit systems. However, they have always been 64-bit
>> aligned. This webrev removes the forced 64-bit alignment on 32-bit
>> systems, saving footprint.
>
> Theory sounds fine.
>
> Not so sure about the practice though. In instanceKlass.hpp you have 
> removed a lot of alignment adjustments and I can't be sure they are 
> all necessarily correct. In particular:
>
> 934   static int header_size()            { return 
> sizeof(InstanceKlass)/HeapWordSize; }
>
> seems to me this will truncate the header size if 
> sizeof(InstanceKlass) is not already 32-bit aligned. Are we assuming 
> the compiler will always pad things such that this is true?
>
> In general I would have expected all the align_object_offset (which 
> aligns to 64-bit) to be replaced with something that aligns to 
> sizeof(void*). Even if we can assume a minimum of 32-bit alignment it 
> isn't clear to me that that always converts to 64-bit alignment on 
> 64-bit.
>
Hi David,

I had looked into pre-existing alignment assumptions and made sure I 
didn't add to the assumptions. It turns out the old alignment would 
approach would fail if sizeof(InstanceKlass() is not already aligned to 
sizeof(void*). Let's look at how things worked before any of my changes. 
The code used to be:

    static int header_size()            { return 
align_object_offset(sizeof(InstanceKlass)/HeapWordSize); }

Keep in mind that this is asking for 64-bit alignment on both 32-bit and 
64-bit systems. That's what align_object_offset() does, although it's 
not really obvious from the name. So first a 32 bit example. Let's 
assume sizeof(InstanceKlass) is 10 (not 32-bit aligned), so / by 
HeapWordSize and you get 2.

     align_object_offset(2) ==
     align_size_up_(2, HeapWordsPerLong) ==
     align_size_up(2, 2) ==
     (2 + 1) & ~1 = 2

This is the wrong answer. We would want an offset of 10 to align to 
64-bit align to 16, which is 4 words, not 2. Now see the difference if 
sizeof(InstanceKlass) is already 32-bit aligned. Assume 
sizeof(InstanceKlass) is 12:

     align_object_offset(3) ==
     align_size_up_(3, HeapWordsPerLong) ==
     align_size_up(3, 2) ==
     (3+ 1) & ~1 = 4

Now we get the right answer. Now for 64-bit systems, using the 
sizeof(InstanceKlass) == 10:

     align_object_offset(2) ==
     align_size_up_(2, HeapWordsPerLong) ==
     align_size_up(2, 1) ==
     (2 + 0) & ~0 = 2

Wrong answer again. Now for sizeof(InstanceKlass) == 12, so it is 32-bit 
aligned, but not 64-bit:

     align_object_offset(3) ==
     align_size_up_(3, HeapWordsPerLong) ==
     align_size_up(3, 1) ==
     (3+ 0) & ~0 = 3

Again the wrong answer. Now sizeof(InstanceKlass) == 16:

     align_object_offset(4) ==
     align_size_up_(4, HeapWordsPerLong) ==
     align_size_up(4, 1) ==
     (4+ 0) & ~0 = 4

This is what we want. So my conclusion is that we already have a 
requirement that sizeof(InstanceKlass) be 32-bit aligned on 32-bit 
systems and 64-bit aligned on 64-bit systems. Also, it turns out that 
align_object_offset() is a no-op on 64-bit systems. The call is just 
there to 64-bit align on 32-bit systems.

I would appreciate someone double checking my assumptions here.

BTW, since the last InstanceKlass field is a pointer type, we at least 
know that the current alignment assumption being made is true, although 
a bit presumptive.

thanks,

Chris
> Thanks,
> David
>
>> This change affects all 32-bit platforms. It should have no net impact
>> on 64-bit platforms since the fields remain (naturally) 64-bit aligned
>> (unless of course I've introduced a bug). The intent is to not change
>> what is done for 64-bit platforms.
>>
>> BTW, there is a change to AARCH64, which may seem odd at first. It just
>> removes an "if" block where the condition should always have evaluated
>> to false, so it should have no net affect.
>>
>> Tested with JPRT "-testset hotspot". Please let me know if you think
>> there are any additional tests that should be run.
>>
>> thanks,
>>
>> Chris
>>