Hi Mikael,
On 12/15/15 1:56 AM, Mikael Gerdin wrote:
Hi Chris,
sorry for the late reply.
On 2015-12-10 22:31, Chris Plummer wrote:
Hi Mikael,
See comments inline below:
On 12/9/15 8:48 AM, Mikael Gerdin wrote:
On 2015-12-08 20:14, Chris Plummer wrote:
[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.
I'd say that it's quite possible to test it!
Create a whitebox.cpp entry point for determining the size of a class.
Ok, but I instead decided to use jcmd with "GC.class_stats KlassBytes"
Sounds good.
Run a java program calling the entry point and printing the
InstanceKlass size as computed by calling InstanceKlass::size() on a
set of pre-determined set of complex and simple classes (vtables,
itables, anonymous, etc.)
For now I just did this from the command line to do some quick checking.
No test written.
Have the java program wait after it's finished printing.
Launch the SA agent and attach it to the process.
Through several layers of magic, execute the incantation:
mgerdin@mgerdin03:~/work/repos/hg/jdk9/hs-rt-work/hotspot$
../build/linux-x86_64-normal-server-slowdebug/images/jdk/bin/java
sun.jvm.hotspot.CLHSDB 6211
Attaching to process 6211, please wait...
hsdb> jseval
"sapkg.utilities.SystemDictionaryHelper.findInstanceKlass('java/lang/Object').getSize();"
472
Ok. I did this to get sizes as SA sees them. They were not just wrong
for existing JDK, but in most cases off by a large margin. I did some
investigating. This is InstanceKlass.getSize()
public long getSize() {
return Oop.alignObjectSize(getHeaderSize() +
Oop.alignObjectOffset(getVtableLen()) +
Oop.alignObjectOffset(getItableLen()) +
Oop.alignObjectOffset(getNonstaticOopMapSize()));
}
The problem is that getHeaderSize() returns the size in bytes, but the
others all return the size in words. They need to be multiplied by the
word size to get the right value since the caller of getSize() expects
the result to be in bytes.
Oh, that seems like a familiar problem :)
So we have multiple problems with SA with respect to the
InstanceKlass.getSize() support:
* Alignment issues introduced by me.
* Some minor other issues like using alignObjectSize when it's not
needed, and not taking into account extra fields for interfaces and
anonymous classes.
* Not multiplying the vtable, itable, and oopMapSize by the number of
bytes in a word.
* No test available.
I'm not too sure how to proceed here w.r.t. my CR. I'm inclined to just
make the SA adjustments needed to take into consideration the alignment
changes I've made to InstanceKlass, and file a CRs for the rest (one for
the existing bugs and one for the lack of any test).
It would be good if you could at least fix the obvious word size
scaling bug,
I've gotten some internal guidance to limit the fixes to just the stuff
that needs adjusting, and then file bugs for the rest.
filing follow ups on the extra alignObjectSize and missing extra fields.
Ok. It turns out the alignObjectSize() is needed. I missed that that
InstanceKlass::size() also does this, and after some internal
discussion, it was agreed that it should be kept to make sure that
InstanceKlass::size() is consistent with the amount of memory actually
allocated for it (after the allocator does padding out to 8-bytes).
However, there are some other issues to be addressed in this aligning of
the size. It should not be using ObjectAlignmentInBytes, but just be
doing 8-byte aligning. I'll file a bug for that, and have it cover both
the InstanceKlass and SA work needed for this specific issue. A second
bug will be filed for the word size scaling issue, not accounting for
the extra word that interface classes and anonymous classes need, and
for the lack of any test.
The main consumer of this is the "jmap -clstats" command which is a
supported external interface for querying the sizes of metadata in
relation to their ClassLoaders.
ok.
thanks,
Chris
/Mikael
Please advise.
thanks,
Chris
You could also create a javascript source file in the test directory
which performs the appropriate calls and do
hsdb> jsload /path/to/file.js
hsdb> jseval "doit()"
where
file.js:
doit = function() {
sd = sapkg.utilities.SystemDictionaryHelper;
do_klass = function(name) {
writeln(sd.findInstanceKlass(name).getSize());
}
do_klass('java/lang/Object');
do_klass('java/lang/Class');
}
The only problem is that the test can't reliably execute on
unconfigured os x setups because of OS level security requirements for
attaching to processes.
After detaching HSDB with the "quit" command you tell the debugged
java process to terminate, for example by printing some string on its
stdin.
Easy, right? :)
/Mikael
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
