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

[serguei.spitsyn at oracle.com]

On 6/1/16 23:59, Staffan Larsen wrote:
>
>> On 1 juni 2016, at 21:43, serguei.spitsyn at oracle.com 
>> <mailto:serguei.spitsyn at oracle.com> wrote:
>>
>>
>> On 6/1/16 10:19, Coleen Phillimore wrote:
>>>
>>>
>>> On 6/1/16 9:49 AM, Aleksey Shipilev wrote:
>>>> On 06/01/2016 09:41 AM, serguei.spitsyn at oracle.com 
>>>> <mailto: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.
>>
>> This looks like a gray zone to me.
>> If this confusing customers then do we want to change the spec to 
>> make it more clear?
>>
>> If there is a motivation or requirements to change the behavior then 
>> it impacts the usage.
>> Also, I'm not convinced yet this must be changed this way.
>>
>> It is good that the suggested behavior is more logical in terms that 
>> the GetObjectSize
>> will return the java heap size of the j.l.Class object.
>> However, it is still confusing as it does not reflect the metadata 
>> memory consumed by the class.
>> The returned size will be the same for any Class object which is not 
>> very useful.
>> Someone may expect that when he changes a class by adding methods or 
>> fields, the returned size is growing too.
>>
>> Of course, this is currently broken as well as this data is located 
>> in the InstanceKlass, not Klass.
>> Did we consider to return the InstanceKlass size instead?
>>
>> My point is that the motivation behind the original behavior, most 
>> likely, was reasonable.
>> So that it would be good to find out what it was.
>> My guess is that the motivation was to make the function useful and 
>> less confusing.
>
> I think the current implementation made sense when the metadata was 
> stored on the heap. Then the heap memory used by the class would 
> include the memory used by the metadata. Now that the metadata has 
> moved off the heap, it makes less sense to include it in 
> GetObjectSize. I think the intention on GetObjectSize is the return 
> the amount of heap data an object uses, but the spec cannot say that 
> since it would be too implementation dependent.

Thanks, Staffan.
It looks like a good motivation for this change.
Even though the spec (listed by Coleen above) does not tell anything 
about the heap memory consumption,
it it is normal and less confusing for the java developers to expect it 
on the heap.

>
> I think the change is fine, but I also think we should add a release 
> note about the change to avoid surprises.

Aleksey, I'm Ok with the change too.

Thanks,
Serguei

>
> /Staffan
>
>
>>
>>
>>>>
>>>>> 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.
>>
>> It does not look useful enough to me.
>> It is hard to estimate the importance of this corner case.
>> Maybe, it is Ok to be useless but more logical.
>> I'm ready to follow any consensus if we have it.
>>
>> The fix itself looks good to me.
>>
>> Thanks,
>> Serguei
>>
>>
>>>
>>> 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/ 
>>>>>> <http://cr.openjdk.java.net/%7Eshade/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
>