Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v4]
On Wed, 12 Jul 2023 23:21:12 GMT, Chris Plummer wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> More review comments >> >> Signed-off-by: Ashutosh Mehra > > Marked as reviewed by cjplummer (Reviewer). @plummercj @tstuefe thank you for reviewing this. - PR Comment: https://git.openjdk.org/jdk/pull/14735#issuecomment-1634289240
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v4]
On Wed, 12 Jul 2023 18:48:42 GMT, Ashutosh Mehra wrote: >> Please review this PR that enables ClassWriter to write annotations to the >> class file being dumped. >> >> The fields annotations are stored in `Annotations::_fields_annotations` >> which is of type `Array*>`. There is no class in SA that can >> represent it. I have added ArrayOfU1Array to correspond to the type >> `Array*>` and it works. I believe there are better approaches but >> that would require a bit more refactoring of the classes representing Array >> types. I will leave that for future work for now. >> >> Testing: `test/hotspot/jtreg/serviceability/sa` and >> `test/jdk/sun/tools/jhsdb` >> Tested it manually by dumping j.l.String class and comparing the annotations >> with the original class using javap. >> The test case mentioned in >> [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide >> better coverage. > > Ashutosh Mehra has updated the pull request incrementally with one additional > commit since the last revision: > > More review comments > > Signed-off-by: Ashutosh Mehra Looks good, thanks! - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14735#pullrequestreview-1528442782
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v4]
On Wed, 12 Jul 2023 15:17:10 GMT, Thomas Stuefe wrote: >>> What would be needed to make the Annotations appear in the "printall" >>> command? I was somehow expecting to see at least something like >>> "Annotation@". >> >> I am not sure what all details `printall` is expected to emit out. Looking >> at the code, printall doesn't seem to use ClassWriter. It uses HTMLGenerator >> to format the method data. I can emit something like "Annotation@" but >> it would be more useful if it can display the contents of the annotations. >> Unfortunately HTMLGenerator doesn't understand Annotations at all. Probably >> it is better left for another task. > >> > What would be needed to make the Annotations appear in the "printall" >> > command? I was somehow expecting to see at least something like >> > "Annotation@". >> >> I am not sure what all details `printall` is expected to emit out. Looking >> at the code, printall doesn't seem to use ClassWriter. It uses HTMLGenerator >> to format the method data. I can emit something like "Annotation@" but >> it would be more useful if it can display the contents of the annotations. >> Unfortunately HTMLGenerator doesn't understand Annotations at all. Probably >> it is better left for another task. > > No problem at all. I only thought about this because it would have making a > regression test very easy (there is a jtreg test that calls printall and then > parses the output). @tstuefe can I get your approval as well if there are no other concerns to address. - PR Comment: https://git.openjdk.org/jdk/pull/14735#issuecomment-1634227457
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v4]
On Wed, 12 Jul 2023 18:48:42 GMT, Ashutosh Mehra wrote: >> Please review this PR that enables ClassWriter to write annotations to the >> class file being dumped. >> >> The fields annotations are stored in `Annotations::_fields_annotations` >> which is of type `Array*>`. There is no class in SA that can >> represent it. I have added ArrayOfU1Array to correspond to the type >> `Array*>` and it works. I believe there are better approaches but >> that would require a bit more refactoring of the classes representing Array >> types. I will leave that for future work for now. >> >> Testing: `test/hotspot/jtreg/serviceability/sa` and >> `test/jdk/sun/tools/jhsdb` >> Tested it manually by dumping j.l.String class and comparing the annotations >> with the original class using javap. >> The test case mentioned in >> [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide >> better coverage. > > Ashutosh Mehra has updated the pull request incrementally with one additional > commit since the last revision: > > More review comments > > Signed-off-by: Ashutosh Mehra Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14735#pullrequestreview-1527362780
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v3]
On Tue, 11 Jul 2023 20:33:59 GMT, Chris Plummer wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Some code motion and factoring out common code >> >> Signed-off-by: Ashutosh Mehra > > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java > line 641: > >> 639: //| | >> 640: //V V >> 641: //| ... | default | type | parameter | method | > > So the `...` part is represented by `getSize()`? It would be good to call > that out. Also point out that each of the annotations pointers is optional. Updated the comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1261592735
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v4]
On Tue, 11 Jul 2023 20:28:29 GMT, Chris Plummer wrote: >> 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. > > I think maybe you misunderstood what I meant by "cache". I'm not talking > about a hashmap of weak references or anything like that. Just add a > `ArrayOfU1Array annotationsArray` field to the Annotations object and store > the result there. Got it. Updated the code as suggested. - PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1261593029
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v4]
> Please review this PR that enables ClassWriter to write annotations to the > class file being dumped. > > The fields annotations are stored in `Annotations::_fields_annotations` which > is of type `Array*>`. There is no class in SA that can represent > it. I have added ArrayOfU1Array to correspond to the type `Array*>` > and it works. I believe there are better approaches but that would require a > bit more refactoring of the classes representing Array types. I will leave > that for future work for now. > > Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb` > Tested it manually by dumping j.l.String class and comparing the annotations > with the original class using javap. > The test case mentioned in > [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide > better coverage. Ashutosh Mehra has updated the pull request incrementally with one additional commit since the last revision: More review comments Signed-off-by: Ashutosh Mehra - Changes: - all: https://git.openjdk.org/jdk/pull/14735/files - new: https://git.openjdk.org/jdk/pull/14735/files/1d79e734..f21b53ab Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14735&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14735&range=02-03 Stats: 20 lines in 2 files changed: 10 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/14735.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14735/head:pull/14735 PR: https://git.openjdk.org/jdk/pull/14735
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v3]
On Tue, 11 Jul 2023 19:13:01 GMT, Ashutosh Mehra wrote: > > What would be needed to make the Annotations appear in the "printall" > > command? I was somehow expecting to see at least something like > > "Annotation@". > > I am not sure what all details `printall` is expected to emit out. Looking at > the code, printall doesn't seem to use ClassWriter. It uses HTMLGenerator to > format the method data. I can emit something like "Annotation@" but it > would be more useful if it can display the contents of the annotations. > Unfortunately HTMLGenerator doesn't understand Annotations at all. Probably > it is better left for another task. No problem at all. I only thought about this because it would have making a regression test very easy (there is a jtreg test that calls printall and then parses the output). - PR Comment: https://git.openjdk.org/jdk/pull/14735#issuecomment-1632735373
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v3]
On Tue, 11 Jul 2023 19:56:25 GMT, Ashutosh Mehra wrote: >> Please review this PR that enables ClassWriter to write annotations to the >> class file being dumped. >> >> The fields annotations are stored in `Annotations::_fields_annotations` >> which is of type `Array*>`. There is no class in SA that can >> represent it. I have added ArrayOfU1Array to correspond to the type >> `Array*>` and it works. I believe there are better approaches but >> that would require a bit more refactoring of the classes representing Array >> types. I will leave that for future work for now. >> >> Testing: `test/hotspot/jtreg/serviceability/sa` and >> `test/jdk/sun/tools/jhsdb` >> Tested it manually by dumping j.l.String class and comparing the annotations >> with the original class using javap. >> The test case mentioned in >> [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide >> better coverage. > > Ashutosh Mehra has updated the pull request incrementally with one additional > commit since the last revision: > > Some code motion and factoring out common code > > Signed-off-by: Ashutosh Mehra src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java line 641: > 639: //| | > 640: //V V > 641: //| ... | default | type | parameter | method | So the `...` part is represented by `getSize()`? It would be good to call that out. Also point out that each of the annotations pointers is optional. - PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1260251805
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v3]
On Tue, 11 Jul 2023 19:34:29 GMT, Ashutosh Mehra wrote:
>> 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.
I think maybe you misunderstood what I meant by "cache". I'm not talking about
a hashmap of weak references or anything like that. Just add a `ArrayOfU1Array
annotationsArray` field to the Annotations object and store the result there.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1260246699
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v3]
On Tue, 11 Jul 2023 06:32:24 GMT, Thomas Stuefe wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Some code motion and factoring out common code >> >> Signed-off-by: Ashutosh Mehra > > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java > line 494: > >> 492: offset += 1; >> 493: } >> 494: Address addr = getAddress().getAddressAt((getSize() - offset) * >> VM.getVM().getAddressSize()); > > Factor this address calculation out, and as @plummercj remarked, comment it? Done. > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java > line 874: > >> 872: return null; >> 873: } >> 874: } > > Would calling these functions even be valid to call if Annotations are not > present? > > If yes, maybe return Optional? But since the rest of the code does not use > Optional either, maybe rather keep the code the same. > > Up to you, feel free to ignore this. I would keep it as is following existing code pattern. - PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1260208546 PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1260209779
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v3]
> Please review this PR that enables ClassWriter to write annotations to the > class file being dumped. > > The fields annotations are stored in `Annotations::_fields_annotations` which > is of type `Array*>`. There is no class in SA that can represent > it. I have added ArrayOfU1Array to correspond to the type `Array*>` > and it works. I believe there are better approaches but that would require a > bit more refactoring of the classes representing Array types. I will leave > that for future work for now. > > Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb` > Tested it manually by dumping j.l.String class and comparing the annotations > with the original class using javap. > The test case mentioned in > [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide > better coverage. Ashutosh Mehra has updated the pull request incrementally with one additional commit since the last revision: Some code motion and factoring out common code Signed-off-by: Ashutosh Mehra - Changes: - all: https://git.openjdk.org/jdk/pull/14735/files - new: https://git.openjdk.org/jdk/pull/14735/files/889537fd..1d79e734 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14735&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14735&range=01-02 Stats: 123 lines in 1 file changed: 63 ins; 56 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14735.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14735/head:pull/14735 PR: https://git.openjdk.org/jdk/pull/14735
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v2]
On Tue, 11 Jul 2023 06:39:09 GMT, Thomas Stuefe wrote:
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java
>> line 470:
>>
>>> 468: if (hasParameterAnnotations()) {
>>> 469: offset += 1;
>>> 470: }
>>
>> Code here and in other places could be tightened:
>>
>>
>> int offset = (hasMethodAnnotations() ? 1 : 0) +
>> (hasParameterAnnotations() ? 1 : 0) +
>> (hasTypeAnnotations() ? 1 : 0);
>
> Possibly even factor it out into separate functions like e.g.
> `offsetOfGenericSignatureIndex` does.
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1260193515
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v2]
On Fri, 30 Jun 2023 17:59:15 GMT, Chris Plummer wrote:
>> Ashutosh Mehra has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review comments
>>
>> Signed-off-by: Ashutosh Mehra
>
> src/hotspot/share/runtime/vmStructs.cpp line 972:
>
>> 970: unchecked_nonstatic_field(Array,_data,
>> sizeof(Klass*))\
>> 971: unchecked_nonstatic_field(Array, _data,
>> sizeof(ResolvedIndyEntry)) \
>> 972: unchecked_nonstatic_field(Array*>, _data,
>> sizeof(Array*))\
>
> 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
Re: RFR: 8311102: Write annotations in the classfile dumped by SA [v2]
> Please review this PR that enables ClassWriter to write annotations to the > class file being dumped. > > The fields annotations are stored in `Annotations::_fields_annotations` which > is of type `Array*>`. There is no class in SA that can represent > it. I have added ArrayOfU1Array to correspond to the type `Array*>` > and it works. I believe there are better approaches but that would require a > bit more refactoring of the classes representing Array types. I will leave > that for future work for now. > > Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb` > Tested it manually by dumping j.l.String class and comparing the annotations > with the original class using javap. > The test case mentioned in > [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide > better coverage. Ashutosh Mehra has updated the pull request incrementally with one additional commit since the last revision: Review comments Signed-off-by: Ashutosh Mehra - Changes: - all: https://git.openjdk.org/jdk/pull/14735/files - new: https://git.openjdk.org/jdk/pull/14735/files/66f2c104..889537fd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14735&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14735&range=00-01 Stats: 58 lines in 2 files changed: 32 ins; 21 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/14735.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14735/head:pull/14735 PR: https://git.openjdk.org/jdk/pull/14735
Re: RFR: 8311102: Write annotations in the classfile dumped by SA
On Tue, 11 Jul 2023 07:41:01 GMT, Thomas Stuefe wrote: > What would be needed to make the Annotations appear in the "printall" > command? I was somehow expecting to see at least something like > "Annotation@". I am not sure what all details `printall` is expected to emit out. Looking at the code, printall doesn't seem to use ClassWriter. It uses HTMLGenerator to format the method data. I can emit something like "Annotation@" but it would be more useful if it can display the contents of the annotations. Unfortunately HTMLGenerator doesn't understand Annotations at all. Probably it is better left for another task. - PR Comment: https://git.openjdk.org/jdk/pull/14735#issuecomment-1631375871
Re: RFR: 8311102: Write annotations in the classfile dumped by SA
On Fri, 30 Jun 2023 14:32:03 GMT, Ashutosh Mehra wrote:
> Please review this PR that enables ClassWriter to write annotations to the
> class file being dumped.
>
> The fields annotations are stored in `Annotations::_fields_annotations` which
> is of type `Array*>`. There is no class in SA that can represent
> it. I have added ArrayOfU1Array to correspond to the type `Array*>`
> and it works. I believe there are better approaches but that would require a
> bit more refactoring of the classes representing Array types. I will leave
> that for future work for now.
>
> Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb`
> Tested it manually by dumping j.l.String class and comparing the annotations
> with the original class using javap.
> The test case mentioned in
> [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide
> better coverage.
Hi @ashu-mehra,
looks mostly good. Some nits inline.
I tried it out and it works fine.
What would be needed to make the Annotations appear in the "printall" command?
I was somehow expecting to see at least something like "Annotation@".
Cheers, Thomas
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java line
470:
> 468: if (hasParameterAnnotations()) {
> 469: offset += 1;
> 470: }
Code here and in other places could be tightened:
int offset = (hasMethodAnnotations() ? 1 : 0) +
(hasParameterAnnotations() ? 1 : 0) +
(hasTypeAnnotations() ? 1 : 0);
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java line
494:
> 492: offset += 1;
> 493: }
> 494: Address addr = getAddress().getAddressAt((getSize() - offset) *
> VM.getVM().getAddressSize());
Factor this address calculation out, and as @plummercj remarked, comment it?
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
line 874:
> 872: return null;
> 873: }
> 874: }
Would calling these functions even be valid to call if Annotations are not
present?
If yes, maybe return Optional? But since the rest of the code does not use
Optional either, maybe rather keep the code the same.
Up to you, feel free to ignore this.
-
PR Review: https://git.openjdk.org/jdk/pull/14735#pullrequestreview-1523513001
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259248538
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259249446
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259259800
Re: RFR: 8311102: Write annotations in the classfile dumped by SA
On Tue, 11 Jul 2023 06:31:24 GMT, Thomas Stuefe wrote:
>> Please review this PR that enables ClassWriter to write annotations to the
>> class file being dumped.
>>
>> The fields annotations are stored in `Annotations::_fields_annotations`
>> which is of type `Array*>`. There is no class in SA that can
>> represent it. I have added ArrayOfU1Array to correspond to the type
>> `Array*>` and it works. I believe there are better approaches but
>> that would require a bit more refactoring of the classes representing Array
>> types. I will leave that for future work for now.
>>
>> Testing: `test/hotspot/jtreg/serviceability/sa` and
>> `test/jdk/sun/tools/jhsdb`
>> Tested it manually by dumping j.l.String class and comparing the annotations
>> with the original class using javap.
>> The test case mentioned in
>> [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide
>> better coverage.
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstMethod.java
> line 470:
>
>> 468: if (hasParameterAnnotations()) {
>> 469: offset += 1;
>> 470: }
>
> Code here and in other places could be tightened:
>
>
> int offset = (hasMethodAnnotations() ? 1 : 0) +
> (hasParameterAnnotations() ? 1 : 0) +
> (hasTypeAnnotations() ? 1 : 0);
Possibly even factor it out into separate functions like e.g.
`offsetOfGenericSignatureIndex` does.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259254940
Re: RFR: 8311102: Write annotations in the classfile dumped by SA
On Fri, 30 Jun 2023 14:32:03 GMT, Ashutosh Mehra wrote:
> Please review this PR that enables ClassWriter to write annotations to the
> class file being dumped.
>
> The fields annotations are stored in `Annotations::_fields_annotations` which
> is of type `Array*>`. There is no class in SA that can represent
> it. I have added ArrayOfU1Array to correspond to the type `Array*>`
> and it works. I believe there are better approaches but that would require a
> bit more refactoring of the classes representing Array types. I will leave
> that for future work for now.
>
> Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb`
> Tested it manually by dumping j.l.String class and comparing the annotations
> with the original class using javap.
> The test case mentioned in
> [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide
> better coverage.
Overall the changes look good. Just a few minor suggestions.
src/hotspot/share/runtime/vmStructs.cpp line 972:
> 970: unchecked_nonstatic_field(Array,_data,
> sizeof(Klass*))\
> 971: unchecked_nonstatic_field(Array, _data,
> sizeof(ResolvedIndyEntry)) \
> 972: unchecked_nonstatic_field(Array*>, _data,
> sizeof(Array*))\
Fix alignment of the _data column.
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()`.
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.
-
Changes requested by cjplummer (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14735#pullrequestreview-1507639071
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1248137517
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259190786
PR Review Comment: https://git.openjdk.org/jdk/pull/14735#discussion_r1259199965
Re: RFR: 8311102: Write annotations in the classfile dumped by SA
On Fri, 30 Jun 2023 14:32:03 GMT, Ashutosh Mehra wrote: > Please review this PR that enables ClassWriter to write annotations to the > class file being dumped. > > The fields annotations are stored in `Annotations::_fields_annotations` which > is of type `Array*>`. There is no class in SA that can represent > it. I have added ArrayOfU1Array to correspond to the type `Array*>` > and it works. I believe there are better approaches but that would require a > bit more refactoring of the classes representing Array types. I will leave > that for future work for now. > > Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb` > Tested it manually by dumping j.l.String class and comparing the annotations > with the original class using javap. > The test case mentioned in > [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide > better coverage. Looking for reviewers for this. TIA. - PR Comment: https://git.openjdk.org/jdk/pull/14735#issuecomment-1628904428
RFR: 8311102: Write annotations in the classfile dumped by SA
Please review this PR that enables ClassWriter to write annotations to the class file being dumped. The fields annotations are stored in `Annotations::_fields_annotations` which is of type `Array*>`. There is no class in SA that can represent it. I have added ArrayOfU1Array to correspond to the type `Array*>` and it works. I believe there are better approaches but that would require a bit more refactoring of the classes representing Array types. I will leave that for future work for now. Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb` Tested it manually by dumping j.l.String class and comparing the annotations with the original class using javap. The test case mentioned in [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide better coverage. - Commit messages: - 8311102: Write annotations in the classfile dumped by SA Changes: https://git.openjdk.org/jdk/pull/14735/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14735&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311102 Stats: 379 lines in 9 files changed: 376 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14735.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14735/head:pull/14735 PR: https://git.openjdk.org/jdk/pull/14735
