On Tue, 14 Mar 2023 01:25:01 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Frederic Parain has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fixes includes and style
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Field.java line 75:
>
>> 73: int initialValueIndex;
>> 74: int genericSignatureIndex;
>> 75: int contendedGroup;
>
> It seems that these should all be shorts. All the getter methods are casting
> them to short.
Indexes in the constant pool are unsigned shorts, but Java shorts are signed,
using ints is the simplest way to store those indexes.
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
> line 108:
>
>> 106: CLASS_STATE_INITIALIZATION_ERROR =
>> db.lookupIntConstant("InstanceKlass::initialization_error").intValue();
>> 107: // We need a new fieldsCache each time we attach.
>> 108: fieldsCache = new HashMap<Address, Field[]>();
>
> This should probably be a WeakHashMap. I tried it and it seems to work (or at
> least didn't cause any problems). However, when doing a heap dump I didn't
> notice the table being any smaller on exit when it was made weak, even though
> there were numerous GC's while dumping the heap.
>
> The `<key>` is the Address of the hotspot InstanceKlass instance, and this
> Address is referenced by the SA InstanceKlass mirror. So theoretically when
> the reference to the mirror goes way, then the cache entry can be cleared.
I've changed the map to a WeakHashMap and didn't see any issue during testing.
But I didn't measure footprint.
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
> line 325:
>
>> 323:
>> 324: public int getFieldOffset(int index) {
>> 325: return (int)getField(index).getOffset();
>
> Cast to int is not needed
Other APIs (like MetadaField) are using longs to pass offsets, doing a cast
here is less disruptive than changing all the other APIs.
-------------
PR: https://git.openjdk.org/jdk/pull/12855
