[Adding serviceability-dev@openjdk.java.net]
Hi Mikael,
Thanks for pointing this out. I'll look into it some more. Are there any
tests that should be failing as a result of this? I get the feeling no,
since I see other issues here that existed before my change. For
example, this code is not returning the proper size if the class is
anonymous or is an interface. It needs to add 1 extra word in that case.
See size() in instanceKlass.hpp.
Another difference from the VM code is alignObjectSize() is being used
by getSize(), but headerSize is set using alignObjectOffset(). The VM
code uses align_object_offset in both cases.
// Align the object size.
public static long alignObjectSize(long size) {
return VM.getVM().alignUp(size,
VM.getVM().getMinObjAlignmentInBytes());
}
// All vm's align longs, so pad out certain offsets.
public static long alignObjectOffset(long offset) {
return VM.getVM().alignUp(offset, VM.getVM().getBytesPerLong());
}
So the difference here is in the use of getMinObjAlignmentInBytes (not
what the VM does) vs getBytesPerLong (what the VM uses):
public int getObjectAlignmentInBytes() {
if (objectAlignmentInBytes == 0) {
Flag flag = getCommandLineFlag("ObjectAlignmentInBytes");
objectAlignmentInBytes = (flag == null) ? 8 : (int)flag.getIntx();
}
return objectAlignmentInBytes;
}
So this seems wrong for use in any InstanceKlass size or embedded field
offset calculation. It is probably a remnant of when class metadata was
stored in the java heap, and the size of InstanceKlass had to be padded
out to the minimum heap object alignment. At least it is harmless if
ObjectAlignmentInBytes is not set, and if set it is only supported for
64-bit:
lp64_product(intx, ObjectAlignmentInBytes,
8, \
"Default object alignment in bytes, 8 is
minimum") \
range(8,
256) \
constraint(ObjectAlignmentInBytesConstraintFunc,AtParse) \
I'll get these cleaned up, but it sure would be nice if there was a way
to reliably test it.
thanks,
Chris
On 12/8/15 1:54 AM, Mikael Gerdin wrote:
Hi Chris,
Not a review but I'm fairly sure that you need to update the
serviceability agent to reflect these changes, see for example:
public long getSize() {
return Oop.alignObjectSize(
getHeaderSize() +
Oop.alignObjectOffset(getVtableLen()) +
Oop.alignObjectOffset(getItableLen()) +
Oop.alignObjectOffset(getNonstaticOopMapSize()));
}
in InstanceKlass.java
/Mikael
On 2015-12-04 23:02, 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.
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
