Integrated: JDK-8322042: HeapDumper should perform merge on the current thread instead of VMThread
On Tue, 2 Apr 2024 00:40:37 GMT, Alex Menkov wrote: > The fix updated HeapDumper to always perform merge on the current thread. > > Testing: tier1-5, all HeapDump-related tests > Covered heap dumping scenarios: > - `jcmd GC.heap_dump` command; > - `HotSpotDiagnosticMXBean.dumpHeap()`; > - `HeapDumpBeforeFullGC`, `HeapDumpAfterFullGC` VM options; > - `HeapDumpOnOutOfMemoryError` VM option. This pull request has now been integrated. Changeset: 12ad09a9 Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/12ad09a966030e7ed0900e205df4b412cf0b6a32 Stats: 26 lines in 2 files changed: 0 ins; 22 del; 4 mod 8322042: HeapDumper should perform merge on the current thread instead of VMThread Reviewed-by: sspitsyn, kevinw - PR: https://git.openjdk.org/jdk/pull/18571
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v13]
On Thu, 4 Apr 2024 12:46:26 GMT, Kevin Walls wrote:
>> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object
>> information.
>>
>> Not recommended for live production use. Requires UnlockDiagnosticVMOptions
>> and not included in jcmd help output, to remind us this is not a
>> general-purpose customer-facing tool.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> VMInspectTest update
The fix looks good to me. I've posted a couple of nits. Do I understand it
right that introducing an unlocking option is going to be addressed separately?
test/hotspot/jtreg/serviceability/dcmd/vm/VMInspectTest.java line 189:
> 187: // so make a few attempts.
> 188: BigInteger ptr = null;
> 189: for (int i=0; i < OBJECT_TRIES; i++) {
Nit: Need spaces around `=`.
test/hotspot/jtreg/serviceability/dcmd/vm/VMInspectTest.java line 224:
> 222: expected = "is a good oop";
> 223: }
> 224: }
Nit: The lines 219-223 can be simplified:
`expected = isGenZGC ? "is a zaddress" : "is a good oop";
-
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17655#pullrequestreview-1981143486
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1552427826
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1552434356
Integrated: 8329332: Remove CompiledMethod and CodeBlobLayout classes
On Fri, 29 Mar 2024 19:35:45 GMT, Vladimir Kozlov wrote: > Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE > [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) > which was used for AOT [JEP 295](https://openjdk.org/jeps/295) > implementation in JDK 9. The code was left in HotSpot assuming it will help > in a future. But during work on Leyden we decided to not use it. In Leyden > cached compiled code will be restored in CodeCache as normal nmethods: no > need to change VM's runtime and GC code to process them. > > I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce > header size in separate changes. In these changes I did simple fields > reordering to keep small (1 byte) fields together. > > I do not see (and not expected) performance difference with these changes. > > Tested tier1-5, xcomp, stress. Running performance testing. > > I need help with testing on platforms which Oracle does not support. This pull request has now been integrated. Changeset: 83eba863 Author:Vladimir Kozlov URL: https://git.openjdk.org/jdk/commit/83eba863fec5ee7e30c4f9b11122ad1deed3d2ec Stats: 3941 lines in 119 files changed: 1287 ins; 1753 del; 901 mod 8329332: Remove CompiledMethod and CodeBlobLayout classes Reviewed-by: vlivanov, stefank - PR: https://git.openjdk.org/jdk/pull/18554
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v4]
On Thu, 4 Apr 2024 17:31:53 GMT, Stefan Karlsson wrote: >> Vladimir Kozlov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address next round of comments > > Looks good. Thank you, @stefank, @iwanowww, @dean-long and @xmas92 for reviews and @RealFYang and @offamitkumar for testing. - PR Comment: https://git.openjdk.org/jdk/pull/18554#issuecomment-2038043339
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v4]
On Thu, 4 Apr 2024 17:04:30 GMT, Vladimir Kozlov wrote: >> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE >> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) >> which was used for AOT [JEP 295](https://openjdk.org/jeps/295) >> implementation in JDK 9. The code was left in HotSpot assuming it will help >> in a future. But during work on Leyden we decided to not use it. In Leyden >> cached compiled code will be restored in CodeCache as normal nmethods: no >> need to change VM's runtime and GC code to process them. >> >> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce >> header size in separate changes. In these changes I did simple fields >> reordering to keep small (1 byte) fields together. >> >> I do not see (and not expected) performance difference with these changes. >> >> Tested tier1-5, xcomp, stress. Running performance testing. >> >> I need help with testing on platforms which Oracle does not support. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Address next round of comments Looks good. - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18554#pullrequestreview-1980693606
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v4]
> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE > [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) > which was used for AOT [JEP 295](https://openjdk.org/jeps/295) > implementation in JDK 9. The code was left in HotSpot assuming it will help > in a future. But during work on Leyden we decided to not use it. In Leyden > cached compiled code will be restored in CodeCache as normal nmethods: no > need to change VM's runtime and GC code to process them. > > I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce > header size in separate changes. In these changes I did simple fields > reordering to keep small (1 byte) fields together. > > I do not see (and not expected) performance difference with these changes. > > Tested tier1-5, xcomp, stress. Running performance testing. > > I need help with testing on platforms which Oracle does not support. Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision: Address next round of comments - Changes: - all: https://git.openjdk.org/jdk/pull/18554/files - new: https://git.openjdk.org/jdk/pull/18554/files/33768fb2..0c18ff17 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18554=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18554=02-03 Stats: 80 lines in 13 files changed: 0 ins; 0 del; 80 mod Patch: https://git.openjdk.org/jdk/pull/18554.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18554/head:pull/18554 PR: https://git.openjdk.org/jdk/pull/18554
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 07:54:16 GMT, Stefan Karlsson wrote: >> Vladimir Kozlov 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: >> >> - Address comments >> - Merge branch 'master' into 8329332 >> - Removed not_used state of nmethod >> - remove trailing whitespace >> - 8329332: Remove CompiledMethod and CodeBlobLayout classes > > I took a second pass over the changes. I've given a few suggestions below. > None of them should require respinning of tests (except for making sure that > this still builds). Thank you, @stefank , for second round of review. I addressed all your comments. I also removed `virtual` from `virtual method() override;` as you suggested before since I touched these files again. I will wait result of new GHA and tier1 in mach5 before integration. - PR Comment: https://git.openjdk.org/jdk/pull/18554#issuecomment-2037731476
Integrated: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature
On Tue, 27 Feb 2024 10:44:20 GMT, Kevin Walls wrote: > The deprecated Subject Delegation feature in JMX will be removed. > > This was marked in JDK 21 as deprecated for removal (JDK-8298966). This pull request has now been integrated. Changeset: 6382a129 Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/6382a1290fbd7cc8fd097a2972bfcfc06fa4de79 Stats: 2026 lines in 35 files changed: 214 ins; 1632 del; 180 mod 832: Remove the Java Management Extension (JMX) Subject Delegation feature Reviewed-by: mchung, dfuchs - PR: https://git.openjdk.org/jdk/pull/18025
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 16:03:12 GMT, Stefan Karlsson wrote: >> Quote: "an" goes before words that begin with vowels. > > I don't think that holds if the 'n' is pronounced the way nmethod is > pronounced. `grep` shows that we have both cases but `an nmethod` is used more. - PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1552024067
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 16:16:41 GMT, Vladimir Kozlov wrote: >> I don't think that holds if the 'n' is pronounced the way nmethod is >> pronounced. > > `grep` shows that we have both cases but `an nmethod` is used more. I will fix it here as you suggested but I am not touching other places. - PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1552025626
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 07:51:47 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov 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:
>>
>> - Address comments
>> - Merge branch 'master' into 8329332
>> - Removed not_used state of nmethod
>> - remove trailing whitespace
>> - 8329332: Remove CompiledMethod and CodeBlobLayout classes
>
> src/hotspot/share/code/codeBlob.hpp line 168:
>
>> 166: bool is_vtable_blob() const { return _kind ==
>> CodeBlobKind::Blob_Vtable; }
>> 167: bool is_method_handles_adapter_blob() const { return _kind ==
>> CodeBlobKind::Blob_MH_Adapter; }
>> 168: bool is_upcall_stub() const { return _kind ==
>> CodeBlobKind::Blob_Upcall; }
>
> The `Blob_` prefix is now redundant since we always have to prefix with
> CodeBlobKind::. Just a suggestion if you want to shorten these.
Good suggestion
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1552009581
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 15:56:34 GMT, Vladimir Kozlov wrote: >> src/hotspot/share/gc/shared/gcBehaviours.hpp line 31: >> >>> 29: #include "oops/oopsHierarchy.hpp" >>> 30: >>> 31: // This is the behaviour for checking if a nmethod is unloading >> >> Maybe this should be *an* nmethod? > > Quote: "an" goes before words that begin with vowels. I don't think that holds if the 'n' is pronounced the way nmethod is pronounced. - PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1552005187
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 07:31:24 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov 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:
>>
>> - Address comments
>> - Merge branch 'master' into 8329332
>> - Removed not_used state of nmethod
>> - remove trailing whitespace
>> - 8329332: Remove CompiledMethod and CodeBlobLayout classes
>
> src/hotspot/share/gc/x/xUnload.cpp line 78:
>
>> 76: class XIsUnloadingBehaviour : public IsUnloadingBehaviour {
>> 77: public:
>> 78: virtual bool has_dead_oop(nmethod* const nm) const {
>
> `nmethod* const nm` => `nmethod* nm`. (ZGC's style is to use const for local
> variables, but not for variables in the parameter list). The same applies to
> the rest of the changes to this file.
Okay. I did not know that it is only used for locals. I will update code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551997411
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 07:26:21 GMT, Stefan Karlsson wrote: >> Vladimir Kozlov 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: >> >> - Address comments >> - Merge branch 'master' into 8329332 >> - Removed not_used state of nmethod >> - remove trailing whitespace >> - 8329332: Remove CompiledMethod and CodeBlobLayout classes > > src/hotspot/share/gc/shared/gcBehaviours.hpp line 31: > >> 29: #include "oops/oopsHierarchy.hpp" >> 30: >> 31: // This is the behaviour for checking if a nmethod is unloading > > Maybe this should be *an* nmethod? Quote: "an" goes before words that begin with vowels. - PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551989120
RFR: 8329674: JvmtiEnvThreadState::reset_current_location function should use JvmtiHandshake
The internal JVM TI JvmtiHandshake and JvmtiUnitedHandshakeClosure classes were introduced in the JDK 22 to unify/simplify the JVM TI functions supporting implementation of the virtual threads. This enhancement is to refactor the JVM TI internal functions JvmtiEnvThreadState::reset_current_location on the base of JvmtiHandshake and JvmtiUnitedHandshakeClosure classes. Testing: - Ran mach5 tiers 1-6 - Commit messages: - 8329674: JvmtiEnvThreadState::reset_current_location function should use JvmtiHandshake Changes: https://git.openjdk.org/jdk/pull/18630/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18630=00 Issue: https://bugs.openjdk.org/browse/JDK-8329674 Stats: 108 lines in 2 files changed: 35 ins; 67 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18630.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18630/head:pull/18630 PR: https://git.openjdk.org/jdk/pull/18630
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 12:19:03 GMT, Stefan Karlsson wrote: >> I'm not even sure what they want to say, really. Should be good to remove, >> and if anybody can make sense of it, record an issue in the bug-tracker? > > OK. I removed the %%%. I'll wait a little bit to see if someone else wants to > keep them for some reason, if not, I'll remove them. I think leaving these comments without the %%% seems fine. Describing this idea in a CR is a lot more difficult than seeing it in context as commentary, and unless the enhancement has other motivation, it won't be picked up. Leaving the comment as a clue seems useful. - PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551686232
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 12:18:24 GMT, Stefan Karlsson wrote:
>> We have a few places that uses the terms `KlassObj` and `klassOop` when
>> referring to Klasses. This is old code from before the PermGen removal, when
>> Klasses also were Java objects.
>>
>> These names tripped me up when I was reading the heap heapInspection.cpp and
>> first though we were mixing the klass *mirror* objects and klass pointers in
>> the hash code calculation:
>>
>>// An aligned reference address (typically the least
>>// address in the perm gen) used for hashing klass
>>// objects.
>>HeapWord* _ref;
>> ...
>> _ref = (HeapWord*) Universe::boolArrayKlassObj();
>> ...
>> uint KlassInfoTable::hash(const Klass* p) {
>> return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
>> }
>>
>>
>> I propose that we rename these functions (and stop casting the Klass* to a
>> (HeapWord*)).
>>
>> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
>> through our lower tiers.
>
> Stefan Karlsson has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Review Roman
This is good. The Obj was confusing.
src/hotspot/share/memory/heapInspection.hpp line 111:
> 109:
> 110: // An aligned reference address (typically the least
> 111: // address in the perm gen) used for hashing klass
Rats I missed this.
-
Marked as reviewed by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18618#pullrequestreview-1979901207
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551690175
Integrated: 8313332: Simplify lazy jmethodID cache in InstanceKlass
On Fri, 29 Mar 2024 15:25:48 GMT, Coleen Phillimore wrote: > This change simplifies the code that grows the jmethodID cache in > InstanceKlass. Instead of lazily, when there's a rare request for a > jmethodID for an obsolete method, the jmethodID cache is grown during the > RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily > allocated when there's a jmethodID allocated, so not every InstanceKlass has > a cache, but the growth now only happens in a safepoint. This code will > become racy with the potential change for deallocating jmethodIDs. > > Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in case > they're not in tier1-4). This pull request has now been integrated. Changeset: 21867c92 Author:Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27 Stats: 229 lines in 7 files changed: 44 ins; 151 del; 34 mod 8313332: Simplify lazy jmethodID cache in InstanceKlass Reviewed-by: amenkov, sspitsyn, dcubed - PR: https://git.openjdk.org/jdk/pull/18549
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v4]
On Thu, 4 Apr 2024 00:07:34 GMT, Coleen Phillimore wrote: >> This change simplifies the code that grows the jmethodID cache in >> InstanceKlass. Instead of lazily, when there's a rare request for a >> jmethodID for an obsolete method, the jmethodID cache is grown during the >> RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily >> allocated when there's a jmethodID allocated, so not every InstanceKlass has >> a cache, but the growth now only happens in a safepoint. This code will >> become racy with the potential change for deallocating jmethodIDs. >> >> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in >> case they're not in tier1-4). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Two more. I've run through tiers 5-7 with the patch now. Thank you for the reviews Alex, Serguei and Dan. - PR Comment: https://git.openjdk.org/jdk/pull/18549#issuecomment-2037185846
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v16]
> The deprecated Subject Delegation feature in JMX will be removed. > > This was marked in JDK 21 as deprecated for removal (JDK-8298966). Kevin Walls has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 25 commits: - Merge remote-tracking branch 'upstream/master' into 832_SubjectDelegation_remove - Merge - Missing code doc nit. - Missing code doc nit. - RMIConnectionImpl_Stub also should explicity inherit the unchecked UOE. - Clarify JMXConnector equivalence comment. - RMIConnectionImpl needs to explicity inherit the unchecked UOE. - typo - Javadoc update - Remove unnecessary constructor in private class - ... and 15 more: https://git.openjdk.org/jdk/compare/b9da1401...7fec01c7 - Changes: https://git.openjdk.org/jdk/pull/18025/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18025=15 Stats: 2026 lines in 35 files changed: 214 ins; 1632 del; 180 mod Patch: https://git.openjdk.org/jdk/pull/18025.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18025/head:pull/18025 PR: https://git.openjdk.org/jdk/pull/18025
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v9]
On Wed, 27 Mar 2024 05:26:09 GMT, Chris Plummer wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Undo include
>
> src/hotspot/share/utilities/debug.cpp line 680:
>
>> 678:
>> 679: // Additional "good oop" checks, separate method to not disturb
>> existing asserts.
>> 680: extern "C" bool dbg_is_good_oop_detailed(oopDesc* o) {
>
> What was the motivation for adding this? Is this copied from other HS code?
> How do we know it's doing a good enough job of verification? What happens if
> verification is not thorough enough?
On use of dbg_is_good_oop() and do we need our own dbg_is_good_oop_detailed():
The worst case is a crash if the data looks enough like an oop to pass basic
checks, but if the klass pointer or a field turns out to be a wild pointer. So
I wanted a stricter and more careful "is good oop" check, e.g. to make it
verify the klass pointer is safe, before calling is_klass(). I cannot now see
a crash when taking a valid object and examining thousands of what must be
wrong pointers in memory after it.
I kept these separate, as had seen an assert when making the existing
dbg_is_good_oop more strict. But that was an error, I had the pointer
arithmetic wrong.
This check is called by asserts in two existing places, plus the new
diagnosticCommand in this PR. With this update I'm running all of tier1 OK,
which includes ContFramePopTest.java where I could previously trigger an assert.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1551632113
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v11]
On Fri, 29 Mar 2024 04:05:53 GMT, Serguei Spitsyn wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Test more pointer types: compiled method and metadata.
>
> test/hotspot/jtreg/serviceability/dcmd/vm/VMInspectTest.java line 117:
>
>> 115: output = executor.execute("VM.inspect -1");
>> 116: output.shouldContain("address not safe");
>> 117:
>
> Nit: Just a suggestion to make the test more readable. Now when more test
> cases have been added you may want to refactor it to call a separate method
> for each sub-test.
> E.g.: `testBaddAddresses()`, `testMisalignedAddress()`,
> `testCompiledMethodAddress()`, `testMetadataAddress()`, `testClassAddress()`,
> `testThreadAddress()`, etc.
Thanks, yes have updated the test and split up the tests.
Also added a retry on the Java object inspection part, as I saw a rare failure
where the pointer found in Thread.print is no longer correct when we do the
inspect.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1551614153
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v13]
> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object > information. > > Not recommended for live production use. Requires UnlockDiagnosticVMOptions > and not included in jcmd help output, to remind us this is not a > general-purpose customer-facing tool. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: VMInspectTest update - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/e7fc0325..17f22d65 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=12 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=11-12 Stats: 82 lines in 1 file changed: 47 ins; 9 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 12:18:24 GMT, Stefan Karlsson wrote:
>> We have a few places that uses the terms `KlassObj` and `klassOop` when
>> referring to Klasses. This is old code from before the PermGen removal, when
>> Klasses also were Java objects.
>>
>> These names tripped me up when I was reading the heap heapInspection.cpp and
>> first though we were mixing the klass *mirror* objects and klass pointers in
>> the hash code calculation:
>>
>>// An aligned reference address (typically the least
>>// address in the perm gen) used for hashing klass
>>// objects.
>>HeapWord* _ref;
>> ...
>> _ref = (HeapWord*) Universe::boolArrayKlassObj();
>> ...
>> uint KlassInfoTable::hash(const Klass* p) {
>> return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
>> }
>>
>>
>> I propose that we rename these functions (and stop casting the Klass* to a
>> (HeapWord*)).
>>
>> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
>> through our lower tiers.
>
> Stefan Karlsson has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Review Roman
Thanks! Looks good to me, now.
Roman
-
Marked as reviewed by rkennke (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18618#pullrequestreview-1979767060
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 12:13:21 GMT, Roman Kennke wrote: >> I think it is an old-style TODO. I'm considering if we shouldn't just remove >> these comments. What do people think about that? > > I'm not even sure what they want to say, really. Should be good to remove, > and if anybody can make sense of it, record an issue in the bug-tracker? OK. I removed the %%%. I'll wait a little bit to see if someone else wants to keep them for some reason, if not, I'll remove them. - PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551568127
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 10:07:11 GMT, Roman Kennke wrote:
>> Stefan Karlsson has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review Roman
>
> src/hotspot/share/memory/heapInspection.cpp line 173:
>
>> 171: KlassInfoTable::KlassInfoTable(bool add_all_classes) {
>> 172: _size_of_instances_in_words = 0;
>> 173: _ref = (uintptr_t) Universe::boolArrayKlass();
>
> It seems weird (non-obvious) to cast to uintptr_t here. I see it is only used
> in KlassInfoTable::hash(), which is weird, too. I am not sure that this even
> does a useful hashing. Might be worth to get rid of the whole thing and use
> the
> [fastHash](https://github.com/rkennke/jdk/blob/JDK-8305896/src/hotspot/share/utilities/fastHash.hpp)
> stuff that @rose00 proposed for Lilliput. Perhaps in a follow-up. I'd
> probably either cast to void* or Klass*, or cast to uintptr_t as you did and
> remove the unnecessary cast in ::hash().
I agree. I'll start by removing the redundant cast in `::hash()`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551554904
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 12:08:23 GMT, Stefan Karlsson wrote: >> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 1202: >> >>> 1200: ldrw(scan_temp, Address(recv_klass, Klass::vtable_length_offset())); >>> 1201: >>> 1202: // %%% Could store the aligned, prescaled offset in the klassoop. >> >> Unrelated, but what's the point of the %%% in all those comments? Might want >> to remove that, while you're there. > > I think it is an old-style TODO. I'm considering if we shouldn't just remove > these comments. What do people think about that? I'm not even sure what they want to say, really. Should be good to remove, and if anybody can make sense of it, record an issue in the bug-tracker? - PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551557034
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
> We have a few places that uses the terms `KlassObj` and `klassOop` when
> referring to Klasses. This is old code from before the PermGen removal, when
> Klasses also were Java objects.
>
> These names tripped me up when I was reading the heap heapInspection.cpp and
> first though we were mixing the klass *mirror* objects and klass pointers in
> the hash code calculation:
>
>// An aligned reference address (typically the least
>// address in the perm gen) used for hashing klass
>// objects.
>HeapWord* _ref;
> ...
> _ref = (HeapWord*) Universe::boolArrayKlassObj();
> ...
> uint KlassInfoTable::hash(const Klass* p) {
> return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
> }
>
>
> I propose that we rename these functions (and stop casting the Klass* to a
> (HeapWord*)).
>
> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
> through our lower tiers.
Stefan Karlsson has updated the pull request incrementally with one additional
commit since the last revision:
Review Roman
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18618/files
- new: https://git.openjdk.org/jdk/pull/18618/files/02bcbd89..85f6bbe6
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18618=01
- incr: https://webrevs.openjdk.org/?repo=jdk=18618=00-01
Stats: 5 lines in 5 files changed: 0 ins; 0 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/18618.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18618/head:pull/18618
PR: https://git.openjdk.org/jdk/pull/18618
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal
On Thu, 4 Apr 2024 09:55:38 GMT, Roman Kennke wrote:
>> We have a few places that uses the terms `KlassObj` and `klassOop` when
>> referring to Klasses. This is old code from before the PermGen removal, when
>> Klasses also were Java objects.
>>
>> These names tripped me up when I was reading the heap heapInspection.cpp and
>> first though we were mixing the klass *mirror* objects and klass pointers in
>> the hash code calculation:
>>
>>// An aligned reference address (typically the least
>>// address in the perm gen) used for hashing klass
>>// objects.
>>HeapWord* _ref;
>> ...
>> _ref = (HeapWord*) Universe::boolArrayKlassObj();
>> ...
>> uint KlassInfoTable::hash(const Klass* p) {
>> return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
>> }
>>
>>
>> I propose that we rename these functions (and stop casting the Klass* to a
>> (HeapWord*)).
>>
>> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
>> through our lower tiers.
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 1202:
>
>> 1200: ldrw(scan_temp, Address(recv_klass, Klass::vtable_length_offset()));
>> 1201:
>> 1202: // %%% Could store the aligned, prescaled offset in the klassoop.
>
> Unrelated, but what's the point of the %%% in all those comments? Might want
> to remove that, while you're there.
I think it is an old-style TODO. I'm considering if we shouldn't just remove
these comments. What do people think about that?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551548890
Re: RFR: 8329432: PopFrame and ForceEarlyReturn functions should use JvmtiHandshake
On Tue, 2 Apr 2024 00:22:28 GMT, Serguei Spitsyn wrote: > The internal JVM TI `JvmtiHandshake` and `JvmtiUnitedHandshakeClosure` > classes were introduced in the JDK 22 to unify/simplify the JVM TI functions > supporting implementation of the virtual threads. This enhancement is to > refactor JVM TI functions `PopFrame` and `ForceEarlyReturn` on the base of > `JvmtiHandshake` and `JvmtiUnitedHandshakeClosure` classes. > > Testing: > > Ran mach5 tiers 1-6 Patricio, thank you for review! - PR Comment: https://git.openjdk.org/jdk/pull/18570#issuecomment-2036948300
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal
On Thu, 4 Apr 2024 09:45:58 GMT, Stefan Karlsson wrote:
> We have a few places that uses the terms `KlassObj` and `klassOop` when
> referring to Klasses. This is old code from before the PermGen removal, when
> Klasses also were Java objects.
>
> These names tripped me up when I was reading the heap heapInspection.cpp and
> first though we were mixing the klass *mirror* objects and klass pointers in
> the hash code calculation:
>
>// An aligned reference address (typically the least
>// address in the perm gen) used for hashing klass
>// objects.
>HeapWord* _ref;
> ...
> _ref = (HeapWord*) Universe::boolArrayKlassObj();
> ...
> uint KlassInfoTable::hash(const Klass* p) {
> return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
> }
>
>
> I propose that we rename these functions (and stop casting the Klass* to a
> (HeapWord*)).
>
> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
> through our lower tiers.
This is a useful change, it has tripped me up a couple of times, too. Change
mostly looks good, just a few minor suggestions.
src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 1202:
> 1200: ldrw(scan_temp, Address(recv_klass, Klass::vtable_length_offset()));
> 1201:
> 1202: // %%% Could store the aligned, prescaled offset in the klassoop.
Unrelated, but what's the point of the %%% in all those comments? Might want to
remove that, while you're there.
src/hotspot/share/memory/heapInspection.cpp line 173:
> 171: KlassInfoTable::KlassInfoTable(bool add_all_classes) {
> 172: _size_of_instances_in_words = 0;
> 173: _ref = (uintptr_t) Universe::boolArrayKlass();
It seems weird (non-obvious) to cast to uintptr_t here. I see it is only used
in KlassInfoTable::hash(), which is weird, too. I am not sure that this even
does a useful hashing. Might be worth to get rid of the whole thing and use the
[fastHash](https://github.com/rkennke/jdk/blob/JDK-8305896/src/hotspot/share/utilities/fastHash.hpp)
stuff that @rose00 proposed for Lilliput. Perhaps in a follow-up. I'd probably
either cast to void* or Klass*, or cast to uintptr_t as you did and remove the
unnecessary cast in ::hash().
-
Changes requested by rkennke (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18618#pullrequestreview-1979371014
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551366745
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551384583
RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal
We have a few places that uses the terms `KlassObj` and `klassOop` when
referring to Klasses. This is old code from before the PermGen removal, when
Klasses also were Java objects.
These names tripped me up when I was reading the heap heapInspection.cpp and
first though we were mixing the klass *mirror* objects and klass pointers in
the hash code calculation:
// An aligned reference address (typically the least
// address in the perm gen) used for hashing klass
// objects.
HeapWord* _ref;
...
_ref = (HeapWord*) Universe::boolArrayKlassObj();
...
uint KlassInfoTable::hash(const Klass* p) {
return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
}
I propose that we rename these functions (and stop casting the Klass* to a
(HeapWord*)).
Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
through our lower tiers.
-
Commit messages:
- 8329655: Cleanup KlassObj and klassOop names after the PermGen removal
Changes: https://git.openjdk.org/jdk/pull/18618/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18618=00
Issue: https://bugs.openjdk.org/browse/JDK-8329655
Stats: 125 lines in 29 files changed: 0 ins; 2 del; 123 mod
Patch: https://git.openjdk.org/jdk/pull/18618.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18618/head:pull/18618
PR: https://git.openjdk.org/jdk/pull/18618
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v12]
> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object > information. > > Not recommended for live production use. Requires UnlockDiagnosticVMOptions > and not included in jcmd help output, to remind us this is not a > general-purpose customer-facing tool. Kevin Walls 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 26 additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 8318026_jcmd_VMdebug_command - One dbg_is_good_oop method - Test more pointer types: compiled method and metadata. - Argument to be named address. - test nit - Undo include - Change to jcmd VM.inspect - Merge remote-tracking branch 'upstream/master' into 8318026_jcmd_VMdebug_command - Test update - Show description if unknown subcommand. - ... and 16 more: https://git.openjdk.org/jdk/compare/ca24041f...e7fc0325 - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/9ed958f6..e7fc0325 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=11 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=10-11 Stats: 13189 lines in 329 files changed: 4768 ins; 6005 del; 2416 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 00:05:20 GMT, Vladimir Kozlov wrote:
>> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE
>> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365)
>> which was used for AOT [JEP 295](https://openjdk.org/jeps/295)
>> implementation in JDK 9. The code was left in HotSpot assuming it will help
>> in a future. But during work on Leyden we decided to not use it. In Leyden
>> cached compiled code will be restored in CodeCache as normal nmethods: no
>> need to change VM's runtime and GC code to process them.
>>
>> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce
>> header size in separate changes. In these changes I did simple fields
>> reordering to keep small (1 byte) fields together.
>>
>> I do not see (and not expected) performance difference with these changes.
>>
>> Tested tier1-5, xcomp, stress. Running performance testing.
>>
>> I need help with testing on platforms which Oracle does not support.
>
> Vladimir Kozlov 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:
>
> - Address comments
> - Merge branch 'master' into 8329332
> - Removed not_used state of nmethod
> - remove trailing whitespace
> - 8329332: Remove CompiledMethod and CodeBlobLayout classes
I took a second pass over the changes. I've given a few suggestions below. None
of them should require respinning of tests (except for making sure that this
still builds).
src/hotspot/share/code/codeBlob.hpp line 168:
> 166: bool is_vtable_blob() const { return _kind ==
> CodeBlobKind::Blob_Vtable; }
> 167: bool is_method_handles_adapter_blob() const { return _kind ==
> CodeBlobKind::Blob_MH_Adapter; }
> 168: bool is_upcall_stub() const { return _kind ==
> CodeBlobKind::Blob_Upcall; }
The `Blob_` prefix is now redundant since we always have to prefix with
CodeBlobKind::. Just a suggestion if you want to shorten these.
src/hotspot/share/gc/shared/gcBehaviours.hpp line 31:
> 29: #include "oops/oopsHierarchy.hpp"
> 30:
> 31: // This is the behaviour for checking if a nmethod is unloading
Maybe this should be *an* nmethod?
src/hotspot/share/gc/shenandoah/shenandoahUnload.cpp line 81:
> 79: class ShenandoahIsUnloadingBehaviour : public IsUnloadingBehaviour {
> 80: public:
> 81: virtual bool has_dead_oop(nmethod* const nm) const {
Is there a reason why this was changed to `nmethod* const nm` instead of
`nmethod* nm`? IsUnloadingBehviour::has_dead_oop uses `nmethod* nm`. This
question applies to the other changes in this file as well.
src/hotspot/share/gc/x/xUnload.cpp line 78:
> 76: class XIsUnloadingBehaviour : public IsUnloadingBehaviour {
> 77: public:
> 78: virtual bool has_dead_oop(nmethod* const nm) const {
`nmethod* const nm` => `nmethod* nm`. (ZGC's style is to use const for local
variables, but not for variables in the parameter list). The same applies to
the rest of the changes to this file.
src/hotspot/share/gc/z/zUnload.cpp line 77:
> 75: class ZIsUnloadingBehaviour : public IsUnloadingBehaviour {
> 76: public:
> 77: virtual bool has_dead_oop(nmethod* const nm) const {
`nmethod* const nm` => `nmethod* nm`. (ZGC's style is to use const for local
variables, but not for variables in the parameter list). The same applies to
the rest of the changes to this file.
src/hotspot/share/runtime/javaThread.hpp line 123:
> 121: DeoptResourceMark* _deopt_mark; // Holds special
> ResourceMark for deoptimization
> 122:
> 123: nmethod* _deopt_nmethod; // nmethod that is
> currently being deoptimized
The alignment is (and was) weird here.
-
Marked as reviewed by stefank (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18554#pullrequestreview-1978954058
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551107567
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551073461
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551077362
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551080101
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551080280
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551100400
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 00:05:20 GMT, Vladimir Kozlov wrote: >> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE >> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) >> which was used for AOT [JEP 295](https://openjdk.org/jeps/295) >> implementation in JDK 9. The code was left in HotSpot assuming it will help >> in a future. But during work on Leyden we decided to not use it. In Leyden >> cached compiled code will be restored in CodeCache as normal nmethods: no >> need to change VM's runtime and GC code to process them. >> >> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce >> header size in separate changes. In these changes I did simple fields >> reordering to keep small (1 byte) fields together. >> >> I do not see (and not expected) performance difference with these changes. >> >> Tested tier1-5, xcomp, stress. Running performance testing. >> >> I need help with testing on platforms which Oracle does not support. > > Vladimir Kozlov 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: > > - Address comments > - Merge branch 'master' into 8329332 > - Removed not_used state of nmethod > - remove trailing whitespace > - 8329332: Remove CompiledMethod and CodeBlobLayout classes There is a stale comment in `test/jdk/com/sun/jdi/EATests.java:1288` -// (See CompiledMethod::is_at_poll_return()) +// (See nmethod::is_at_poll_return()) - PR Review: https://git.openjdk.org/jdk/pull/18554#pullrequestreview-1978884423
