Re: RFR: 8316342: CLHSDB "dumpclass" command produces invalid classes
On Thu, 28 Sep 2023 23:40:58 GMT, Chris Plummer wrote: >> Please review this change to fix the operands of the bytecodes that operate >> on fields when dumping a class using SA. >> >> Testing: hotspot_serviceability >> >> I have also verified that >> `test/hotspot/jtreg/serviceability/sa/ClhsdbDumpclass.javaClhsdbDumpclass.java`, >> which is in the problem list because of this issue, passes with this change. >> I have also verified it by dumping the class that has getstatic/putstatic >> bytecodes and comparing the bytecodes manually with the original classfile. > > Ok. The changes look good to me, but I don't have much of understanding of > the hotspot code involved here, so I'll defer to a hotspot expert for the 2nd > review. I am returning from a break. Thanks @plummercj @sspitsyn for reviewing the changes. It looks like this doesn't have any merge conflicts, so can still be integrated. - PR Comment: https://git.openjdk.org/jdk/pull/15973#issuecomment-1810366426
Integrated: 8316342: CLHSDB "dumpclass" command produces invalid classes
On Thu, 28 Sep 2023 21:23:25 GMT, Ashutosh Mehra wrote: > Please review this change to fix the operands of the bytecodes that operate > on fields when dumping a class using SA. > > Testing: hotspot_serviceability > > I have also verified that > `test/hotspot/jtreg/serviceability/sa/ClhsdbDumpclass.javaClhsdbDumpclass.java`, > which is in the problem list because of this issue, passes with this change. > I have also verified it by dumping the class that has getstatic/putstatic > bytecodes and comparing the bytecodes manually with the original classfile. This pull request has now been integrated. Changeset: 7bb1999c Author:Ashutosh Mehra URL: https://git.openjdk.org/jdk/commit/7bb1999c51cdfeb020047e1094229fda1ec5a99d Stats: 49 lines in 2 files changed: 7 ins; 37 del; 5 mod 8316342: CLHSDB "dumpclass" command produces invalid classes Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/15973
Re: RFR: 8316342: CLHSDB "dumpclass" command produces invalid classes
On Thu, 28 Sep 2023 22:32:29 GMT, Chris Plummer wrote:
>> Please review this change to fix the operands of the bytecodes that operate
>> on fields when dumping a class using SA.
>>
>> Testing: hotspot_serviceability
>>
>> I have also verified that
>> `test/hotspot/jtreg/serviceability/sa/ClhsdbDumpclass.javaClhsdbDumpclass.java`,
>> which is in the problem list because of this issue, passes with this change.
>> I have also verified it by dumping the class that has getstatic/putstatic
>> bytecodes and comparing the bytecodes manually with the original classfile.
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/jcore/ByteCodeRewriter.java
> line 137:
>
>> 135: case Bytecodes._invokedynamic: {
>> 136: int cpci = method.getNativeIntArg(bci + 1);
>> 137: cpoolIndex = (short)
>> cpCache.getIndyEntryAt(~cpci).getConstantPoolIndex();
>
> Is this really suppose to be `~cpci` and not just `cpci`?
That's right. See
https://github.com/openjdk/jdk/blob/ecb5e8a03f67c92d7956201de1fa7d07cc6af9cb/src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp#L1053
and
https://github.com/openjdk/jdk/blob/ecb5e8a03f67c92d7956201de1fa7d07cc6af9cb/src/hotspot/share/oops/constantPool.hpp#L256
I tend to use `JvmtiClassFileReconstituter` as the reference for the code
involved in dumping the classfile, and that makes things easier to follow and
implement.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15973#discussion_r1340737723
RFR: 8316342: CLHSDB "dumpclass" command produces invalid classes
Please review this change to fix the operands of the bytecodes that operate on fields when dumping a class using SA. Testing: hotspot_serviceability I have also verified that `test/hotspot/jtreg/serviceability/sa/ClhsdbDumpclass.javaClhsdbDumpclass.java`, which is in the problem list because of this issue, passes with this change. I have also verified it by dumping the class that has getstatic/putstatic bytecodes and comparing the bytecodes manually with the original classfile. - Commit messages: - Remove ClhsdbDumpclass.java from ProblemList - 8316342: CLHSDB "dumpclass" command produces invalid classes Changes: https://git.openjdk.org/jdk/pull/15973/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15973=00 Issue: https://bugs.openjdk.org/browse/JDK-8316342 Stats: 49 lines in 2 files changed: 7 ins; 37 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/15973.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15973/head:pull/15973 PR: https://git.openjdk.org/jdk/pull/15973
Integrated: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 20:45:00 GMT, Ashutosh Mehra wrote: > Please review trivial fix to display class name in the output of "where" > command of SA. > > Testing: hotspot_serviceability This pull request has now been integrated. Changeset: 065203d4 Author:Ashutosh Mehra URL: https://git.openjdk.org/jdk/commit/065203d44a651ee850807bb1f2bed59cea7de3ea Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8313631: SA: stack trace printed using "where" command does not show class name Reviewed-by: cjplummer, dholmes - PR: https://git.openjdk.org/jdk/pull/15952
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Thu, 28 Sep 2023 01:57:59 GMT, David Holmes wrote: >> Please review trivial fix to display class name in the output of "where" >> command of SA. >> >> Testing: hotspot_serviceability > > Has the change been tested in the HSDB GUI as requested in the JBS issue? @dholmes-ora thanks for reviewing. - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1739179894
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 20:45:00 GMT, Ashutosh Mehra wrote: > Please review trivial fix to display class name in the output of "where" > command of SA. > > Testing: hotspot_serviceability I believe my github user name has been linked to the OpenJDK user name. So I am going to try integrate it. - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1739184770
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Thu, 28 Sep 2023 01:57:59 GMT, David Holmes wrote: >> Please review trivial fix to display class name in the output of "where" >> command of SA. >> >> Testing: hotspot_serviceability > > Has the change been tested in the HSDB GUI as requested in the JBS issue? @dholmes-ora thanks for pointing out the issue. I have created one - https://bugs.openjdk.org/browse/SKARA-2046 > Has the change been tested in the HSDB GUI as requested in the JBS issue? Yes, I have checked hsdb as well. It shows same contents as the `where` command. - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738335000
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 21:07:18 GMT, Chris Plummer wrote: >> Please review trivial fix to display class name in the output of "where" >> command of SA. >> >> Testing: hotspot_serviceability > > Can you add to the CR a copy of the `where` output after this fix? Just a > short snippet equivalent to the broken output already in the CR. thanks. @plummercj for some reason the bot refuses to recognize my committer status in jdk. As per https://openjdk.org/census#jdk I see my name in the Committers list. Any idea what needs to be done here? - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738219081
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 21:07:18 GMT, Chris Plummer wrote: >> Please review trivial fix to display class name in the output of "where" >> command of SA. >> >> Testing: hotspot_serviceability > > Can you add to the CR a copy of the `where` output after this fix? Just a > short snippet equivalent to the broken output already in the CR. thanks. thanks @plummercj for reviewing it. - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738215616
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 20:45:00 GMT, Ashutosh Mehra wrote: > Please review trivial fix to display class name in the output of "where" > command of SA. > > Testing: hotspot_serviceability Updated CR with the output of `where` command after the fix. - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738087405
RFR: 8313631: SA: stack trace printed using "where" command does not show class name
Please review trivial fix to display class name in the output of "where" command of SA. Testing: hotspot_serviceability - Commit messages: - 8313631: SA: stack trace printed using "where" command does not show class name Changes: https://git.openjdk.org/jdk/pull/15952/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15952=00 Issue: https://bugs.openjdk.org/browse/JDK-8313631 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15952.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15952/head:pull/15952 PR: https://git.openjdk.org/jdk/pull/15952
Re: RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Mon, 24 Jul 2023 22:12:28 GMT, Ashutosh Mehra wrote: > This patch adds NestHost and NestMembers attributes to the class dumped by SA. > > Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb` > Manual testing by dumping `j.l.String` and > `j.l.String$CaseInsensitiveComparator` classes. > `j.l.String` shows one entry in `NestMembers` attribute for > `j.l.String$CaseInsensitiveComparator` and > `j.l.String$CaseInsensitiveComparator` has `j.l.String` as its `NestHost`. Thanks every one for reviewing this PR. - PR Comment: https://git.openjdk.org/jdk/pull/15005#issuecomment-1666039907
Integrated: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Mon, 24 Jul 2023 22:12:28 GMT, Ashutosh Mehra wrote: > This patch adds NestHost and NestMembers attributes to the class dumped by SA. > > Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb` > Manual testing by dumping `j.l.String` and > `j.l.String$CaseInsensitiveComparator` classes. > `j.l.String` shows one entry in `NestMembers` attribute for > `j.l.String$CaseInsensitiveComparator` and > `j.l.String$CaseInsensitiveComparator` has `j.l.String` as its `NestHost`. This pull request has now been integrated. Changeset: 873d1179 Author:Ashutosh Mehra Committer: Chris Plummer URL: https://git.openjdk.org/jdk/commit/873d11793211717c37c6c72c80a76d1472c64c8a Stats: 50 lines in 3 files changed: 50 ins; 0 del; 0 mod 8312623: SA add NestHost and NestMembers attributes when dumping class Reviewed-by: cjplummer, sspitsyn, stuefe - PR: https://git.openjdk.org/jdk/pull/15005
Re: RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Thu, 3 Aug 2023 15:59:14 GMT, Chris Plummer wrote: >> @ashu-mehra thanks for doing the additional testing. Pity there is no >> regression/functional test for this. > >> @dholmes-ora @plummercj I have improved [dumpclass >> tests](https://github.com/openjdk/jdk/commit/97618cc9bbadce4b51fc0fbee93557f4dcc8d26a) >> to cover up some cases for this PR and >> [JDK-8311971](https://bugs.openjdk.org/browse/JDK-8311971). If the tests >> makes sense I would like to include it in this PR. Is that okay? > > I think given the scope of the changes it would be better to do this with a > "Improve SA dumpclass testing" CR/PR. @plummercj can you please sponsor it. - PR Comment: https://git.openjdk.org/jdk/pull/15005#issuecomment-1666023204
Re: RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Thu, 3 Aug 2023 15:59:14 GMT, Chris Plummer wrote: >> @ashu-mehra thanks for doing the additional testing. Pity there is no >> regression/functional test for this. > >> @dholmes-ora @plummercj I have improved [dumpclass >> tests](https://github.com/openjdk/jdk/commit/97618cc9bbadce4b51fc0fbee93557f4dcc8d26a) >> to cover up some cases for this PR and >> [JDK-8311971](https://bugs.openjdk.org/browse/JDK-8311971). If the tests >> makes sense I would like to include it in this PR. Is that okay? > > I think given the scope of the changes it would be better to do this with a > "Improve SA dumpclass testing" CR/PR. @plummercj > I think given the scope of the changes it would be better to do this with a > "Improve SA dumpclass testing" CR/PR. Okay. I guess this can then be integrated as is. Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/15005#issuecomment-1664254792
Re: RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Wed, 2 Aug 2023 05:07:31 GMT, David Holmes wrote: > Pity there is no regression/functional test for this. @dholmes-ora @plummercj I have improved [dumpclass tests](https://github.com/openjdk/jdk/commit/97618cc9bbadce4b51fc0fbee93557f4dcc8d26a) to cover up some cases for this PR and [JDK-8311971](https://bugs.openjdk.org/browse/JDK-8311971). If the tests makes sense I would like to include it in this PR. Is that okay? - PR Comment: https://git.openjdk.org/jdk/pull/15005#issuecomment-1664011833
Re: RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Tue, 1 Aug 2023 08:33:24 GMT, David Holmes wrote:
>> @dholmes-ora I confirmed there is no nest-host or nest-members attributes
>> generated by this patch for a top level class which doesn't have any
>> nest-members. Is that what you wanted to verify?
>
> @ashu-mehra That was one case. I also want to know that you have tested
> deeply nested classes; and hidden classes (if applicable). Thanks.
@dholmes-ora I verified the case for hidden dynamically injected classes. The
dumped class data for a hidden dynamically injected class does not have any
Nest-Host attribute. When generating these classes dynamically the VM does not
expose nest-host information in the class data, but sets the nest-host directly
in the InstanceKlass.
Also verified the case for deeply nested classes by creating chain of nested
classes as:
class DeepNest {
class NestLvl1 {
class NestLvl2 {
class NestLvl3 {
}
}
}
}
Only `DeepNest` has the `NestMembers` attribute which lists all the
NestLvl[1-3] classes. Rest all have `DeepNest` as the `NestHost` attribute.
Does this cover all the cases you flagged?
-
PR Comment: https://git.openjdk.org/jdk/pull/15005#issuecomment-1661077708
Re: RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Thu, 27 Jul 2023 22:39:03 GMT, David Holmes wrote: >> @dholmes-ora sorry for responding late. I got sidetracked by some other work. >> >>> We need to be sure this works as expected for top-level classes that have >>> no nest members, and deeply nested nest members, plus dynamically injected >>> hidden classes that are nest members. >> >> I am not sure I understand this concern. We are getting nest-host and >> nest-members from the InstanceKlass. As long as this information is recorded >> in InstanceKlass, it would work. Can you please elaborate your concern about >> the cases you feel may not work. >> >>> I'm unclear if this is intended to only expose the same details as would be >>> statically defined in the attribute in the classfile? >> >> It is to expose the details as the JVM sees, which may be different from >> what is statically defined in the classfile if agents are involved. > > @ashu-mehra you indicated that you had only done two basic manual tests to > check the output. You need to check it for the cases that I flagged too. In > the VM every top-level class is its own nest-host, but that is not expressed > in a classfile attribute (it is just the defined semantics) so displaying > this as-if it were an explicit attribute may not be right. @dholmes-ora I confirmed there is no nest-host or nest-members attributes generated by this patch for a top level class which doesn't have any nest-members. Is that what you wanted to verify? - PR Comment: https://git.openjdk.org/jdk/pull/15005#issuecomment-1658599841
Re: RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Tue, 25 Jul 2023 05:36:15 GMT, David Holmes wrote: >> This patch adds NestHost and NestMembers attributes to the class dumped by >> SA. >> >> Testing: `test/hotspot/jtreg/serviceability/sa` and >> `test/jdk/sun/tools/jhsdb` >> Manual testing by dumping `j.l.String` and >> `j.l.String$CaseInsensitiveComparator` classes. >> `j.l.String` shows one entry in `NestMembers` attribute for >> `j.l.String$CaseInsensitiveComparator` and >> `j.l.String$CaseInsensitiveComparator` has `j.l.String` as its `NestHost`. > > We need to be sure this works as expected for top-level classes that have no > nest members, and deeply nested nest members, plus dynamically injected > hidden classes that are nest members. I'm unclear if this is intended to only > expose the same details as would be statically defined in the attribute in > the classfile? @dholmes-ora sorry for responding late. I got sidetracked by some other work. > We need to be sure this works as expected for top-level classes that have no > nest members, and deeply nested nest members, plus dynamically injected > hidden classes that are nest members. I am not sure I understand this concern. We are getting nest-host and nest-members from the InstanceKlass. As long as this information is recorded in InstanceKlass, it would work. Can you please elaborate your concern about the cases you feel may not work. > I'm unclear if this is intended to only expose the same details as would be > statically defined in the attribute in the classfile? It is to expose the details as the JVM sees, which may be different from what is statically defined in the classfile if agents are involved. - PR Comment: https://git.openjdk.org/jdk/pull/15005#issuecomment-1653934767
RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
This patch adds NestHost and NestMembers attributes to the class dumped by SA. Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb` Manual testing by dumping `j.l.String` and `j.l.String$CaseInsensitiveComparator` classes. `j.l.String` shows one entry in `NestMembers` attribute for `j.l.String$CaseInsensitiveComparator` and `j.l.String$CaseInsensitiveComparator` has `j.l.String` as its `NestHost`. - Commit messages: - 8312623: SA add NestHost and NestMembers attributes when dumping class Changes: https://git.openjdk.org/jdk/pull/15005/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15005=00 Issue: https://bugs.openjdk.org/browse/JDK-8312623 Stats: 50 lines in 3 files changed: 50 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15005.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15005/head:pull/15005 PR: https://git.openjdk.org/jdk/pull/15005
Integrated: 8311971: SA's ConstantPool.java uses incorrect computation to read long value in the constant pool
On Wed, 12 Jul 2023 19:48:52 GMT, Ashutosh Mehra wrote: > Please review this fix to correctly read a long value in the runtime constant > pool. Details are mentioned in the issue [0]. > As an example, before this fix the long value generated by SA's dumpclass for > java.lang.String.serialVersionUID was: > > > private static final long serialVersionUID; > descriptor: J > flags: (0x001a) ACC_PRIVATE, ACC_STATIC, ACC_FINAL > ConstantValue: long 2050732866l > > > After this fix value of java.lang.String.serialVersionUID is: > > > private static final long serialVersionUID; > descriptor: J > flags: (0x001a) ACC_PRIVATE, ACC_STATIC, ACC_FINAL > ConstantValue: long -6849794470754667710l > > > Correct value as obtained from original java.lang.String is > `-6849794470754667710l`. > > Testing: tests under `serviceability/sa` and `sun/tools/jhsdb` are passing. > > [0] https://bugs.openjdk.org/browse/JDK-8311971 This pull request has now been integrated. Changeset: c1190375 Author:Ashutosh Mehra Committer: Thomas Stuefe URL: https://git.openjdk.org/jdk/commit/c1190375fc6def8a5520549157389f615161d7d7 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod 8311971: SA's ConstantPool.java uses incorrect computation to read long value in the constant pool Reviewed-by: cjplummer, dholmes, stuefe - PR: https://git.openjdk.org/jdk/pull/14855
Re: RFR: 8311971: SA's ConstantPool.java uses incorrect computation to read long value in the constant pool
On Mon, 17 Jul 2023 22:59:30 GMT, David Holmes wrote: > Approved, but please file that follow up issue. Follow up issue for removing the dead code - https://bugs.openjdk.org/browse/JDK-8312232 - PR Comment: https://git.openjdk.org/jdk/pull/14855#issuecomment-1640234531
Re: RFR: 8311971: SA's ConstantPool.java uses incorrect computation to read long value in the constant pool
On Mon, 17 Jul 2023 02:37:38 GMT, David Holmes wrote: > I think we need a follow-up RFE to get rid of buildLongFromIntsPD and any > other dead code. okay. @dholmes-ora can you please approve it if there are no other concerns. - PR Comment: https://git.openjdk.org/jdk/pull/14855#issuecomment-1638122858
Re: RFR: 8311971: SA's ConstantPool.java uses incorrect computation to read long value in the constant pool
On Fri, 14 Jul 2023 03:08:50 GMT, David Holmes wrote:
> First, what is it about constructing the long from two ints that is incorrect?
The incorrect part is fetching the ints from index and index+1. This doesn't
work for 64-bit platforms because the single entry in runtime constant pool is
large enough to store the long value, so the entry at index+1 is not used and
is invalid.
In ClassFileParser::parse_constant_pool_entries():
case JVM_CONSTANT_Long: {
// some code
const u8 bytes = cfs->get_u8_fast();
cp->long_at_put(index, bytes);
index++; // Skip entry following eigth-byte constant, see JVM book p.
98
The existing code that uses VM.buildLongFromIntsPD() would have worked if each
constant pool entry is of 4 bytes. So 32-bit systems it wouldn't be a problem.
>From my understanding getJLongAt() should work for both 32 and 64 bit systems.
> the fact we have VM.buildLongFromIntsPD suggests this is the intended way to
> do things. Why do we also have Address.getJLongAt()? Do we not actually need
> VM.buildLongFromIntsPD?
It seems both would produce the same result i.e. getJLongAt(0x1000) ==
VM.buildLongFromIntsPD(getIntAt(0x1004), getIntAt(0x1000));
> Is its other use in the code in
> ./jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/StackValueCollection.java
> also incorrect?
Not sure about that. Looking at the code for StackValueCollection.longAt() it
would depend on whether any value is stored in slot+1. If the slots in the
stack behave the same as the runtime constant pool, then it might be incorrect.
Also there are no users of StackValueCollection.longAt() method, so its a dead
code as of now.
> how can it be that we seemingly have no test for this???
As per my knowledge we have very basic tests that just verify that the
classfile generated by SA is parseable by javap. There is not test that checks
for equality of each individual component of the classfile.
I have a test [0] that can be a good proxy for ensuring the generated
classfiles are in a use-able shape because this test has helped me in
uncovering quite a few issues including this one and another one in handling of
invokedynamic bytecodes which is being fixed here
https://github.com/openjdk/jdk/pull/14852 (although it was reported through
another channel).
[0] https://bugs.openjdk.org/browse/JDK-8311101
-
PR Comment: https://git.openjdk.org/jdk/pull/14855#issuecomment-1636011869
Integrated: 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. This pull request has now been integrated. Changeset: c710e711 Author:Ashutosh Mehra Committer: Thomas Stuefe URL: https://git.openjdk.org/jdk/commit/c710e711780b3c334fdb9e1299b3c39a2b48649e Stats: 426 lines in 9 files changed: 408 ins; 5 del; 13 mod 8311102: Write annotations in the classfile dumped by SA Reviewed-by: cjplummer, stuefe - PR: https://git.openjdk.org/jdk/pull/14735
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: 8311971: SA's ConstantPool.java uses incorrect computation to read long value in the constant pool
On Wed, 12 Jul 2023 19:48:52 GMT, Ashutosh Mehra wrote: > Please review this fix to correctly read a long value in the runtime constant > pool. Details are mentioned in the issue [0]. > As an example, before this fix the long value generated by SA's dumpclass for > java.lang.String.serialVersionUID was: > > > private static final long serialVersionUID; > descriptor: J > flags: (0x001a) ACC_PRIVATE, ACC_STATIC, ACC_FINAL > ConstantValue: long 2050732866l > > > After this fix value of java.lang.String.serialVersionUID is: > > > private static final long serialVersionUID; > descriptor: J > flags: (0x001a) ACC_PRIVATE, ACC_STATIC, ACC_FINAL > ConstantValue: long -6849794470754667710l > > > Correct value as obtained from original java.lang.String is > `-6849794470754667710l`. > > Testing: tests under `serviceability/sa` and `sun/tools/jhsdb` are passing. > > [0] https://bugs.openjdk.org/browse/JDK-8311971 All tests under `serviceability/sa` and `sun/tools/jhsdb` are passing. - PR Comment: https://git.openjdk.org/jdk/pull/14855#issuecomment-1634275963
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
RFR: 8311971: SA's ConstantPool.java uses incorrect computation to read long value in the constant pool
Please review this fix to correctly read a long value in the runtime constant pool. Details are mentioned in the issue [0]. Before this fix the long value generated by SA's dumpclass for java.lang.String.serialVersionUID was: private static final long serialVersionUID; descriptor: J flags: (0x001a) ACC_PRIVATE, ACC_STATIC, ACC_FINAL ConstantValue: long 2050732866l After this fix value of java.lang.String.serialVersionUID is: private static final long serialVersionUID; descriptor: J flags: (0x001a) ACC_PRIVATE, ACC_STATIC, ACC_FINAL ConstantValue: long -6849794470754667710l Correct value as obtained from original java.lang.String is -6849794470754667710l. [0] https://bugs.openjdk.org/browse/JDK-8311971 - Commit messages: - 8311971: SA's ConstantPool.java uses incorrect computation to read long value in the constant pool Changes: https://git.openjdk.org/jdk/pull/14855/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14855=00 Issue: https://bugs.openjdk.org/browse/JDK-8311971 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14855.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14855/head:pull/14855 PR: https://git.openjdk.org/jdk/pull/14855
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=14735=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14735=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 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=14735=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14735=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=14735=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14735=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: JDK-8293114: JVM should trim the native heap [v8]
On Mon, 10 Jul 2023 13:53:36 GMT, Thomas Stuefe wrote:
>> This is a continuation of https://github.com/openjdk/jdk/pull/10085. I
>> closed https://github.com/openjdk/jdk/pull/10085 because it had accumulated
>> too much comment history and got confusing. For a history of this issue, see
>> previous discussions [1] and the comment section of 10085.
>>
>> ---
>>
>> This RFE adds the option to trim the Glibc heap periodically. This can
>> recover a significant memory footprint if the VM process suffers from
>> high-but-rare malloc spikes. It does not matter who causes the spikes: the
>> JDK or customer code running in the JVM process.
>>
>> ### Background:
>>
>> The Glibc is reluctant to return memory to the OS. Temporary malloc spikes
>> often carry over as permanent RSS increase. Note that C-heap retention is
>> difficult to observe. Since it is freed memory, it won't appear in NMT; it
>> is just a part of RSS.
>>
>> This is, effectively, caching - a performance tradeoff by the glibc. It
>> makes a lot of sense with applications that cause high traffic on the
>> C-heap. The JVM, however, clusters allocations and often rolls its own
>> memory management based on virtual memory for many of its use cases.
>>
>> To manually trim the C-heap, Glibc exposes `malloc_trim(3)`. With JDK 18
>> [2], we added a new jcmd command to *manually* trim the C-heap on Linux
>> (`jcmd System.trim_native_heap`). We then observed customers running this
>> command periodically to slim down process sizes of container-bound jvms.
>> That is cumbersome, and the JVM can do this a lot better - among other
>> things because it knows best when *not* to trim.
>>
>> GLIBC internals
>>
>> The following information I took from the glibc source code and
>> experimenting.
>>
>> # Why do we need to trim manually? Does the Glibc not trim on free?
>>
>> Upon `free()`, glibc may return memory to the OS if:
>> - the returned block was mmap'ed
>> - the returned block was not added to tcache or to fastbins
>> - the returned block, possibly merged with its two immediate neighbors, had
>> they been free, is larger than FASTBIN_CONSOLIDATION_THRESHOLD (64K) - in
>> that case:
>> a) for the main arena, glibc attempts to lower the brk()
>> b) for mmap-ed heaps, glibc attempts to completely unmap or shrink the
>> heap.
>> In both cases, (a) and (b), only the top portion of the heap is reclaimed.
>> "Holes" in the middle of other in-use chunks are not reclaimed.
>>
>> So: glibc *may* automatically reclaim memory. In normal configurations, with
>> typical C-heap allocation granularity, it is unlikely.
>>
>> To increase the ...
>
> Thomas Stuefe has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 32 additional
> commits since the last revision:
>
> - Make test spikes more pronounced
> - Dont query procfs if logging is off
> - rename logtag again
> - When probing for safepoint end, use the smaller of (interval, 250ms)
> - Remove TrimNativeHeap and expand TrimNativeHeapInterval
> - Improve comments for non-supportive platforms
> - Aleksey cosmetics
> - suspend count return 16 bits
> - Fix linker errors
> - Merge branch 'master' into JDK-8293114-JVM-should-trim-the-native-heap
> - ... and 22 more: https://git.openjdk.org/jdk/compare/061a10c7...15566761
src/hotspot/share/runtime/trimNativeHeap.cpp line 139:
> 137: double t2 = now();
> 138: if (sc.after != SIZE_MAX) {
> 139: const size_t delta = sc.after < sc.before ? (sc.before -
> sc.after) : (sc.after - sc.before);
@tstuefe under what situations can `sc.after` be more than `sc.before` after
trimming? Is it to handle the case where memory allocations happened in-between
the malloc_trim() and the calls to get process memory?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14781#discussion_r1258323486
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
Re: RFR: JDK-8293114: JVM should trim the native heap [v4]
On Fri, 7 Jul 2023 13:38:34 GMT, Thomas Stuefe wrote: > (just noticed the patch adds +666 lines, bad sign, I should add another line > somewhere). It also deletes 2 lines so that makes it 664 - PR Comment: https://git.openjdk.org/jdk/pull/14781#issuecomment-1625515410
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=14735=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
Integrated: 8309979: BootstrapMethods attribute is missing in class files recreated by SA
On Thu, 15 Jun 2023 15:06:54 GMT, Ashutosh Mehra wrote: > Please review this PR that extends SA to write BootstrapMethods attribute > when dumping the class files. > > Tested it by dumping the class file for java/lang/String and comparing the > BootstrapMethods attribute shown by javap for the original and the dumped > class. This pull request has now been integrated. Changeset: 05c2b6cd Author:Ashutosh Mehra Committer: Kevin Walls URL: https://git.openjdk.org/jdk/commit/05c2b6cd47c68d96dcb7b3db594a334e05c6ee36 Stats: 92 lines in 3 files changed: 84 ins; 2 del; 6 mod 8309979: BootstrapMethods attribute is missing in class files recreated by SA Reviewed-by: cjplummer, kevinw - PR: https://git.openjdk.org/jdk/pull/14495
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v3]
On Thu, 29 Jun 2023 14:16:16 GMT, Kevin Walls wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add comment about the layout of operands array in constant pool >> >> Signed-off-by: Ashutosh Mehra > > Yes 8-) @kevinjwalls thank you! - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1613263248
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Fri, 16 Jun 2023 18:10:58 GMT, Chris Plummer wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments by plummercj >> >> Signed-off-by: Ashutosh Mehra > > Yes, it is already problem listed. How did you run the tests? If you use > "make test" it should be including the problem list automatically. @plummercj @kevinjwalls can either of you sponsor it as well? - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1613247796
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Fri, 16 Jun 2023 18:10:58 GMT, Chris Plummer wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments by plummercj >> >> Signed-off-by: Ashutosh Mehra > > Yes, it is already problem listed. How did you run the tests? If you use > "make test" it should be including the problem list automatically. Thanks @plummercj @kevinjwalls @sspitsyn for reviewing the PR. - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1609535045
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Thu, 22 Jun 2023 16:10:13 GMT, Ashutosh Mehra wrote: > I am thinking of a comprehensive test that creates a classfile with specific > attribute, load it in the VM, dump that class file using SA, then disassemble > the generated class file to check for the presence of the attribute. We would > also need some mechanism to ensure all attributes and cp tags supported by > the VM level being tested are covered. Does that sound feasible? I am working on a test case that uses `buildreplayjars` command to dump the boot and app classfiles and reuse them to run the same application again. I think that would provide good coverage for classfile dumping functionality as well and would remove the requirement to check every other attribute in the dumped classfile. But the new test currently fails because we are not dumping many of the attributes in the classfile. So far I have added support for Annotations, NestMembers and NestHost. Once I have the test working I will open the PR to add it. As for this PR, we can either wait for the new test and other changes to be ready, or integrate it based on the manual testing. I don't mind either approach. - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1609484548
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Thu, 22 Jun 2023 03:21:33 GMT, Serguei Spitsyn wrote:
> Do we have any automatic test coverage for this?
Nope, I don't think there is any test for BootstrapMethods. As mentioned
[here](https://github.com/openjdk/jdk/pull/14556#issuecomment-1601946451) the
existing tests - ClhsdbDumpclass.java and TestClassDump.java - are very basic.
I think there is scope for improving testing so that SA's dump feature does not
become stale over a period of time as new attributes/tags are added to the
classfile format.
I am thinking of a comprehensive test that creates a classfile with specific
attribute, load it in the VM, dump that class file using SA, then disassemble
the generated class file to check for the presence of the attribute. We would
also need some mechanism to ensure all attributes and cp tags supported by the
VM level being tested are covered. Does that sound feasible?
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstantPool.java
> line 475:
>
>> 473: if (operands != null) {
>> 474: count = getOperandOffsetAt(operands, 0) / 2;
>> 475: }
>
> Nit: Could you, please, add a small comment why the bootstrap methods count
> is calculated this way?
Done.
-
PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1602943392
PR Review Comment: https://git.openjdk.org/jdk/pull/14495#discussion_r1238752462
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v3]
> Please review this PR that extends SA to write BootstrapMethods attribute > when dumping the class files. > > Tested it by dumping the class file for java/lang/String and comparing the > BootstrapMethods attribute shown by javap for the original and the dumped > class. Ashutosh Mehra has updated the pull request incrementally with one additional commit since the last revision: Add comment about the layout of operands array in constant pool Signed-off-by: Ashutosh Mehra - Changes: - all: https://git.openjdk.org/jdk/pull/14495/files - new: https://git.openjdk.org/jdk/pull/14495/files/3a7ad373..4cb63978 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14495=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14495=01-02 Stats: 15 lines in 1 file changed: 15 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14495/head:pull/14495 PR: https://git.openjdk.org/jdk/pull/14495
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Wed, 21 Jun 2023 12:47:10 GMT, Kevin Walls wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments by plummercj >> >> Signed-off-by: Ashutosh Mehra > > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/jcore/ClassWriter.java > line 756: > >> 754: } >> 755: } >> 756: } > > Hi, it looks odd to me that we only write one short in the loop: should that > be a bootstrap method ref, num arguments, and arguments? > > I was comparing with: > https://docs.oracle.com/javase/specs/jvms/se20/jvms20.pdf > 4.7.23 The BootstrapMethods Attribute > > We have bootstrap attribute written by other code, e.g. > "src/jdk.jdeps/share/classes/com/sun/tools/classfile/ClassWriter.java" > visitBootstrapMethods > > "src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassWriter.java" > writeBootstrapMethods This is because the array returned by getBootstrapMethodAt() includes bootstrap method ref, num arguments and arguments. getBootstrapMethodAt() gets all the pieces and assembles them in the array which is then written out in this loop. There is already getBootstrapSpecifierAt() which is doing the same thing. So I just reused that implementation. - PR Review Comment: https://git.openjdk.org/jdk/pull/14495#discussion_r1237011431
Re: RFR: 8242152: SA does not include StackMapTables when dumping .class files
On Tue, 20 Jun 2023 11:31:40 GMT, Daohan Qu wrote: > This patch adds `StackMapTable` for the class files generated by `clhsdb`'s > `buildreplayjars` command. This bug manifests itself during my diagnosing > [JDK-8309751](https://bugs.openjdk.org/browse/JDK-8309751) and needs to be > fixed first. > > I have run jtreg test `tier1-3` of release build on x86 linux finding only > one failure in `tier2` caused by > [JDK-8309214](https://bugs.openjdk.org/browse/JDK-8309214). > `jtreg:test/hotspot/jtreg/serviceability` and `jtreg:test/jdk/sun/tools/` > also passed. @quadhier you beat me to this! Changes look good. On another note since you mentioned [JDK-8309751](https://bugs.openjdk.org/browse/JDK-8309751) I should let you know I have a fix for it and am planning to open a PR soon. My bad that I didn't assign that issue to myself before starting working on it. - PR Comment: https://git.openjdk.org/jdk/pull/14556#issuecomment-1598858991
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Fri, 16 Jun 2023 18:10:58 GMT, Chris Plummer wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments by plummercj >> >> Signed-off-by: Ashutosh Mehra > > Yes, it is already problem listed. How did you run the tests? If you use > "make test" it should be including the problem list automatically. @plummercj do you have any suggestions for who can be the second reviewer? - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1597697220
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Thu, 15 Jun 2023 20:24:39 GMT, Ashutosh Mehra wrote: >> Please review this PR that extends SA to write BootstrapMethods attribute >> when dumping the class files. >> >> Tested it by dumping the class file for java/lang/String and comparing the >> BootstrapMethods attribute shown by javap for the original and the dumped >> class. > > Ashutosh Mehra has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments by plummercj > > Signed-off-by: Ashutosh Mehra I didn't run it through `make test`. I used `java -jar jtreg.jar `. - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1595145103
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Thu, 15 Jun 2023 23:20:56 GMT, Chris Plummer wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments by plummercj >> >> Signed-off-by: Ashutosh Mehra > > Can you run the tests in `test/hotspot/jtreg/serviceability/sa` and > `test/jdk/sun/tools/jhsdb/`? thanks. @plummercj I ran `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb/` . Thanks for pointing out. I see only one failure - `test/hotspot/jtreg/serviceability/sa/TestRevPtrsForInvokeDynamic.java`. However this test failed with the same exception without this patch as well. Reason for the failure is: stderr: [Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: -16 19 at jdk.hotspot.agent/sun.jvm.hotspot.oops.ResolvedIndyArray.getAt(ResolvedIndyArray.java:60) at jdk.hotspot.agent/sun.jvm.hotspot.oops.ConstantPoolCache.getIndyEntryAt(ConstantPoolCache.java:91) at jdk.hotspot.agent/sun.jvm.hotspot.oops.ConstantPool.to_cp_index(ConstantPool.java:261) at jdk.hotspot.agent/sun.jvm.hotspot.oops.ConstantPool.getNameAndTypeRefIndexAt(ConstantPool.java:343) at jdk.hotspot.agent/sun.jvm.hotspot.oops.ConstantPool.getSignatureRefAt(ConstantPool.java:303) at jdk.hotspot.agent/sun.jvm.hotspot.oops.GenerateOopMap.doMethod(GenerateOopMap.java:1730) at jdk.hotspot.agent/sun.jvm.hotspot.oops.GenerateOopMap.interp1(GenerateOopMap.java:1385) at jdk.hotspot.agent/sun.jvm.hotspot.oops.GenerateOopMap.interpBB(GenerateOopMap.java:802) at jdk.hotspot.agent/sun.jvm.hotspot.oops.GenerateOopMap.interpAll(GenerateOopMap.java:1108) at jdk.hotspot.agent/sun.jvm.hotspot.oops.GenerateOopMap.doInterpretation(GenerateOopMap.java:981) at jdk.hotspot.agent/sun.jvm.hotspot.oops.GenerateOopMap.computeMap(GenerateOopMap.java:2198) at jdk.hotspot.agent/sun.jvm.hotspot.interpreter.OopMapForCacheEntry.computeMap(OopMapForCacheEntry.java:80) at jdk.hotspot.agent/sun.jvm.hotspot.interpreter.OopMapCacheEntry.fill(OopMapCacheEntry.java:53) at jdk.hotspot.agent/sun.jvm.hotspot.oops.Method.getMaskFor(Method.java:257) at jdk.hotspot.agent/sun.jvm.hotspot.runtime.Frame.oopsInterpretedDo(Frame.java:590) at jdk.hotspot.agent/sun.jvm.hotspot.runtime.Frame.oopsDo(Frame.java:442) at jdk.hotspot.agent/sun.jvm.hotspot.utilities.ReversePtrsAnalysis.doStack(ReversePtrsAnalysis.java:297) at jdk.hotspot.agent/sun.jvm.hotspot.utilities.ReversePtrsAnalysis.run(ReversePtrsAnalysis.java:102) at TestRevPtrsForInvokeDynamic.computeReversePointers(TestRevPtrsForInvokeDynamic.java:60) at TestRevPtrsForInvokeDynamic.main(TestRevPtrsForInvokeDynamic.java:92) It looks like this test is already in the ProblemList because of [JDK-8241235](https://bugs.openjdk.org/browse/JDK-8241235). - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1594974422
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Thu, 15 Jun 2023 20:55:12 GMT, Chris Plummer wrote: > Can you tell me what testing you've done? Would be best to call that out in > the PR description. Edited the description to add a comment about the testing. Hope this helps. - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1593820565
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Thu, 15 Jun 2023 18:15:41 GMT, Chris Plummer wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments by plummercj >> >> Signed-off-by: Ashutosh Mehra > > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstantPool.java > line 498: > >> 496: U2Array operands = getOperands(); >> 497: if (operands == null) return null; // safety first >> 498: int basePos = getOperandOffsetAt(bsmIndex); > > Maybe you should pass `operands` into `getOperandOffsetAt()` so it does not > need to be fetched again. Done. > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/jcore/ClassWriter.java > line 694: > >> 692: if (bsmCount != 0) >> 693: classAttributeCount++; >> 694: > > Please use curly braces here. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/14495#discussion_r1231495232 PR Review Comment: https://git.openjdk.org/jdk/pull/14495#discussion_r1231495480
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
> Please review this PR that extends SA to write BootstrapMethods attribute > when dumping the class files. Ashutosh Mehra has updated the pull request incrementally with one additional commit since the last revision: Address review comments by plummercj Signed-off-by: Ashutosh Mehra - Changes: - all: https://git.openjdk.org/jdk/pull/14495/files - new: https://git.openjdk.org/jdk/pull/14495/files/a2755e4e..3a7ad373 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14495=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14495=00-01 Stats: 10 lines in 2 files changed: 1 ins; 4 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/14495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14495/head:pull/14495 PR: https://git.openjdk.org/jdk/pull/14495
RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA
Please review this PR that extends SA to write BootstrapMethods attribute when dumping the class files. - Commit messages: - 8309979: BootstrapMethods attribute is missing in class files recreated by SA Changes: https://git.openjdk.org/jdk/pull/14495/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14495=00 Issue: https://bugs.openjdk.org/browse/JDK-8309979 Stats: 80 lines in 3 files changed: 72 ins; 2 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/14495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14495/head:pull/14495 PR: https://git.openjdk.org/jdk/pull/14495
Re: RFR: 8298048: Combine CDS archive heap into a single block
On Mon, 3 Apr 2023 03:32:27 GMT, Ioi Lam wrote:
> This PR combines the "open" and "closed" regions of the CDS archive heap into
> a single region. This significantly simplifies the implementation, making it
> more compatible with non-G1 collectors. There's a net removal of ~1000 lines
> in src code and another ~1200 lines of tests.
>
> **Notes for reviewers:**
> - Most of the code changes in CDS are removing the handling of "open" vs
> "closed" objects.
> - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer().
> - It might be easier to see the diff with whitespaces off.
> - There are two major changes in the G1 code
> - The archived objects are now stored in the "old" region (see
> g1CollectedHeap.cpp in
> [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770))
> - The majority of the other changes are removal of the "archive" region
> type (see heapRegionType.hpp). For ease of review, such code is isolated in
> [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6)
> - Testing changes:
> - Now the archived java objects can move, along with the "old" regions that
> contain them. It's no longer possible to test whether a heap object came from
> CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed.
> - Many tests that uses this API are removed. Most of them were written
> during early development of CDS archived objects and are no longer relevant.
>
> **Testing:**
> - Mach5 tiers 1 ~ 7
Marked as reviewed by ashu-me...@github.com (no known OpenJDK username).
cds changes look good! just few nitpicks.
src/hotspot/share/cds/archiveHeapLoader.cpp line 265:
> 263:MemRegion& archive_space) {
> 264: size_t total_bytes = 0;
> 265: int i = MetaspaceShared::hp;
nitpick: this can be replaced with a better variable name instead of `i`,
probably region_idx.
src/hotspot/share/cds/archiveHeapLoader.cpp line 274:
> 272: assert(is_aligned(r->used(), HeapWordSize), "must be");
> 273: total_bytes += r->used();
> 274: loaded_region->_region_index = i;
nitpick: we can do away with `_region_index` and use `MetaspaceShared::hp`
wherever required.
src/hotspot/share/cds/archiveHeapLoader.cpp line 445:
> 443: }
> 444:
> 445: int i = MetaspaceShared::hp;
nitpick: same as before, suggest to replace `i` with `region_idx`.
src/hotspot/share/cds/archiveHeapWriter.cpp line 54:
> 52: // The following are offsets from buffer_bottom()
> 53: size_t ArchiveHeapWriter::_buffer_used;
> 54: size_t ArchiveHeapWriter::_heap_roots_bottom;
nitpick: would be clearer if `_heap_roots_bottom` is named as
`_heap_roots_bottom_offset`
src/hotspot/share/cds/metaspaceShared.hpp line 63:
> 61: ro = 1, // read-only shared space
> 62: bm = 2, // relocation bitmaps (freed after file mapping is finished)
> 63: hp = 3, // relocation bitmaps (freed after file mapping is finished)
This comment needs to be updated.
-
PR Review: https://git.openjdk.org/jdk/pull/13284#pullrequestreview-1380125495
PR Comment: https://git.openjdk.org/jdk/pull/13284#issuecomment-1504143568
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163361181
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163361230
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163361633
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163362914
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163362267
