Re: RFR: JDK-8318671: Potential uninitialized uintx value after JDK-8317683 [v5]
On Mon, 13 Nov 2023 13:33:26 GMT, Thomas Stuefe wrote: >> When using 'MemStat' CompileCommand, we accidentally register the command if >> an invalid suboption had been specified. Fixed, added regression test >> (verified). > > Thomas Stuefe has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - Merge branch 'master' into > JDK-8318671-Potential-uninitialized-uintx-value-after-JDK-8317683 > - Fix Windows build > - remove tab > - Make patch more palatable > - Merge branch 'openjdk:master' into > JDK-8318671-Potential-uninitialized-uintx-value-after-JDK-8317683 > - JDK-8318671-Potential-uninitialized-uintx-value-after-JDK-8317683 @shipilev maybe, as bug owner? - PR Comment: https://git.openjdk.org/jdk/pull/16335#issuecomment-1809719133
Re: RFR: 8319961: JvmtiEnvBase doesn't zero _ext_event_callbacks [v2]
> Zero'ing memory of extension event callbacks Roman Marchenko has updated the pull request incrementally with one additional commit since the last revision: Fixing review comments - Changes: - all: https://git.openjdk.org/jdk/pull/16647/files - new: https://git.openjdk.org/jdk/pull/16647/files/aa3bfaec..2ef1e341 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16647&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16647&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16647.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16647/head:pull/16647 PR: https://git.openjdk.org/jdk/pull/16647
Re: RFR: 8319961: JvmtiEnvBase doesn't zero _ext_event_callbacks [v2]
On Tue, 14 Nov 2023 07:42:08 GMT, David Holmes wrote: >> Roman Marchenko has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixing review comments > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 217: > >> 215: // all callbacks initially null >> 216: memset(&_event_callbacks,0,sizeof(jvmtiEventCallbacks)); >> 217: memset(&_ext_event_callbacks, 0, sizeof(jvmtiExtEventCallbacks)); > > Looks good. > > While you are here could you adjust the existing memset line and add spaces > after the commas please. Thanks. Done - PR Review Comment: https://git.openjdk.org/jdk/pull/16647#discussion_r1392157095
Re: RFR: 8319961: JvmtiEnvBase doesn't zero _ext_event_callbacks [v2]
On Tue, 14 Nov 2023 08:18:41 GMT, Roman Marchenko wrote: >> Zero'ing memory of extension event callbacks > > Roman Marchenko has updated the pull request incrementally with one > additional commit since the last revision: > > Fixing review comments The previous test run was OK https://github.com/wkia/jdk/actions/runs/6852317395 Now it fails on MacOS: "hotspot/jtreg/gc/TestAllocHumongousFragment.java: java.lang.OutOfMemoryError: Java heap space", so I guess it is caused by infrastructure issues. - PR Comment: https://git.openjdk.org/jdk/pull/16647#issuecomment-1809990842
Integrated: 8319961: JvmtiEnvBase doesn't zero _ext_event_callbacks
On Tue, 14 Nov 2023 06:50:58 GMT, Roman Marchenko wrote: > Zero'ing memory of extension event callbacks This pull request has now been integrated. Changeset: 97ea5bf0 Author:Roman Marchenko Committer: Yuri Nesterenko URL: https://git.openjdk.org/jdk/commit/97ea5bf0ffafaf8009c19483b9a9b1c30401cf9a Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8319961: JvmtiEnvBase doesn't zero _ext_event_callbacks Reviewed-by: dholmes - PR: https://git.openjdk.org/jdk/pull/16647
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
Re: RFR: JDK-8318671: Potential uninitialized uintx value after JDK-8317683 [v5]
On Mon, 13 Nov 2023 13:33:26 GMT, Thomas Stuefe wrote:
>> When using 'MemStat' CompileCommand, we accidentally register the command if
>> an invalid suboption had been specified. Fixed, added regression test
>> (verified).
>
> Thomas Stuefe has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains six commits:
>
> - Merge branch 'master' into
> JDK-8318671-Potential-uninitialized-uintx-value-after-JDK-8317683
> - Fix Windows build
> - remove tab
> - Make patch more palatable
> - Merge branch 'openjdk:master' into
> JDK-8318671-Potential-uninitialized-uintx-value-after-JDK-8317683
> - JDK-8318671-Potential-uninitialized-uintx-value-after-JDK-8317683
src/hotspot/share/compiler/compilerOracle.cpp line 678:
> 676: // Parse an uintx-based option value. Also takes care of parsing enum
> values for options that are enums.
> 677: // Returns true if ok, false if the value could not be parsed.
> 678: static bool parseUintxValue(enum CompileCommand option, const char*
> line, uintx& value, int& bytes_read) {
It is honestly weird to see `parse***Uintx***Value` dealing with enums, and be
specialized for `MemStat`. Can you reflow this to match how `MemLimit` does it?
https://github.com/openjdk/jdk/blob/7bb1999c51cdfeb020047e1094229fda1ec5a99d/src/hotspot/share/compiler/compilerOracle.cpp#L702
src/hotspot/share/compiler/compilerOracle.cpp line 727:
> 725: line += bytes_read;
> 726: register_command(matcher, option, value);
> 727: return;
Why this `return` removed? All other cases in this file have the `return` after
`register_command`, which I assume the style here: once any command is properly
matched, return.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16335#discussion_r1392741630
PR Review Comment: https://git.openjdk.org/jdk/pull/16335#discussion_r1392718662
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread
On Tue, 14 Nov 2023 03:10:20 GMT, David Holmes wrote: > Is this a case where the code should be checking for `is_attaching_via_jni()`? That's a good question. I think maybe we should try to completely avoid the situation where a 'partial' `JvmtiThreadState` is created when a native thread is attaching and is in the middle of allocating the thread oop. Looking at `JvmtiSampledObjectAllocEventCollector::start()`, I think we can check if the current JavaThread `is_attaching_via_jni()` and the `threadObj()` is null. If that's the case, don't try `setup_jvmti_thread_state()` as things are not ready. In `JvmtiThreadState::state_for_while_locked` we probably want to assert that thread->threadObj() is not null if thread->jvmti_thread_state() not null, to make sure that we don't see a incomplete `JvmtiThreadState`. @caoman, I think this can also address your input on keeping `JvmtiThreadState::_thread_oop_h` always properly initialized for the attaching native thread. I tested it and it seems to work well. I'll update this PR. - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1811187153
Re: RFR: 8319961: JvmtiEnvBase doesn't zero _ext_event_callbacks [v2]
On Tue, 14 Nov 2023 08:18:41 GMT, Roman Marchenko wrote: >> Zero'ing memory of extension event callbacks > > Roman Marchenko has updated the pull request incrementally with one > additional commit since the last revision: > > Fixing review comments Doing a post integration review. This is a trivial fix and does not need a second review nor wait 24 hours. Just a heads up that HotSpot code normally requires two reviews (1 from a (R)eviewer) and 24 hours unless it is called trivial AND agreed to be trivial by your reviewers. - PR Review: https://git.openjdk.org/jdk/pull/16647#pullrequestreview-1730761590 PR Comment: https://git.openjdk.org/jdk/pull/16647#issuecomment-1811258657
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v2]
> Please review JvmtiThreadState::state_for_while_locked change to handle the > state->get_thread_oop() null case. Please see > https://bugs.openjdk.org/browse/JDK-8319935 for details. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Don't try to setup_jvmti_thread_state for obj allocation sampling if the current thread is attaching from native and is allocating the thread oop. That's to make sure we don't create a 'partial' JvmtiThreadState. - Changes: - all: https://git.openjdk.org/jdk/pull/16642/files - new: https://git.openjdk.org/jdk/pull/16642/files/959305be..c2f83e8a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16642&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16642&range=00-01 Stats: 24 lines in 2 files changed: 19 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16642.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16642/head:pull/16642 PR: https://git.openjdk.org/jdk/pull/16642
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v42]
> 8315149: Add hsperf counters for CPU time of internal GC threads Jonathan Joo has updated the pull request incrementally with one additional commit since the last revision: Update parallel workers time after Remark - Changes: - all: https://git.openjdk.org/jdk/pull/15082/files - new: https://git.openjdk.org/jdk/pull/15082/files/533af850..189d1852 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15082&range=41 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15082&range=40-41 Stats: 10 lines in 4 files changed: 5 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/15082.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15082/head:pull/15082 PR: https://git.openjdk.org/jdk/pull/15082
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v41]
On Sat, 11 Nov 2023 00:23:28 GMT, Jonathan Joo wrote: >> 8315149: Add hsperf counters for CPU time of internal GC threads > > Jonathan Joo has updated the pull request incrementally with two additional > commits since the last revision: > > - Refactor ConcurrentRefine logic > - Make CPUTimeCounters a singleton class I believe all comments have been addressed and this PR is once again RFR! - PR Comment: https://git.openjdk.org/jdk/pull/15082#issuecomment-1811353654
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v2]
On Mon, 13 Nov 2023 23:04:19 GMT, Man Cao wrote: >> Jiangli Zhou has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Don't try to setup_jvmti_thread_state for obj allocation sampling if the >> current thread is attaching from native and is allocating the thread oop. >> That's to make sure we don't create a 'partial' JvmtiThreadState. > > src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 94: > >> 92: // The state->get_thread_oop() may be null if the state is created >> during >> 93: // the allocation of the thread oop when a native thread is attaching. >> Make >> 94: // sure we don't create a new state for the JavaThread. > > I think it is important to maintain `JvmtiThreadState::_thread_oop_h` > correctly for the attached native thread. In the existing logic, with and > without the proposed change, `JvmtiThreadState::_thread_oop_h` could stay > null for an attached native thread, which seems wrong. > > It may be OK since `JvmtiThreadState::_thread_oop_h` is only used by support > for virtual threads. It is unlikely that an attached native thread becomes a > carrier for a virtual thread. However, it is probably still desirable to > update `JvmtiThreadState::_thread_oop_h` to the correct java.lang.Thread oop. Thanks for the input @caoman. I updated the PR to avoid creating a JvmtiThreadState during attaching and allocating thread oop. I think that avoids a incomplete JvmtiThreadState being created, which is seems to be cleaner. - PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1393334558
RFR: 8299426: Heap dump does not contain virtual Thread stack references
The change impelements dumping of unmounted virtual threads data (stack traces
and stack references).
Unmounted vthreads can be detected only by iterating over the heap, but hprof
stack trace records (HPROF_FRAME/HPROF_TRACE) should be written before
HPROF_HEAP_DUMP/HPROF_HEAP_DUMP_SEGMENT.
HeapDumper supports segment dump (parallel dump to separate files with
subsequent file merge outside of safepoint), the fix switches HeapDumper to
always use segment dump: 1st segment contains only non-heap data, other
segments are used for dumping heap objects. For serial dumping single-threaded
dumping is performed, but 2 segments are created anyway.
When HeapObjectDumper detects unmounted virtual thread, it writes
HPROF_FRAME/HPROF_TRACE records to the 1st segment ("global writer"), and
writes thread object (HPROF_GC_ROOT_JAVA_FRAME) and stack references
(HPROF_GC_ROOT_JAVA_FRAME/HPROF_GC_ROOT_JNI_LOCAL) to the HeapObjectDumper
segment.
As parallel dumpers may write HPROF_FRAME/HPROF_TRACE concurrently and VMDumper
needs to write non-heap data before heap object dumpers can write virtual
threads data, writing to global writer is protected with
DumperController::_global_writer_lock.
-
Commit messages:
- vthreads in heapdump
Changes: https://git.openjdk.org/jdk/pull/16665/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16665&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8299426
Stats: 308 lines in 3 files changed: 145 ins; 76 del; 87 mod
Patch: https://git.openjdk.org/jdk/pull/16665.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16665/head:pull/16665
PR: https://git.openjdk.org/jdk/pull/16665
RFR: 8320130: Problemlist 2 vmTestbase/nsk/jdi/StepRequest/addClassFilter_rt tests with Xcomp
ProblemList 2 nsk jdi tests with Xcomp on all platforms - Commit messages: - _cur_stack_depth_pl Changes: https://git.openjdk.org/jdk/pull/16667/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16667&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8320130 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16667.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16667/head:pull/16667 PR: https://git.openjdk.org/jdk/pull/16667
Re: RFR: 8320130: Problemlist 2 vmTestbase/nsk/jdi/StepRequest/addClassFilter_rt tests with Xcomp
On Wed, 15 Nov 2023 02:24:17 GMT, Alex Menkov wrote: > ProblemList 2 nsk jdi tests with Xcomp on all platforms Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16667#pullrequestreview-1731230983
Re: RFR: JDK-8318671: Potential uninitialized uintx value after JDK-8317683 [v5]
On Tue, 14 Nov 2023 15:03:42 GMT, Aleksey Shipilev wrote:
>> Thomas Stuefe has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains six commits:
>>
>> - Merge branch 'master' into
>> JDK-8318671-Potential-uninitialized-uintx-value-after-JDK-8317683
>> - Fix Windows build
>> - remove tab
>> - Make patch more palatable
>> - Merge branch 'openjdk:master' into
>> JDK-8318671-Potential-uninitialized-uintx-value-after-JDK-8317683
>> - JDK-8318671-Potential-uninitialized-uintx-value-after-JDK-8317683
>
> src/hotspot/share/compiler/compilerOracle.cpp line 678:
>
>> 676: // Parse an uintx-based option value. Also takes care of parsing enum
>> values for options that are enums.
>> 677: // Returns true if ok, false if the value could not be parsed.
>> 678: static bool parseUintxValue(enum CompileCommand option, const char*
>> line, uintx& value, int& bytes_read) {
>
> It is honestly weird to see `parse***Uintx***Value` dealing with enums, and
> be specialized for `MemStat`. Can you reflow this to match how `MemLimit`
> does it?
> https://github.com/openjdk/jdk/blob/7bb1999c51cdfeb020047e1094229fda1ec5a99d/src/hotspot/share/compiler/compilerOracle.cpp#L702
Okay, rewritten in the style of https://github.com/openjdk/jdk/pull/16631.
Retested too. Let's get this out of the door, please.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16335#discussion_r1393696933
Re: RFR: JDK-8318671: Potential uninitialized uintx value after JDK-8317683 [v6]
> When using 'MemStat' CompileCommand, we accidentally register the command if > an invalid suboption had been specified. Fixed, added regression test > (verified). Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision: feedback shade - Changes: - all: https://git.openjdk.org/jdk/pull/16335/files - new: https://git.openjdk.org/jdk/pull/16335/files/7abf7cc4..591ce6e1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16335&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16335&range=04-05 Stats: 27 lines in 1 file changed: 9 ins; 8 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/16335.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16335/head:pull/16335 PR: https://git.openjdk.org/jdk/pull/16335
Re: RFR: 8319961: JvmtiEnvBase doesn't zero _ext_event_callbacks [v2]
On Tue, 14 Nov 2023 20:51:22 GMT, Daniel D. Daugherty wrote: >> Roman Marchenko has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixing review comments > > Just a heads up that HotSpot code normally requires two reviews (1 from a > (R)eviewer) > and 24 hours unless it is called trivial AND agreed to be trivial by your > reviewers. @dcubed-ojdk Sorry, I didn't know that. Could you point the discussion about +24 hours waiting please? BTW I seems like both requirements may be automated in github. - PR Comment: https://git.openjdk.org/jdk/pull/16647#issuecomment-1811943616
