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
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&pr=14495&range=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: 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&pr=14495&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14495&range=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
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]
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 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: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 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: 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 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: 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&pr=14495&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14495&range=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 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 [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 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 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 [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
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
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
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
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 [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 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 [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 [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]
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 [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 [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 [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 [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 [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
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&pr=14855&range=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 [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: 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 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
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: 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
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 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
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
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&pr=15005&range=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
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
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, 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 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 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 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
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 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
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&pr=15952&range=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: 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
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 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 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 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 reviewing. - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1739179894
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
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&pr=15973&range=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
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
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 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
RFR: 8337031: Improvements to CompilationMemoryStatistic
Some minor improvements to CompilationMemoryStatistic. More details are in [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) Testing: test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java - Commit messages: - Update comments in tests to reflect new output format - 8337031: Improvements to CompilationMemoryStatistic Changes: https://git.openjdk.org/jdk/pull/20304/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20304&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337031 Stats: 173 lines in 6 files changed: 77 ins; 21 del; 75 mod Patch: https://git.openjdk.org/jdk/pull/20304.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20304/head:pull/20304 PR: https://git.openjdk.org/jdk/pull/20304
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic
On Tue, 23 Jul 2024 21:46:50 GMT, Ashutosh Mehra wrote: > Some minor improvements to CompilationMemoryStatistic. More details are in > [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) > > Testing: > test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java > > test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java @tstuefe fyi - PR Comment: https://git.openjdk.org/jdk/pull/20304#issuecomment-2246372292
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic
On Fri, 26 Jul 2024 06:08:03 GMT, Thomas Stuefe wrote:
>> Some minor improvements to CompilationMemoryStatistic. More details are in
>> [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031)
>>
>> Testing:
>> test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java
>>
>> test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java
>
> src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 118:
>
>> 116: if (total != _current) {
>> 117: log_info(compilation, alloc)("WARNING!!! Total does not match
>> current");
>> 118: }
>
> Why do we calculate total? Just for this test? I would then put this into an
> ASSERT section, and make the info log an assert.
>
> However, I wonder if this is really needed. The logic updating both _current
> and _tags_size is pretty straightforward, I don't see how there could be a
> mismatch.
This code should not have been there. I forgot to remove it. There is no use of
`total` here.
> src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 204:
>
>> 202: size_t _total;
>> 203: // usage per arena tag when total peaked
>> 204: size_t _tags_size_at_peak[Arena::tag_count()];
>
> Can you please make sure Arena::tag_count() evaluates to constexpr? When in
> doubt, just use the enum value instead.
Arena::tag_count() is declared as a constexpr. I wanted to avoid writing
`static_cast(Arena::Tag::tag_count)` every time I need tag_count, so I
wrapped it in Arena::tag_count() and declared it with constexpr. Is that not
sufficient to make it a constexpr?
> src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 242:
>
>> 240: for (int tag = 0; tag < Arena::tag_count(); tag++) {
>> 241: st->print_cr(" " LEGEND_KEY_FMT ": %s", Arena::tag_name[tag],
>> Arena::tag_desc[tag]);
>> 242: }
>
> use x macro?
What do you mean by x macro? Do you have an example that shows the use of x
macro?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1693443814
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1693443227
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1693445269
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic [v2]
> Some minor improvements to CompilationMemoryStatistic. More details are in > [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) > > Testing: > test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java > > test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java Ashutosh Mehra has updated the pull request incrementally with one additional commit since the last revision: Address review comments by Thomas S. Signed-off-by: Ashutosh Mehra - Changes: - all: https://git.openjdk.org/jdk/pull/20304/files - new: https://git.openjdk.org/jdk/pull/20304/files/1ffbd696..008ac6b9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20304&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20304&range=00-01 Stats: 91 lines in 4 files changed: 31 ins; 30 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/20304.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20304/head:pull/20304 PR: https://git.openjdk.org/jdk/pull/20304
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic
On Wed, 24 Jul 2024 10:45:05 GMT, Thomas Stuefe wrote: >> Some minor improvements to CompilationMemoryStatistic. More details are in >> [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) >> >> Testing: >> test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java >> >> test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java > > I plan to look at this later this week. @tstuefe I have added a patch to address your review comments. > or just write a small wrapper class holding a size_t vector and taking care > of the copying. Using a wrapper class is a good idea. I have added `ArenaTagsCounter` for that. - PR Comment: https://git.openjdk.org/jdk/pull/20304#issuecomment-2256315773
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic [v2]
On Sat, 27 Jul 2024 05:44:14 GMT, Thomas Stuefe wrote:
>> What do you mean by x macro? Do you have an example that shows the use of x
>> macro?
>
> You use them already in your patch.
>
> E.g.
>
>
> #define XX(name, whatever, desc) st->print_cr(" " LEGEND_KEY_FMT ": " #name
> #desc
> DO_ARENA_TAG(XX)
> #undef XX
>
>
> Admittedly, it is not a lot less code than the for loop. Up to you.
I will keep the loop if you don't have strong preference for the macro usage
here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1695486252
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic [v2]
On Tue, 30 Jul 2024 05:18:17 GMT, Thomas Stuefe wrote:
>> Ashutosh Mehra has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address review comments by Thomas S.
>>
>> Signed-off-by: Ashutosh Mehra
>
> src/hotspot/share/compiler/compilationMemoryStatistic.hpp line 40:
>
>> 38:
>> 39: // Helper class to wrap the array of arena tags for easier processing
>> 40: class ArenaTagsCounter {
>
> Sorry for being a stickler for precise names, but I would like plural for
> counters here - it is not a single counter, its a series/vector/array of
> counters.
> Any of these work for me: ArenaCountersByTag - ArenaCountersByTagVector -
> ArenaTagCounterVector - ArenaTagCounters
I am pretty bad in naming things, so I welcome these suggestions. I will go
with ArenaCountersByTag.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1696973723
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic [v3]
> Some minor improvements to CompilationMemoryStatistic. More details are in > [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) > > Testing: > test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java > > test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java Ashutosh Mehra has updated the pull request incrementally with one additional commit since the last revision: Rename ArenaTagsCounter to ArenaCountersByTag Signed-off-by: Ashutosh Mehra - Changes: - all: https://git.openjdk.org/jdk/pull/20304/files - new: https://git.openjdk.org/jdk/pull/20304/files/008ac6b9..70762110 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20304&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20304&range=01-02 Stats: 7 lines in 2 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/20304.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20304/head:pull/20304 PR: https://git.openjdk.org/jdk/pull/20304
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic
On Wed, 24 Jul 2024 10:45:05 GMT, Thomas Stuefe wrote: >> Some minor improvements to CompilationMemoryStatistic. More details are in >> [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) >> >> Testing: >> test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java >> >> test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java > > I plan to look at this later this week. @tstuefe renamed ArenaTagsCounter to ArenaCountersByTag - PR Comment: https://git.openjdk.org/jdk/pull/20304#issuecomment-2258563618
Integrated: 8337031: Improvements to CompilationMemoryStatistic
On Tue, 23 Jul 2024 21:46:50 GMT, Ashutosh Mehra wrote: > Some minor improvements to CompilationMemoryStatistic. More details are in > [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) > > Testing: > test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java > > test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java This pull request has now been integrated. Changeset: e63d0165 Author:Ashutosh Mehra URL: https://git.openjdk.org/jdk/commit/e63d01654e0b702b3a8c0c4de97a6bb6893fbd1f Stats: 182 lines in 6 files changed: 84 ins; 27 del; 71 mod 8337031: Improvements to CompilationMemoryStatistic Reviewed-by: kvn, stuefe - PR: https://git.openjdk.org/jdk/pull/20304
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic
On Wed, 24 Jul 2024 10:45:05 GMT, Thomas Stuefe wrote: >> Some minor improvements to CompilationMemoryStatistic. More details are in >> [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) >> >> Testing: >> test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java >> >> test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java > > I plan to look at this later this week. thanks @tstuefe @vnkozlov for reviewing and testing - PR Comment: https://git.openjdk.org/jdk/pull/20304#issuecomment-2259466393
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v3]
On Tue, 10 Sep 2024 00:53:32 GMT, Ioi Lam wrote:
>> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading &
>> Linking](https://bugs.openjdk.org/browse/JDK-8315737).
>>
>> **Overview**
>>
>> - A new `-XX:+AOTClassLinking` flag is added. See [JEP
>> 498](https://bugs.openjdk.org/browse/JDK-8315737) and the
>> [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this
>> command-line option, its default value, and its impact on compatibility.
>> - When this flag is enabled during the creation of an AOT cache (aka CDS
>> archive), an `AOTLinkedClassTable` is added to the cache to include all
>> classes that are AOT-linked. For this PR, only classes for the
>> boot/platform/application loaders are eligible. The main logic is in
>> `aotClassLinker.cpp`.
>> - When an AOT archive is loaded in a production run, all classes in the
>> `AOTLinkedClassTable` are loaded into their respective class loaders at the
>> earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`.
>> - The boot classes are loaded as part of `vmClasses::resolve_all()`
>> - The platform/application classes are loaded after the module graph is
>> restored (see changes in `threads.cpp`).
>> - Since all classes in a `AOTLinkedClassTable` are loaded before any
>> user-code is executed, we can resolve constant pool entries that refer to
>> these classes during AOT cache creation. See changes in
>> `AOTConstantPoolResolver::is_class_resolution_deterministic()`.
>>
>> **All-or-nothing Loading**
>>
>> - Because AOT-linked classes can refer to each other, using direct C++
>> pointers, all AOT-linked classes must be loaded together. Otherwise we will
>> have dangling C++ pointers in the class metadata structures.
>> - During a production run, we check during VM start-up for incompatible VM
>> options that would prevent some of the AOT-linked classes from being loaded.
>> For example:
>> - If the VM is started with an JVMTI agent that has ClassFileLoadHook
>> capabilities, it could replace some of the AOT-linked classes with
>> alternative versions.
>> - If the VM is started with certain module options, such as
>> `--patch-module` or `--module`, some AOT-linked classes may be replaced with
>> patched versions, or may become invisible and cannot be loaded into the JVM.
>> - When incompatible VM options are detected, the JVM will refuse to load an
>> AOT cache that has AOT-linked classes. See
>> `FileMapInfo::validate_aot_class_linking()`.
>> - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires
>> `CDSConfig::is_using_full_module_graph()` to be true. This means that the
>> exa...
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> @dholmes-ora comments: logging indents
src/hotspot/share/cds/aotClassLinker.cpp line 149:
> 147: add_candidate(ik);
> 148:
> 149: if (log_is_enabled(Info, cds, aot, load)) {
Is `load` the correct log tag to use in this class? Can it be replaced with
`link` tag?
src/hotspot/share/cds/aotClassLinker.hpp line 111:
> 109:
> 110: static int num_app_initiated_classes();
> 111: static int num_platform_initiated_classes();
I don't see these methods (num_app_initiated_classes and
num_platform_initiated_classes) used anywhere. Should they be removed?
src/hotspot/share/cds/aotConstantPoolResolver.cpp line 111:
> 109:
> 110: if (CDSConfig::is_dumping_aot_linked_classes()) {
> 111: if (AOTClassLinker::try_add_candidate(ik)) {
Are we relying on the call to `try_add_candidate` to add the class to the
candidate list? I guess that shouldn't be the case as the class have already
been added through
ArchiveBuilder::gather_klasses_and_symbols()->AOTClassLinker::add_candidates().
If so can we use AOTClassLinker::is_candidate(ik) here?
src/hotspot/share/cds/archiveBuilder.cpp line 766:
> 764: #define ADD_COUNT(x) \
> 765: x += 1; \
> 766: x ## _a += aotlinked;
Can we do this instead:
```x ## _a += (aotlinked ? 1 : 0)```
and make `aotlinked` a bool.
src/hotspot/share/cds/archiveBuilder.cpp line 779:
> 777: DECLARE_INSTANCE_KLASS_COUNTER(num_app_klasses);
> 778: DECLARE_INSTANCE_KLASS_COUNTER(num_hidden_klasses);
> 779: DECLARE_INSTANCE_KLASS_COUNTER(num_unlinked_klasses);
Nit-picking here - "unlinked" category doesn't need the "aot-linked" counter.
src/hotspot/share/cds/filemap.cpp line 2455:
> 2453: const char* prop =
> Arguments::get_property("java.system.class.loader");
> 2454: if (prop != nullptr) {
> 2455: if (has_aot_linked_classes()) {
Should this check be part of `FileMapInfo::validate_aot_class_linking`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1752750296
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1752692333
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1752689932
PR Review Comment: https://git.openjdk.org/jdk/pull/2084
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v3]
On Tue, 10 Sep 2024 00:53:32 GMT, Ioi Lam wrote: >> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & >> Linking](https://bugs.openjdk.org/browse/JDK-8315737). >> >> **Overview** >> >> - A new `-XX:+AOTClassLinking` flag is added. See [JEP >> 498](https://bugs.openjdk.org/browse/JDK-8315737) and the >> [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this >> command-line option, its default value, and its impact on compatibility. >> - When this flag is enabled during the creation of an AOT cache (aka CDS >> archive), an `AOTLinkedClassTable` is added to the cache to include all >> classes that are AOT-linked. For this PR, only classes for the >> boot/platform/application loaders are eligible. The main logic is in >> `aotClassLinker.cpp`. >> - When an AOT archive is loaded in a production run, all classes in the >> `AOTLinkedClassTable` are loaded into their respective class loaders at the >> earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. >> - The boot classes are loaded as part of `vmClasses::resolve_all()` >> - The platform/application classes are loaded after the module graph is >> restored (see changes in `threads.cpp`). >> - Since all classes in a `AOTLinkedClassTable` are loaded before any >> user-code is executed, we can resolve constant pool entries that refer to >> these classes during AOT cache creation. See changes in >> `AOTConstantPoolResolver::is_class_resolution_deterministic()`. >> >> **All-or-nothing Loading** >> >> - Because AOT-linked classes can refer to each other, using direct C++ >> pointers, all AOT-linked classes must be loaded together. Otherwise we will >> have dangling C++ pointers in the class metadata structures. >> - During a production run, we check during VM start-up for incompatible VM >> options that would prevent some of the AOT-linked classes from being loaded. >> For example: >> - If the VM is started with an JVMTI agent that has ClassFileLoadHook >> capabilities, it could replace some of the AOT-linked classes with >> alternative versions. >> - If the VM is started with certain module options, such as >> `--patch-module` or `--module`, some AOT-linked classes may be replaced with >> patched versions, or may become invisible and cannot be loaded into the JVM. >> - When incompatible VM options are detected, the JVM will refuse to load an >> AOT cache that has AOT-linked classes. See >> `FileMapInfo::validate_aot_class_linking()`. >> - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires >> `CDSConfig::is_using_full_module_graph()` to be true. This means that the >> exa... > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > @dholmes-ora comments: logging indents Overall it looks fine - just some minor nit-picking. I haven't finished reviewing yet though. I will continue my review tomorrow. - PR Comment: https://git.openjdk.org/jdk/pull/20843#issuecomment-2342157928
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v3]
On Tue, 10 Sep 2024 22:17:00 GMT, Ashutosh Mehra wrote: > I will continue my review tomorrow. My review is done now. - PR Comment: https://git.openjdk.org/jdk/pull/20843#issuecomment-2346553992
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v4]
On Wed, 11 Sep 2024 20:26:51 GMT, Ioi Lam wrote:
>> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading &
>> Linking](https://bugs.openjdk.org/browse/JDK-8315737).
>>
>> **Overview**
>>
>> - A new `-XX:+AOTClassLinking` flag is added. See [JEP
>> 498](https://bugs.openjdk.org/browse/JDK-8315737) and the
>> [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this
>> command-line option, its default value, and its impact on compatibility.
>> - When this flag is enabled during the creation of an AOT cache (aka CDS
>> archive), an `AOTLinkedClassTable` is added to the cache to include all
>> classes that are AOT-linked. For this PR, only classes for the
>> boot/platform/application loaders are eligible. The main logic is in
>> `aotClassLinker.cpp`.
>> - When an AOT archive is loaded in a production run, all classes in the
>> `AOTLinkedClassTable` are loaded into their respective class loaders at the
>> earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`.
>> - The boot classes are loaded as part of `vmClasses::resolve_all()`
>> - The platform/application classes are loaded after the module graph is
>> restored (see changes in `threads.cpp`).
>> - Since all classes in a `AOTLinkedClassTable` are loaded before any
>> user-code is executed, we can resolve constant pool entries that refer to
>> these classes during AOT cache creation. See changes in
>> `AOTConstantPoolResolver::is_class_resolution_deterministic()`.
>>
>> **All-or-nothing Loading**
>>
>> - Because AOT-linked classes can refer to each other, using direct C++
>> pointers, all AOT-linked classes must be loaded together. Otherwise we will
>> have dangling C++ pointers in the class metadata structures.
>> - During a production run, we check during VM start-up for incompatible VM
>> options that would prevent some of the AOT-linked classes from being loaded.
>> For example:
>> - If the VM is started with an JVMTI agent that has ClassFileLoadHook
>> capabilities, it could replace some of the AOT-linked classes with
>> alternative versions.
>> - If the VM is started with certain module options, such as
>> `--patch-module` or `--module`, some AOT-linked classes may be replaced with
>> patched versions, or may become invisible and cannot be loaded into the JVM.
>> - When incompatible VM options are detected, the JVM will refuse to load an
>> AOT cache that has AOT-linked classes. See
>> `FileMapInfo::validate_aot_class_linking()`.
>> - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires
>> `CDSConfig::is_using_full_module_graph()` to be true. This means that the
>> exa...
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> @adinn comments
test/hotspot/jtreg/runtime/cds/appcds/jvmti/ClassFileLoadHookTest.java line 110:
> 108: "" + ClassFileLoadHook.TestCaseId.SHARING_ON_CFLH_ON);
> 109: if (out.contains("Using AOT-linked classes: false")) {
> 110: // We are running with VM options that do not support
> -XX:+AOTClassLinking
When will we run into this case? Is there a VM option that would silently
disable AOTClassLinking in prod run?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1756943024
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v5]
On Thu, 12 Sep 2024 21:43:24 GMT, Ioi Lam wrote:
>> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading &
>> Linking](https://bugs.openjdk.org/browse/JDK-8315737).
>>
>> **Overview**
>>
>> - A new `-XX:+AOTClassLinking` flag is added. See [JEP
>> 498](https://bugs.openjdk.org/browse/JDK-8315737) and the
>> [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this
>> command-line option, its default value, and its impact on compatibility.
>> - When this flag is enabled during the creation of an AOT cache (aka CDS
>> archive), an `AOTLinkedClassTable` is added to the cache to include all
>> classes that are AOT-linked. For this PR, only classes for the
>> boot/platform/application loaders are eligible. The main logic is in
>> `aotClassLinker.cpp`.
>> - When an AOT archive is loaded in a production run, all classes in the
>> `AOTLinkedClassTable` are loaded into their respective class loaders at the
>> earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`.
>> - The boot classes are loaded as part of `vmClasses::resolve_all()`
>> - The platform/application classes are loaded after the module graph is
>> restored (see changes in `threads.cpp`).
>> - Since all classes in a `AOTLinkedClassTable` are loaded before any
>> user-code is executed, we can resolve constant pool entries that refer to
>> these classes during AOT cache creation. See changes in
>> `AOTConstantPoolResolver::is_class_resolution_deterministic()`.
>>
>> **All-or-nothing Loading**
>>
>> - Because AOT-linked classes can refer to each other, using direct C++
>> pointers, all AOT-linked classes must be loaded together. Otherwise we will
>> have dangling C++ pointers in the class metadata structures.
>> - During a production run, we check during VM start-up for incompatible VM
>> options that would prevent some of the AOT-linked classes from being loaded.
>> For example:
>> - If the VM is started with an JVMTI agent that has ClassFileLoadHook
>> capabilities, it could replace some of the AOT-linked classes with
>> alternative versions.
>> - If the VM is started with certain module options, such as
>> `--patch-module` or `--module`, some AOT-linked classes may be replaced with
>> patched versions, or may become invisible and cannot be loaded into the JVM.
>> - When incompatible VM options are detected, the JVM will refuse to load an
>> AOT cache that has AOT-linked classes. See
>> `FileMapInfo::validate_aot_class_linking()`.
>> - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires
>> `CDSConfig::is_using_full_module_graph()` to be true. This means that the
>> exa...
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> @ashu-mehra comments
src/hotspot/share/cds/aotClassLinker.cpp line 227:
> 225: }
> 226:
> 227: int AOTClassLinker::num_initiated_classes(oop loader1, oop loader2) {
The two loader arguments here are quite confusing marking it hard to understand
the code. Can it be refactored as this:
int AOTClassLinker::num_platform_initiated_classes() {
// AOTLinkedClassBulkLoader will initiate loading of all public boot classes
in the platform loader.
return num_initiated_classes(nullptr);
}
int AOTClassLinker::num_app_initiated_classes() {
// AOTLinkedClassBulkLoader will initiate loading of all public boot/platform
classes in the app loader.
return num_platform_initiated_classes +
num_initiated_classes(SystemDictionary::java_platform_loader());
}
int AOTClassLinker::num_initiated_classes(oop loader) {
int n = 0;
for (int i = 0; i < _sorted_candidates->length(); i++) {
InstanceKlass* ik = _sorted_candidates->at(i);
if (ik->is_public() && !ik->is_hidden() &&
(ik->class_loader() == loader) {
n++;
}
}
return n;
}
src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 199:
> 197: InstanceKlass* ik = classes->at(i);
> 198: assert(ik->is_loaded(), "must have already been loaded by a parent
> loader");
> 199: assert(ik->class_loader() != initiating_loader(), "must be a parent
> loader");
Can we also add an assert that ik->class_loader() must be either boot or
platform loader.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1759128918
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1759129552
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v6]
On Sat, 7 Sep 2024 00:30:34 GMT, Ioi Lam wrote: >> I've taken an initial look through but there is an awful lot to try and >> digest here. I've flagged numerous typos and minor nits. >> >> One general query: does this stuff work if the user defines their own >> initial application classloader? > >> I've taken an initial look through but there is an awful lot to try and >> digest here. I've flagged numerous typos and minor nits. >> >> One general query: does this stuff work if the user defines their own >> initial application classloader? > > Hi David thanks for the review. I've pushed a new version that has most of > your suggestions. > > I also added code to avoid loading the CDS archive if it has aot-linked > classes, and the user has specified `-Djava.system.class.loader` @iklam thank you for addressing the comments. - PR Comment: https://git.openjdk.org/jdk/pull/20843#issuecomment-2354146490
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v6]
On Mon, 16 Sep 2024 21:54:49 GMT, Ioi Lam wrote: >> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & >> Linking](https://bugs.openjdk.org/browse/JDK-8315737). >> >> **Overview** >> >> - A new `-XX:+AOTClassLinking` flag is added. See [JEP >> 498](https://bugs.openjdk.org/browse/JDK-8315737) and the >> [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this >> command-line option, its default value, and its impact on compatibility. >> - When this flag is enabled during the creation of an AOT cache (aka CDS >> archive), an `AOTLinkedClassTable` is added to the cache to include all >> classes that are AOT-linked. For this PR, only classes for the >> boot/platform/application loaders are eligible. The main logic is in >> `aotClassLinker.cpp`. >> - When an AOT archive is loaded in a production run, all classes in the >> `AOTLinkedClassTable` are loaded into their respective class loaders at the >> earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. >> - The boot classes are loaded as part of `vmClasses::resolve_all()` >> - The platform/application classes are loaded after the module graph is >> restored (see changes in `threads.cpp`). >> - Since all classes in a `AOTLinkedClassTable` are loaded before any >> user-code is executed, we can resolve constant pool entries that refer to >> these classes during AOT cache creation. See changes in >> `AOTConstantPoolResolver::is_class_resolution_deterministic()`. >> >> **All-or-nothing Loading** >> >> - Because AOT-linked classes can refer to each other, using direct C++ >> pointers, all AOT-linked classes must be loaded together. Otherwise we will >> have dangling C++ pointers in the class metadata structures. >> - During a production run, we check during VM start-up for incompatible VM >> options that would prevent some of the AOT-linked classes from being loaded. >> For example: >> - If the VM is started with an JVMTI agent that has ClassFileLoadHook >> capabilities, it could replace some of the AOT-linked classes with >> alternative versions. >> - If the VM is started with certain module options, such as >> `--patch-module` or `--module`, some AOT-linked classes may be replaced with >> patched versions, or may become invisible and cannot be loaded into the JVM. >> - When incompatible VM options are detected, the JVM will refuse to load an >> AOT cache that has AOT-linked classes. See >> `FileMapInfo::validate_aot_class_linking()`. >> - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires >> `CDSConfig::is_using_full_module_graph()` to be true. This means that the >> exa... > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > @ashu-mehra reviews Marked as reviewed by asmehra (Committer). - PR Review: https://git.openjdk.org/jdk/pull/20843#pullrequestreview-2307949022
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v6]
On Mon, 16 Sep 2024 21:54:49 GMT, Ioi Lam wrote:
>> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading &
>> Linking](https://bugs.openjdk.org/browse/JDK-8315737).
>>
>> **Overview**
>>
>> - A new `-XX:+AOTClassLinking` flag is added. See [JEP
>> 498](https://bugs.openjdk.org/browse/JDK-8315737) and the
>> [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this
>> command-line option, its default value, and its impact on compatibility.
>> - When this flag is enabled during the creation of an AOT cache (aka CDS
>> archive), an `AOTLinkedClassTable` is added to the cache to include all
>> classes that are AOT-linked. For this PR, only classes for the
>> boot/platform/application loaders are eligible. The main logic is in
>> `aotClassLinker.cpp`.
>> - When an AOT archive is loaded in a production run, all classes in the
>> `AOTLinkedClassTable` are loaded into their respective class loaders at the
>> earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`.
>> - The boot classes are loaded as part of `vmClasses::resolve_all()`
>> - The platform/application classes are loaded after the module graph is
>> restored (see changes in `threads.cpp`).
>> - Since all classes in a `AOTLinkedClassTable` are loaded before any
>> user-code is executed, we can resolve constant pool entries that refer to
>> these classes during AOT cache creation. See changes in
>> `AOTConstantPoolResolver::is_class_resolution_deterministic()`.
>>
>> **All-or-nothing Loading**
>>
>> - Because AOT-linked classes can refer to each other, using direct C++
>> pointers, all AOT-linked classes must be loaded together. Otherwise we will
>> have dangling C++ pointers in the class metadata structures.
>> - During a production run, we check during VM start-up for incompatible VM
>> options that would prevent some of the AOT-linked classes from being loaded.
>> For example:
>> - If the VM is started with an JVMTI agent that has ClassFileLoadHook
>> capabilities, it could replace some of the AOT-linked classes with
>> alternative versions.
>> - If the VM is started with certain module options, such as
>> `--patch-module` or `--module`, some AOT-linked classes may be replaced with
>> patched versions, or may become invisible and cannot be loaded into the JVM.
>> - When incompatible VM options are detected, the JVM will refuse to load an
>> AOT cache that has AOT-linked classes. See
>> `FileMapInfo::validate_aot_class_linking()`.
>> - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires
>> `CDSConfig::is_using_full_module_graph()` to be true. This means that the
>> exa...
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> @ashu-mehra reviews
src/hotspot/share/runtime/threads.cpp line 322:
> 320: universe_post_module_init();
> 321:
> 322: if (CDSConfig::is_using_aot_linked_classes()) {
call_initPhase2 has a timer that computes cost for initializing module system.
Before this patch call_initPhase2 was only initializing the module system. But
now it is doing work which is not part of the module system initialization. So
probably in future we may want to refactor this work out of call_initPhase2.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1763851899
Re: RFR: 8331497: Implement JEP 483: Ahead-of-Time Class Loading & Linking [v7]
On Mon, 4 Nov 2024 03:15:25 GMT, Ioi Lam wrote: >> This is an implementation of [JEP 483: Ahead-of-Time Class Loading & >> Linking](https://openjdk.org/jeps/483). >> >> >> Note: this is a combined PR of the following individual PRs >> - https://github.com/openjdk/jdk/pull/20516 >> - https://github.com/openjdk/jdk/pull/20517 >> - https://github.com/openjdk/jdk/pull/20843 >> - https://github.com/openjdk/jdk/pull/20958 >> - https://github.com/openjdk/jdk/pull/20959 >> - https://github.com/openjdk/jdk/pull/21049 >> - https://github.com/openjdk/jdk/pull/21143 > > Ioi Lam has updated the pull request with a new target base due to a merge or > a rebase. The pull request now contains 172 commits: > > - fixed merge > - Merge branch 'master' into jep-483-candidate > - 8343493: Perform module checks during MetaspaceShared::map_archives() > - reverted changes in modules.cpp to make it easy to merge with mainline > - Fixed whitespace; fixed minimal build > - Fixed 8343245: AOT cache creation crashes with > "assert(HeapShared::is_archivable_hidden_klass(ik)) failed: sanity" > - fixed comments > - @ashu-mehra comment - renamed function to > SystemDictionaryShared::should_be_excluded(); added comments and asserts; > make sure the class is indeed checked > - Backed out 58233cc20fa22abfd711bb59aafb78e20fabc195 > - @ashu-mehra comment - rename/comment AOTClassLinker::add_new_candidate() > and added asserts for thread safety > - ... and 162 more: https://git.openjdk.org/jdk/compare/29882bfe...935dcc68 Marked as reviewed by asmehra (Committer). - PR Review: https://git.openjdk.org/jdk/pull/21642#pullrequestreview-2414263812
Re: RFR: 8331497: Implement JEP 483: Ahead-of-Time Class Loading & Linking [v4]
On Thu, 24 Oct 2024 03:01:54 GMT, Ioi Lam wrote: >> This is an implementation of [JEP 483: Ahead-of-Time Class Loading & >> Linking](https://openjdk.org/jeps/483). >> >> >> Note: this is a combined PR of the following individual PRs >> - https://github.com/openjdk/jdk/pull/20516 >> - https://github.com/openjdk/jdk/pull/20517 >> - https://github.com/openjdk/jdk/pull/20843 >> - https://github.com/openjdk/jdk/pull/20958 >> - https://github.com/openjdk/jdk/pull/20959 >> - https://github.com/openjdk/jdk/pull/21049 >> - https://github.com/openjdk/jdk/pull/21143 > > Ioi Lam has updated the pull request incrementally with two additional > commits since the last revision: > > - 8342907: Implement AOT testing mode for jtreg tests (authored by @katyapav) > - disable test that fails with hotspot_runtime_non_cds_mode @iklam I remember there was [ConstantPool::iterate_archivable_resolved_references](https://github.com/iklam/jdk/blob/f0bc1ae60fea80d5914d520457986a753e75fae4/src/hotspot/share/oops/constantPool.cpp#L288) in https://github.com/openjdk/jdk/pull/21143 but I don't see it any more in this PR. Can you please comment on why was that removed? - PR Comment: https://git.openjdk.org/jdk/pull/21642#issuecomment-2438079354
Re: RFR: 8331497: Implement JEP 483: Ahead-of-Time Class Loading & Linking [v4]
On Thu, 24 Oct 2024 03:01:54 GMT, Ioi Lam wrote:
>> This is an implementation of [JEP 483: Ahead-of-Time Class Loading &
>> Linking](https://openjdk.org/jeps/483).
>>
>>
>> Note: this is a combined PR of the following individual PRs
>> - https://github.com/openjdk/jdk/pull/20516
>> - https://github.com/openjdk/jdk/pull/20517
>> - https://github.com/openjdk/jdk/pull/20843
>> - https://github.com/openjdk/jdk/pull/20958
>> - https://github.com/openjdk/jdk/pull/20959
>> - https://github.com/openjdk/jdk/pull/21049
>> - https://github.com/openjdk/jdk/pull/21143
>
> Ioi Lam has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - 8342907: Implement AOT testing mode for jtreg tests (authored by @katyapav)
> - disable test that fails with hotspot_runtime_non_cds_mode
src/hotspot/share/cds/aotClassLinker.cpp line 117:
> 115:
> 116: void AOTClassLinker::add_candidate(InstanceKlass* ik) {
> 117: _candidates->put_when_absent(ik, true);
I just noticed the use of `put_when_absent` here. This means the caller should
ensure that `ik` has not already been added to the `_candidates`. We do that
currently (`try_add_candidate` calls `is_candidate` before calling
`add_candidate`) but probably worth mentioning this contract explicitly in a
comment somewhere for future readers.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21642#discussion_r1816850893
Re: RFR: 8329706: Implement -XX:+AOTClassLinking [v11]
On Thu, 19 Sep 2024 04:19:18 GMT, Ioi Lam wrote: >> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & >> Linking](https://bugs.openjdk.org/browse/JDK-8315737). >> >> **Overview** >> >> - A new `-XX:+AOTClassLinking` flag is added. See [JEP >> 498](https://bugs.openjdk.org/browse/JDK-8315737) and the >> [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this >> command-line option, its default value, and its impact on compatibility. >> - When this flag is enabled during the creation of an AOT cache (aka CDS >> archive), an `AOTLinkedClassTable` is added to the cache to include all >> classes that are AOT-linked. For this PR, only classes for the >> boot/platform/application loaders are eligible. The main logic is in >> `aotClassLinker.cpp`. >> - When an AOT archive is loaded in a production run, all classes in the >> `AOTLinkedClassTable` are loaded into their respective class loaders at the >> earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`. >> - The boot classes are loaded as part of `vmClasses::resolve_all()` >> - The platform/application classes are loaded after the module graph is >> restored (see changes in `threads.cpp`). >> - Since all classes in a `AOTLinkedClassTable` are loaded before any >> user-code is executed, we can resolve constant pool entries that refer to >> these classes during AOT cache creation. See changes in >> `AOTConstantPoolResolver::is_class_resolution_deterministic()`. >> >> **All-or-nothing Loading** >> >> - Because AOT-linked classes can refer to each other, using direct C++ >> pointers, all AOT-linked classes must be loaded together. Otherwise we will >> have dangling C++ pointers in the class metadata structures. >> - During a production run, we check during VM start-up for incompatible VM >> options that would prevent some of the AOT-linked classes from being loaded. >> For example: >> - If the VM is started with an JVMTI agent that has ClassFileLoadHook >> capabilities, it could replace some of the AOT-linked classes with >> alternative versions. >> - If the VM is started with certain module options, such as >> `--patch-module` or `--module`, some AOT-linked classes may be replaced with >> patched versions, or may become invisible and cannot be loaded into the JVM. >> - When incompatible VM options are detected, the JVM will refuse to load an >> AOT cache that has AOT-linked classes. See >> `FileMapInfo::validate_aot_class_linking()`. >> - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires >> `CDSConfig::is_using_full_module_graph()` to be true. This means that the >> exa... > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > @dholmes-ora comments LGTM - PR Comment: https://git.openjdk.org/jdk/pull/20843#issuecomment-2361038557
Re: RFR: 8331497: Implement JEP 483: Ahead-of-Time Class Loading & Linking [v4]
On Thu, 24 Oct 2024 03:01:54 GMT, Ioi Lam wrote:
>> This is an implementation of [JEP 483: Ahead-of-Time Class Loading &
>> Linking](https://openjdk.org/jeps/483).
>>
>>
>> Note: this is a combined PR of the following individual PRs
>> - https://github.com/openjdk/jdk/pull/20516
>> - https://github.com/openjdk/jdk/pull/20517
>> - https://github.com/openjdk/jdk/pull/20843
>> - https://github.com/openjdk/jdk/pull/20958
>> - https://github.com/openjdk/jdk/pull/20959
>> - https://github.com/openjdk/jdk/pull/21049
>> - https://github.com/openjdk/jdk/pull/21143
>
> Ioi Lam has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - 8342907: Implement AOT testing mode for jtreg tests (authored by @katyapav)
> - disable test that fails with hotspot_runtime_non_cds_mode
src/hotspot/share/cds/aotConstantPoolResolver.cpp line 113:
> 111:
> 112: if (CDSConfig::is_dumping_aot_linked_classes()) {
> 113: // Need to call try_add_candidate instead of is_candidate, as this
> may be called
I think in this version of the code this method is not used before
`AOTClassLinker::add_candidates`. Can we switch back to `is_candidate` then?
src/hotspot/share/cds/heapShared.hpp line 298:
> 296: static SeenObjectsTable *_seen_objects_table;
> 297:
> 298: // The "special subgraph" contains all the all archived objects that
> are reachable
an extra `all` in the comment
src/hotspot/share/classfile/systemDictionaryShared.cpp line 685:
> 683: InstanceKlass* ik = InstanceKlass::cast(k);
> 684:
> 685: if (SafepointSynchronize::is_at_safepoint()) {
Why is this piece of block required?
It calls `is_excluded_class` which reads `DumpTimeClassInfo::_excluded` without
checking for `has_checked_exclusion`. That means it can return false (the
default value) even for classes that may later be marked for exclusion by
`check_for_exclusion(ik, p)`.
On the same note, I think we should add an assert in
`DumpTimeClassInfo::is_excluded` that `has_checked_exclusion()` is true.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21642#discussion_r1816853423
PR Review Comment: https://git.openjdk.org/jdk/pull/21642#discussion_r1816996074
PR Review Comment: https://git.openjdk.org/jdk/pull/21642#discussion_r1817159936
Re: RFR: 8331497: Implement JEP 483: Ahead-of-Time Class Loading & Linking [v4]
On Fri, 25 Oct 2024 18:08:17 GMT, Ashutosh Mehra wrote:
>> Ioi Lam has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - 8342907: Implement AOT testing mode for jtreg tests (authored by
>> @katyapav)
>> - disable test that fails with hotspot_runtime_non_cds_mode
>
> src/hotspot/share/classfile/systemDictionaryShared.cpp line 685:
>
>> 683: InstanceKlass* ik = InstanceKlass::cast(k);
>> 684:
>> 685: if (SafepointSynchronize::is_at_safepoint()) {
>
> Why is this piece of block required?
> It calls `is_excluded_class` which reads `DumpTimeClassInfo::_excluded`
> without checking for `has_checked_exclusion`. That means it can return false
> (the default value) even for classes that may later be marked for exclusion
> by `check_for_exclusion(ik, p)`.
> On the same note, I think we should add an assert in
> `DumpTimeClassInfo::is_excluded` that `has_checked_exclusion()` is true.
Does the check `SafepointSynchronize::is_at_safepoint` imply that exclusion
checks for all classes have already been done?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21642#discussion_r1817167862
Re: RFR: 8280682: Refactor AOT code source validation checks
On Wed, 5 Feb 2025 22:32:58 GMT, Calvin Cheung wrote:
> This changset refactors CDS class paths and module paths validation code into
> a new class `AOTCodeSource` and related class `AOTCodeSourceConfig`. Code has
> been moved from filemap.[c|h]pp, classLoader.[c|h]pp, and
> classLoaderExt.[c|h]pp to aotCodeSource.[c|h]pp. CDS dependencies have been
> removed from `classLoader.cpp`. More refactoring could be done, such as
> removing `classLoaderExt.cpp`, in a future RFE.
>
> Passed tiers 1 - 5 testing.
src/hotspot/share/cds/aotCodeSource.cpp line 762:
> 760: }
> 761:
> 762: if (is_boot_classpath && runtime_css.has_next() &&
> (need_to_check_app_classpath() || num_module_paths() > 0)) {
I am not sure I get what this block is for. Is it for the case where runtime
boot cp has more entries than the dumptime boot cp, and it is checking if the
extra entries really exist or they are just empty? If so, then
`check_paths_existence` should only be checking the extra entries in the boot
cp, not all of them.
Can you please explain this and probably add a comment as well to describe what
this block is for.
src/hotspot/share/cds/aotCodeSource.cpp line 894:
> 892: // matched exactly.
> 893: bool AOTCodeSourceConfig::need_lcp_match(AllCodeSourceStreams& all_css)
> const {
> 894: if (!need_lcp_match_helper(boot_start(), boot_end(),
> all_css.boot_cp()) ||
Can we reverse these conditions to make it easier to read?
if (need_lcp_match_helper(boot_start(), boot_end(), all_css.boot_cp()) &&
need_lcp_match_helper(app_start(), app_end(), all_css.app_cp())) {
return true;
} else {
return false;
}
src/hotspot/share/cds/aotCodeSource.cpp line 903:
> 901:
> 902: bool AOTCodeSourceConfig::need_lcp_match_helper(int start, int end,
> CodeSourceStream& css) const {
> 903: if (app_end() == boot_start()) {
I feel this block belongs to the caller `need_lcp_match`.
src/hotspot/share/cds/aotCodeSource.hpp line 213:
> 211:
> 212: // Common accessors
> 213: int boot_start() const { return 1; }
Can we rename these methods to something like boot_start() ->
boot_cp_start_index().
At the call site it makes it clear it is referring to the bootclasspath index,
and not booting something :)
src/hotspot/share/cds/aotCodeSource.hpp line 234:
> 232: // Functions used only during dumptime
> 233: static void dumptime_init(TRAPS);
> 234: static size_t estimate_size_for_archive() {
This method doesn't seem to be in use. Can this be removed?
src/hotspot/share/cds/filemap.cpp line 318:
> 316: if (header()->has_full_module_graph() && has_extra_module_paths) {
> 317: CDSConfig::stop_using_optimized_module_handling();
> 318: log_info(cds)("optimized module handling: disabled because of extra
> module path(s) are specified");
typo: "disabled because ~of~ extra module path(s) are specified"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953766439
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953732835
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953722869
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953489002
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953383815
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1953768345
Re: RFR: 8353597: Refactor handling VM options for AOT cache input and output [v4]
On Wed, 9 Apr 2025 02:18:41 GMT, Ioi Lam wrote: >> Since [JEP 483: Ahead-of-Time Class Loading & >> Linking](https://openjdk.org/jeps/483), VM options such as `-XX:AOTCache >> `are implemented as aliases of "classical" CDS options such as >> `-XX:SharedArchiveFile`. >> >> In anticipation of the [JEP: Ahead-of-time Command Line >> Ergonomics](https://bugs.openjdk.org/browse/JDK-8350022), we should refactor >> the code that deals with the AOT options. Specifically, as we expect the JVM >> to be able to load from an "input AOT cache" and write to an "output AOT >> cache", we should clearly identify the input and output caches in separate >> APIs: >> >> >> const char* CDSConfig::input_static_archive_path(); >> const char* CDSConfig::input_dynamic_archive_path(); >> const char* CDSConfig::output_archive_path(); >> >> >> This PR also cleans up the code by: >> - renaming a few function to reflect what they actually do >> - moving more "config" management code into cdsConfig.cpp >> >> There's also a behavioral bug fix: before this PR, `-XX:AOTCache` was >> handled by the `ergo_init_classic_archive_paths()` function, which allows >> two files to be specified. E.g., `java -XX:AOTCache=static.jsa:dynamic.jsa`. >> That's because `-XX:AOTCache` was implemented as an alias of >> `-XX:SharedArchiveFile`, and the latter allows this usage. >> >> However, this behavior is not specified in JEP 483. Allowing two files in >> -XX:AOTCache will cause unnecessary complexity when we implement >> [JDK-8353598: Allow AOT cache to be used in training >> run](https://bugs.openjdk.org/browse/JDK-8353598). Therefore, I added new >> test cases to disallow the use of two files. This also means that we don't >> need to modify the already over-complicated >> `ergo_init_classic_archive_paths()` for the AOT use cases > > Ioi Lam has updated the pull request with a new target base due to a merge or > a rebase. The pull request now contains nine commits: > > - Merge branch 'master' into 8353597-refactor-aot-cache-input-output > - @lmesnik comments > - more clean up > - Minimized changes in ergo_init_classic_archive_paths() > - Clean up CDS input/output path handling > - Refactored CollectClassesForLinking for simplification > - Merge branch 'master' into 8353014-exclude-tooling-classes-from-aot-cache > - Reverted some fixes in systemDictionaryShared.cpp that causes test failures > - 8353014: Exclude AOT tooling classes from AOT cache lgtm - Marked as reviewed by asmehra (Committer). PR Review: https://git.openjdk.org/jdk/pull/24401#pullrequestreview-2751975775
Re: RFR: 8280682: Refactor AOT code source validation checks [v2]
On Fri, 14 Feb 2025 19:21:32 GMT, Calvin Cheung wrote: >> This changset refactors CDS class paths and module paths validation code >> into a new class `AOTCodeSource` and related class `AOTCodeSourceConfig`. >> Code has been moved from filemap.[c|h]pp, classLoader.[c|h]pp, and >> classLoaderExt.[c|h]pp to aotCodeSource.[c|h]pp. CDS dependencies have been >> removed from `classLoader.cpp`. More refactoring could be done, such as >> removing `classLoaderExt.cpp`, in a future RFE. >> >> Passed tiers 1 - 5 testing. > > Calvin Cheung has updated the pull request incrementally with one additional > commit since the last revision: > > @iklam and @ashu-mehra comment Marked as reviewed by asmehra (Committer). Just one more comment, rest looks good. src/hotspot/share/runtime/threads.cpp line 27: > 25: */ > 26: > 27: #include "cds/aotCodeSource.hpp" Why is this include needed? - PR Review: https://git.openjdk.org/jdk/pull/23476#pullrequestreview-2622461930 PR Comment: https://git.openjdk.org/jdk/pull/23476#issuecomment-2664635612 PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959077605
Re: RFR: 8344009: Improve compiler memory statistics [v4]
On Thu, 20 Feb 2025 13:14:34 GMT, Thomas Stuefe wrote:
>> Greetings,
>>
>> This is a rewrite of the Compiler Memory Statistic. The primary new feature
>> is the capability to track allocations by C2 phases. This will allow for a
>> much faster, more thorough analysis of footprint issues.
>>
>> Tracking Arena memory movement is not trivial since one needs to follow the
>> ebb and flow of allocations over nested C2 phases. A phase typically
>> allocates more than it releases, accruing new nodes and resource area. A
>> phase can also release more than allocated when Arenas carried over from
>> other phases go out of scope in this phase. Finally, it can have high
>> temporary peaks that vanish before the phase ends.
>>
>> I wanted to track that information correctly and display it clearly in a way
>> that is easy to understand.
>>
>> The patch implements per-phase tracking by instrumenting the `TracePhase`
>> stack object (thanks to @rwestrel for this idea).
>>
>> The nice thing with this technique is that it also allows for quick analysis
>> of a suspected hot spot (eg, the inside of a loop): drop a TracePhase in
>> there with a speaking name, and you can see the allocations inside that
>> phase.
>>
>> The statistic gives us two new forms of output:
>>
>> 1) At the moment the compilation memory *peaked*, we now get a detailed
>> breakdown of that peak usage per phase:
>>
>>
>> Arena Usage by Arena Type and compilation phase, at arena usage peak of
>> 58817816:
>> Phase Totalra node comp
>> type index reglive regsplit cienv other
>> none1205512155104982984 33712
>> 0 0 0 0 0 33712
>> parse 11685376720016 6578728 1899064
>> 0 0 0 0 1832888654680
>> optimizer916584 0556416 0
>> 0 0 0 0 0360168
>> escapeAnalysis 1983400 0 1276392707008
>> 0 0 0 0 0 0
>> connectionGraph 720016 0 0621832
>> 0 0 0 0 98184 0
>> macroEliminate 196448 0196448 0
>> 0 0 0 0 0 0
>> iterGVN 327440 0196368131072
>> 0 0 0 0 0 0
>> incrementalInline 3992816 0 304370462...
>
> Thomas Stuefe has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> avoid Thread::current in high traffic chunk alloc path
src/hotspot/share/compiler/compilationMemStatInternals.hpp line 92:
> 90:
> 91: // A very simple fixed-width FIFO buffer, used for the phase timeline
> 92: template
Would `size` be a better name than `max`?
src/hotspot/share/compiler/compilationMemStatInternals.hpp line 160:
> 158: void init(T v){ start = cur = peak = v; }
> 159: void update(T v) { cur = v; if (v > peak) peak = v; }
> 160: dT end_delta() const { return (dT)cur - (dT)start; }
Should it be `return (dT)(cur - start); }`
src/hotspot/share/compiler/compilationMemStatInternals.hpp line 161:
> 159: void update(T v) { cur = v; if (v > peak) peak = v; }
> 160: dT end_delta() const { return (dT)cur - (dT)start; }
> 161: size_t temporary_peak_size() const { return MIN2(peak - cur, peak -
> start); }
shouldn't it be `MAX2(peak - cur, peak - start)`? Why not just `peak - start`?
src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 149:
> 147: int col = start_indent;
> 148: check_phase_trace_id(e.info.id);
> 149: if (omit_empty_phases && e._bytes.end_delta() == 0 &&
> e._bytes.temporary_peak_size() == 0) {
`omit_empty_phases` is always false. Can it be just removed?
src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 205:
> 203: // seed current entry
> 204: Entry& e = _fifo.current();
> 205: e._bytes.start = e._bytes.cur = e._bytes.peak = cur_abs;
This can be replaced by `e._bytes.init(cur_abs)`.
Same for the next statement.
On same lines I would suggest to add `Entry::init()` and call it here.
src/hotspot/share/memory/arena.hpp line 48:
> 46: const size_t _len; // Size of this Chunk
> 47: // Used for Compilation Memory Statistic
> 48: uint64_t _stamp;
This is wasted space if compilation memory stats is not enabled. One way to
avoid this is to subclass `Chunk` as a `StampedChunk` and use that if
compilation memory stats is enabled. Is this complexity worth the space saving?
s
Re: RFR: 8344009: Improve compiler memory statistics [v4]
On Tue, 25 Feb 2025 16:13:48 GMT, Thomas Stuefe wrote:
>> src/hotspot/share/compiler/compilationMemStatInternals.hpp line 160:
>>
>>> 158: void init(T v){ start = cur = peak = v; }
>>> 159: void update(T v) { cur = v; if (v > peak) peak = v; }
>>> 160: dT end_delta() const { return (dT)cur - (dT)start; }
>>
>> Should it be `return (dT)(cur - start); }`
>
> hmm, I like to avoid the inner overflow is cur < start (if the phase released
> more memory than it allocated)
right, I missed that.
>> src/hotspot/share/compiler/compilationMemStatInternals.hpp line 161:
>>
>>> 159: void update(T v) { cur = v; if (v > peak) peak = v; }
>>> 160: dT end_delta() const { return (dT)cur - (dT)start; }
>>> 161: size_t temporary_peak_size() const { return MIN2(peak - cur, peak
>>> - start); }
>>
>> shouldn't it be `MAX2(peak - cur, peak - start)`? Why not just `peak -
>> start`?
>
> We are only interested in a rise that rose significantly above **both** the
> start and end point of the measurements.
>
> E.g.:
> - if we have this: start = 0, end = 20MB, peak = 20MB, this is not a
> temporary peak and we already know that the end usage is 20MB.
> - if we have this: start = 20MB, end = 0, peak = 20MB, this is not a
> temporary peak either, because we already know the starting footprint was
> 20MB.
> - but if we have start = 0, end = 0, peak = 20MB, this is interesting since
> if we just print start and end we will miss the fact that in between those
> times we had temporarily allocated 20MB.
Thanks for the explanation. It would be great if some comment can be added,
possibly along with some example like the one in the previous comment, to
explain the meaning of `temporary_peak_size` and the corresponding calculation.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1970880396
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1970880439
Re: RFR: 8344009: Improve compiler memory statistics [v6]
On Tue, 25 Feb 2025 16:43:21 GMT, Thomas Stuefe wrote: >> Greetings, >> >> This is a rewrite of the Compiler Memory Statistic. The primary new feature >> is the capability to track allocations by C2 phases. This will allow for a >> much faster, more thorough analysis of footprint issues. >> >> Tracking Arena memory movement is not trivial since one needs to follow the >> ebb and flow of allocations over nested C2 phases. A phase typically >> allocates more than it releases, accruing new nodes and resource area. A >> phase can also release more than allocated when Arenas carried over from >> other phases go out of scope in this phase. Finally, it can have high >> temporary peaks that vanish before the phase ends. >> >> I wanted to track that information correctly and display it clearly in a way >> that is easy to understand. >> >> The patch implements per-phase tracking by instrumenting the `TracePhase` >> stack object (thanks to @rwestrel for this idea). >> >> The nice thing with this technique is that it also allows for quick analysis >> of a suspected hot spot (eg, the inside of a loop): drop a TracePhase in >> there with a speaking name, and you can see the allocations inside that >> phase. >> >> The statistic gives us two new forms of output: >> >> 1) At the moment the compilation memory *peaked*, we now get a detailed >> breakdown of that peak usage per phase: >> >> >> Arena Usage by Arena Type and compilation phase, at arena usage peak of >> 58817816: >> Phase Totalra node comp >> type index reglive regsplit cienv other >> none1205512155104982984 33712 >> 0 0 0 0 0 33712 >> parse 11685376720016 6578728 1899064 >> 0 0 0 0 1832888654680 >> optimizer916584 0556416 0 >> 0 0 0 0 0360168 >> escapeAnalysis 1983400 0 1276392707008 >> 0 0 0 0 0 0 >> connectionGraph 720016 0 0621832 >> 0 0 0 0 98184 0 >> macroEliminate 196448 0196448 0 >> 0 0 0 0 0 0 >> iterGVN 327440 0196368131072 >> 0 0 0 0 0 0 >> incrementalInline 3992816 0 304370462... > > Thomas Stuefe has updated the pull request incrementally with five additional > commits since the last revision: > > - feedback ashu > - feedback roberto > - final-statistics-switch > - performance fix > - remove test code Marked as reviewed by asmehra (Committer). - PR Review: https://git.openjdk.org/jdk/pull/23530#pullrequestreview-2643027285
Re: RFR: 8344009: Improve compiler memory statistics
On Tue, 25 Feb 2025 16:39:14 GMT, Thomas Stuefe wrote: >>> > @robcasloz I identified and hopefully fixed a small issue that hit the >>> > "disabled" path. Turns out we allocate arena chunks a lot more frequently >>> > than I thought, and the new unconditional call to Thread::current() in >>> > there was hurting a bit. I now avoid this unless I know the statistic is >>> > enabled. >>> > With this patch, on my machine the difference between unpatched and >>> > patched JVM with stats disabled is below one standard deviation for the >>> > benchmark in question. >>> >>> Great, thanks! Will re-run benchmarking and report results early next week. >> >> Functional test results (Oracle tier1-5) still look good for the latest >> commit (dd7a06ad). I can confirm that the C2 speed regression on our >> linux-x64 machines is almost fully mitigated. The 2-3% regression on our >> macosx-aarch64 machines does not seem to be addressed by the latest changes >> though, but as I mentioned before I think it is in the acceptable range (and >> only affects one benchmark). > > @robcasloz, @ashu-mehra thanks a lot for your reviews. I incorporated most of > them into the PR. Changes look good to me. Thanks @tstuefe for addressing the comments. - PR Comment: https://git.openjdk.org/jdk/pull/23530#issuecomment-2683863222
Re: RFR: 8356693: AOT assembly phase fails with -javaagent
On Sun, 11 May 2025 04:57:44 GMT, Ioi Lam wrote:
> https://openjdk.org/jeps/483 mentions:
>
>> To enjoy the benefits of the AOT cache generated during a training run, the
>> training run and all subsequent runs must be essentially similar. [...] All
>> runs must not use JVMTI agents that can arbitrarily rewrite classfiles
>> using ClassFileLoadHook.
>
> However, when *any* java agent is specified in the training run, the JVM
> fails at start-up. E.g.,
>
>
> $ java -XX:AOTMode=record -javaagent:agent.jar -cp app.jar App
> Error occurred during CDS dumping
> Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS
> dumping
>
>
> With the AOT cache, the main concern for JVMTI agents is that they can modify
> the contents of loaded Java classes. If we store such modified classes into
> the AOT cache, their contents will no longer match the original class files
> (from application JAR files, etc). As a result, when using the AOT cache in
> production runs, the application may have unexpected behavior.
>
> With this PR, we allow JVMTI agents in the AOT workflow. To address the above
> concern, we ensure that JVMTI agents cannot affect the contents of AOT cache:
>
> - In training runs (`java -XX:AOTMode=record`), JVMTI agents are allowed,
> but the AOT configuration file should filter out classes that are transformed
> by the agents. This can be checking `InstanceKlass::has_been_redefined()` and
> `ClassFileStream::from_class_file_load_hook()`.
>
> - In the assembly phase (`java -XX:AOTMode=record`), agents can be specified
> in the command-line. However, since the assembly phase doesn't execute any
> application logic, we will also not load any of the specified agents.
> Therefore, the agents cannot affect the contents of the AOT cache created in
> the assembly phase.
src/hotspot/share/prims/jvmtiAgentList.cpp line 79:
> 77: JvmtiAgent* next = head(list);
> 78: #if INCLUDE_CDS
> 79: if (_disable_agent_list) {
Instead of intercepting here would it be better if we can clear
`JvmtiAgentList::_list` in `CDSConfig::check_aotmode_create()`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25170#discussion_r2087603909
Re: RFR: 8356693: AOT assembly phase fails with -javaagent
On Tue, 13 May 2025 20:59:48 GMT, Ioi Lam wrote: > That's what I had in an earlier version, but then there's no code to free the > allocated entries, and I am too lazy to write the deallocator. > > Either way the effect is the same. The trivial amount of memory used by the > entries are not reclaimed, but the VM will exit after a finite amount of time > anyway, after the cache creation is finish. > It looks like the memory for these entries is never freed even after unloading the agents during shutdown. So there memory is never reclaimed. And setting `JvmtiAgentList::_list` to nullptr is not really changing that behavior. > I thought intercepting it here is a bit cleaner and avoids the awkward > question of freeing the entries. hmmm...not sure about that. Ideally we should ignore the jvmti options that add agents if the JVM is in assembly phase. But we detect the assembly phase after jvmti options have been processed. IMO clearing the _list is the next best approach. If that doesn't work then more intrusive fix like intercepting the `JvmtiAgentList::Iterator`. - PR Review Comment: https://git.openjdk.org/jdk/pull/25170#discussion_r2087914075
Re: RFR: 8356693: AOT assembly phase fails with -javaagent [v2]
On Wed, 14 May 2025 21:03:35 GMT, Ioi Lam wrote: >> https://openjdk.org/jeps/483 mentions: >> >>> To enjoy the benefits of the AOT cache generated during a training run, the >>> training run and all subsequent runs must be essentially similar. [...] All >>> runs must not use JVMTI agents that can arbitrarily rewrite classfiles >>> using ClassFileLoadHook. >> >> However, when *any* java agent is specified in the training run, the JVM >> fails at start-up. E.g., >> >> >> $ java -XX:AOTMode=record -javaagent:agent.jar -cp app.jar App >> Error occurred during CDS dumping >> Must enable AllowArchivingWithJavaAgent in order to run Java agent during >> CDS dumping >> >> >> With the AOT cache, the main concern for JVMTI agents is that they can >> modify the contents of loaded Java classes. If we store such modified >> classes into the AOT cache, their contents will no longer match the original >> class files (from application JAR files, etc). As a result, when using the >> AOT cache in production runs, the application may have unexpected behavior. >> >> With this PR, we allow JVMTI agents in the AOT workflow. To address the >> above concern, we ensure that JVMTI agents cannot affect the contents of AOT >> cache: >> >> - In training runs (`java -XX:AOTMode=record`), JVMTI agents are allowed, >> but the AOT configuration file should filter out classes that are >> transformed by the agents. This can be checking >> `InstanceKlass::has_been_redefined()` and >> `ClassFileStream::from_class_file_load_hook()`. >> >> - In the assembly phase (`java -XX:AOTMode=record`), agents can be specified >> in the command-line. However, since the assembly phase doesn't execute any >> application logic, we will also not load any of the specified agents. >> Therefore, the agents cannot affect the contents of the AOT cache created in >> the assembly phase. > > Ioi Lam 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 five additional commits since the > last revision: > > - Merge branch 'master' into 8356693-aot-assembly-phase-fails-with-java-agent > - @ashu-mehra comments > - fixed whitespace > - 8356693: AOT assembly phase fails with -javaagent > - step1 Marked as reviewed by asmehra (Committer). - PR Review: https://git.openjdk.org/jdk/pull/25170#pullrequestreview-2841532608
Re: RFR: 8356693: AOT assembly phase fails with -javaagent [v2]
On Wed, 14 May 2025 16:05:18 GMT, Ioi Lam wrote: >>> That's what I had in an earlier version, but then there's no code to free >>> the allocated entries, and I am too lazy to write the deallocator. >>> >>> Either way the effect is the same. The trivial amount of memory used by the >>> entries are not reclaimed, but the VM will exit after a finite amount of >>> time anyway, after the cache creation is finish. >>> >> >> It looks like the memory for these entries is never freed even after >> unloading the agents during shutdown. So there memory is never reclaimed. >> And setting `JvmtiAgentList::_list` to nullptr is not really changing that >> behavior. >> >>> I thought intercepting it here is a bit cleaner and avoids the awkward >>> question of freeing the entries. >> >> hmmm...not sure about that. Ideally we should ignore the jvmti options that >> add agents if the JVM is in assembly phase. But we detect the assembly phase >> after jvmti options have been processed. >> IMO clearing the _list is the next best approach. If that doesn't work then >> more intrusive fix like intercepting the `JvmtiAgentList::Iterator`. > > I cleared the `_list` as you suggested. @iklam thank you for addressing my comments. - PR Review Comment: https://git.openjdk.org/jdk/pull/25170#discussion_r2089762404
