On Fri, 30 Jun 2023 17:59:15 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Ashutosh Mehra has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review comments
>>
>> Signed-off-by: Ashutosh Mehra <asme...@redhat.com>
>
> src/hotspot/share/runtime/vmStructs.cpp line 972:
>
>> 970: unchecked_nonstatic_field(Array<Klass*>, _data,
>> sizeof(Klass*)) \
>> 971: unchecked_nonstatic_field(Array<ResolvedIndyEntry>, _data,
>> sizeof(ResolvedIndyEntry)) \
>> 972: unchecked_nonstatic_field(Array<Array<u1>*>, _data,
>> sizeof(Array<u1>*)) \
>
> Fix alignment of the _data column.
Fixed.
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Annotations.java
> line 74:
>
>> 72: public U1Array getFieldAnnotations(int fieldIndex) {
>> 73: Address addr = fieldsAnnotations.getValue(getAddress());
>> 74: ArrayOfU1Array annotationsArray =
>> VMObjectFactory.newObject(ArrayOfU1Array.class, addr);
>
> How about caching this result so you don't need to allocate a new object
> every time this API is called. Same thing in `getFieldTypeAnnotations()`.
I think VMObjectFactory is a better place to implement the caching behavior so
that all such patterns can benefit from it. I think it is better addressed in
another task.
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java
> line 451:
>
>> 449: offset += 1;
>> 450: }
>> 451: Address addr = getAddress().getAddressAt((getSize() - offset) *
>> VM.getVM().getAddressSize());
>
> A comment on the address computation would be useful here and in the changes
> below.
Added a comment about the layout of the annotation pointers.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1260185912
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1260194851
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1260193404
