RFR (S) 8075030: JvmtiEnv::GetObjectSize reports incorrect java.lang.Class instance size

[Coleen Phillimore]

On 6/1/16 9:49 AM, Aleksey Shipilev wrote:
> On 06/01/2016 09:41 AM, serguei.spitsyn at oracle.com wrote:
>> I guess, at least, this needs a CCC request.
> Why? This changes behavior within the spec.

I agree, here's a bit of the spec:


      Get Object Size

    jvmtiError
    GetObjectSize(jvmtiEnv* env,
                 jobject object,
                 jlong* size_ptr)

For the object indicated by|object|, return via|size_ptr|the size of the 
object. This size is an implementation-specific approximation of the 
amount of storage consumed by this object. It may include some or all of 
the object's overhead, and thus is useful for comparison within an 
implementation but not between implementations. The estimate may change 
during a single invocation of the JVM.
>
>> I'm uncomfortable to support the change without knowing the history
>> behind the original implementation. Does anyone know the reasons why
>> current implementation was chosen in the first place?
> Stefan Karlsson forwarded me a discussion back from 2011, which says:
>
> --------- 8< -----------------------------------------------------------
>
>> For the permgen removal project I'm reworking how the sizes of =
> metadata are calculated, and stumbled upon this code.
>> For "normal" objects it calculates the size and are done. But, for
>> non-primitive java.lang.Class instances (mirrors), it uses the size of
>> the Klass instead. Does anyone have a reasonable explanation why this
>> is done?
> My guess would be that it's attempting to provide a more abstractly =
> accurate value, though it's still way off.

No, as I said, the statics used to be in the InstanceKlass, but we moved 
them before permgen.  I think this value has been more or less arbitrary 
for a long time.

>
> [...]
>
> So we can return whatever we want but we need to be consistent.  There
> are other places where the klassOop is swapped for the Class in
> jvmtiTagMap.cpp and the size reported so it will need to be consistent
> with whatever GetObjectSize does.  I'd maintain the current size
> reporting behaviour if possible.
>
> --------- 8< -----------------------------------------------------------
>
> But I looked around and saw no discrepancies. The tests are also fine.
>
> This block history predates OpenJDK. Metaspace work left it untouched
> because it had bigger fish to fry. But at this point I believe we don't
> have to drag this debt around anymore.

Agree.
>
>> Are there any tools that depend on the current behavior?
> I don't think so. Specification itself allows for returning a size
> estimate, so strictly speaking relying on the reported size is not safe.
> However, I know at least one tool which acts all surprised when the
> reported instance size is way off, and it looks like Class instances are
> overlapping with subsequent objects.

I think if the tests were run and showed no difference, any tools would 
be okay with the new value we give it, which is closer to useful.

Coleen
> Thanks,
> -Aleksey
>
>> Thanks,
>> Serguei
>>
>> On 5/31/16 00:40, Aleksey Shipilev wrote:
>>> Hi,
>>>
>>> Please review a tiny fix in GetObjectSize:
>>>     https://bugs.openjdk.java.net/browse/JDK-8075030
>>>     http://cr.openjdk.java.net/~shade/8075030/webrev.01/
>>>
>>> I think this is a leftover from Metaspace work.
>>>
>>> For some reason, JvmtiEnv::GetObjectSize has a special case for
>>> java.lang.Class: it returns the Klass* size, not the java.lang.Class
>>> instance size. This is incorrect: Klass* is indeed referenced from
>>> java.lang.Class.metadata field, but the instance size itself does not
>>> depend on metadata size. This confuses Instrumentation.getObjectSize
>>> users.
>>>
>>> Testing: new test; RBT, :hotspot_compiler, :hotspot_gc,
>>> :hotspot_runtime, :hotspot_serviceability, :hotspot_misc,
>>> :jdk_management, :jdk_instrument, :jdk_jmx, :jdk_jdi, :svc_tools.
>>>
>>> Thanks,
>>> -Aleksey
>>>
>