Re: RFR: 8344562: Remove security manager dependency from module jdk.jdi
On Tue, 19 Nov 2024 18:29:33 GMT, Brian Burkhalter wrote:
> Trivial removal of the use of the `SecurityManager` from a single class.
src/jdk.jdi/share/classes/com/sun/tools/jdi/VirtualMachineManagerImpl.java line
65:
> 63: SecurityManager sm = System.getSecurityManager();
> 64: if (sm != null) {
> 65: JDIPermission vmmPermission =
`JDIPermission` is now unused
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22251#discussion_r1849655914
Re: RFR: 8344177: Remove SecurityManager and related calls from java.management [v3]
On Mon, 18 Nov 2024 17:08:21 GMT, Kevin Walls wrote:
>> Remove redundant SecurityManager, AccessController references
>> (following on from JDK-8338411: Implement JEP 486: Permanently Disable the
>> Security Manager).
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add back checkClassLoader
src/java.management/share/classes/sun/management/ManagementFactoryHelper.java
line 194:
> 192: if (logging.isPresent()) {
> 193: return Class.forName(logging.get(), className);
> 194: } else {
Suggestion:
} else {
src/java.management/share/classes/sun/management/VMManagementImpl.java line 249:
> 247:
> 248: // construct PerfInstrumentation object
> 249: Perf perf = Perf.getPerf();
Suggestion:
Perf perf = Perf.getPerf();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22100#discussion_r1848511628
PR Review Comment: https://git.openjdk.org/jdk/pull/22100#discussion_r1848512131
Re: RFR: 8343741: SA jstack --mixed should print information about VM locks [v6]
On Mon, 18 Nov 2024 20:56:27 GMT, Leonid Mesnik wrote:
>> Hi
>> Could you please review the the fix that add locks information into SA jhsdb
>> stack --mixed output.
>>
>> Here is the motivation for this rfe and explanation why I add it into SA now.
>>
>> The information about current owners of Hotspot Mutex is often very useful
>> for dealock investigations.
>>
>> The jcmd usually doesn't work because VM can't reach or exit safepoints. So
>> it doesn't respond to jcmd.
>>
>> The SA 'jstack --mixed' provides information about stacktraces on Java and
>> non-Java Threads. So having information about locks along with stack traces
>> might significantly help to identify issues.
>>
>> The gdb allows to provide stacktraces, but the debug symbols are required to
>> get info about locks. These symbols are often absent during execution.
>> Also the debugger solution is OS specifc.
>>
>> The significant part of fix is refacotorrng of mutex_array to be vmStructs
>> compatible.
>>
>> The adding support of non-JavaThreads into SA might be implemented later to
>> obtain more info about their names.
>> The example of output:
>>
>> [2024-11-06T21:32:48.897414435Z] Gathering output for process 1620563
>> Attaching to process ID 1620533, please wait...
>> Debugger attached successfully.
>> Server compiler detected.
>> JVM version is 24-internal-adhoc.lmesnik.open
>> Deadlock Detection:
>>
>> No deadlocks found.
>>
>> Internal VM Mutex Threads_lock is owned by Unknnown thread (Might be
>> non-Java Thread) with address: 0x7f8cf825b190
>> Internal VM Mutex Compile_lock is owned by LockerThread with address:
>> 0x7f8cf8309a00
>> - 1620559 -
>> "C1 CompilerThread4" https://github.com/openjdk/jdk/pull/28 daemon prio=9
>> tid=0x7f8c300566a0 nid=1620559 runnable [0x]
>> java.lang.Thread.State: RUNNABLE
>> JavaThread state: _thread_blocked
>> 0x7f8cff11e88d syscall + 0x1d
>> 0x7f8cfe6c99de LinuxWaitBarrier::wait(int) + 0x8e
>> 0x7f8cfe2be409 SafepointMechanism::process(JavaThread*, bool, bool) +
>> 0x79
>> 0x7f8cfd53ea91 ThreadInVMfromNative::ThreadInVMfromNative(JavaThread*) +
>> 0xc1
>> 0x7f8cfd534a00 ciEnv::cache_jvmti_state() + 0x30
>> 0x7f8cfd679614 CompileBroker::invoke_compiler_on_method(CompileTask*) +
>> 0x204
>> 0x7f8cfd67adc8 CompileBroker::compiler_thread_loop() + 0x5c8
>> 0x7f8cfdb4426c JavaThread::thread_main_inner() + 0xcc
>> 0x7f8cfe5a0bbe Thread::call_run() + 0xbe
>> 0x7f8cfe16813b thread_native_entry(Thread*) + 0x12b
>> .
>> - 1620554 -
>> "LockerThread" https://...
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> tests using osthread fixed
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Mutex.java line 39:
> 37:
> 38: private static AddressField mutex_array;
> 39: private static int maxNum;
Let's align with `mutex_array`
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Mutex.java line 52:
> 50:
> 51: private static synchronized void initialize(TypeDataBase db) throws
> WrongTypeException {
> 52: Type type = db.lookupType("Mutex");
Suggestion:
Type type = db.lookupType("Mutex");
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Mutex.java line 60:
> 58: ownerFieldOffset = f.getOffset();
> 59: mutex_array = type.getAddressField("_mutex_array");
> 60: maxNum = type.getCIntegerField("_num_mutex").getJInt();
Suggestion:
maxNum = type.getCIntegerField("_num_mutex").getJInt();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1848364844
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1848365330
PR Review Comment: https://git.openjdk.org/jdk/pull/22171#discussion_r1848365177
Re: RFR: 8343984: Fix Unsafe address overflow [v7]
On Tue, 12 Nov 2024 16:30:12 GMT, Shaojin Wen wrote: >> In the JDK code, there are some places that may cause Unsafe offset >> overflow. The probability of occurrence is low, but if it occurs, it will >> cause JVM crash. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > fix copyright src/java.base/share/classes/java/lang/StringLatin1.java line 833: > 831: assert index >= 0 && index + 3 < length(val) : "Trusted caller > missed bounds check"; > 832: // Don't use the putChar method, Its instrinsic will cause C2 > unable to combining values into larger stores. > 833: long offset = (long) Unsafe.ARRAY_BYTE_BASE_OFFSET + index; Suggestion: long offset = (long) Unsafe.ARRAY_BYTE_BASE_OFFSET + index; - PR Review Comment: https://git.openjdk.org/jdk/pull/22027#discussion_r1838642848
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Thu, 17 Oct 2024 14:28:30 GMT, Patricio Chilano Mateo
wrote:
> This is the implementation of JEP 491: Synchronize Virtual Threads without
> Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for
> further details.
>
> In order to make the code review easier the changes have been split into the
> following initial 4 commits:
>
> - Changes to allow unmounting a virtual thread that is currently holding
> monitors.
> - Changes to allow unmounting a virtual thread blocked on synchronized trying
> to acquire the monitor.
> - Changes to allow unmounting a virtual thread blocked in `Object.wait()` and
> its timed-wait variants.
> - Changes to tests, JFR pinned event, and other changes in the JDK libraries.
>
> The changes fix pinning issues for all 4 ports that currently implement
> continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added
> recently and stand in its own commit after the initial ones.
>
> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default
> locking mode, (and `LM_MONITOR` which comes for free), but not when using
> `LM_LEGACY` mode. Note that the `LockingMode` flag has already been
> deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)), with
> the intention to remove `LM_LEGACY` code in future releases.
>
>
> ## Summary of changes
>
> ### Unmount virtual thread while holding monitors
>
> As stated in the JEP, currently when a virtual thread enters a synchronized
> method or block, the JVM records the virtual thread's carrier platform thread
> as holding the monitor, not the virtual thread itself. This prevents the
> virtual thread from being unmounted from its carrier, as ownership
> information would otherwise go wrong. In order to fix this limitation we will
> do two things:
>
> - We copy the oops stored in the LockStack of the carrier to the stackChunk
> when freezing (and clear the LockStack). We copy the oops back to the
> LockStack of the next carrier when thawing for the first time (and clear them
> from the stackChunk). Note that we currently assume carriers don't hold
> monitors while mounting virtual threads.
>
> - For inflated monitors we now record the `java.lang.Thread.tid` of the owner
> in the ObjectMonitor's `_owner` field instead of a JavaThread*. This allows
> us to tie the owner of the monitor to a `java.lang.Thread` instance, rather
> than to a JavaThread which is only created per platform thread. The tid is
> already a 64 bit field so we can ignore issues of the counter wrapping around.
>
> General notes about this part:
>
> - Since virtual threads don't need to worry about holding monitors anymo...
test/jdk/java/lang/Thread/virtual/JfrEvents.java line 323:
> 321: var started2 = new AtomicBoolean();
> 322:
> 323: Thread vthread1 = Thread.ofVirtual().unstarted(() -> {
Suggestion:
Thread vthread1 = Thread.ofVirtual().unstarted(() -> {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1808287799
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
test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/AOTClassLinkingVMOptions.java
line 143:
> 141: }
> 142:
> 143: static void init() throws Exception {
Suggestion:
static void init() throws Exception {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21642#discussion_r1819747252
Re: RFR: 8338383: Implementation of Synchronize Virtual Threads without Pinning
On Thu, 17 Oct 2024 14:28:30 GMT, Patricio Chilano Mateo
wrote:
> This is the implementation of JEP 491: Synchronize Virtual Threads without
> Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for
> further details.
>
> In order to make the code review easier the changes have been split into the
> following initial 4 commits:
>
> - Changes to allow unmounting a virtual thread that is currently holding
> monitors.
> - Changes to allow unmounting a virtual thread blocked on synchronized trying
> to acquire the monitor.
> - Changes to allow unmounting a virtual thread blocked in `Object.wait()` and
> its timed-wait variants.
> - Changes to tests, JFR pinned event, and other changes in the JDK libraries.
>
> The changes fix pinning issues for all 4 ports that currently implement
> continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added
> recently and stand in its own commit after the initial ones.
>
> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default
> locking mode, (and `LM_MONITOR` which comes for free), but not when using
> `LM_LEGACY` mode. Note that the `LockingMode` flag has already been
> deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)), with
> the intention to remove `LM_LEGACY` code in future releases.
>
>
> ## Summary of changes
>
> ### Unmount virtual thread while holding monitors
>
> As stated in the JEP, currently when a virtual thread enters a synchronized
> method or block, the JVM records the virtual thread's carrier platform thread
> as holding the monitor, not the virtual thread itself. This prevents the
> virtual thread from being unmounted from its carrier, as ownership
> information would otherwise go wrong. In order to fix this limitation we will
> do two things:
>
> - We copy the oops stored in the LockStack of the carrier to the stackChunk
> when freezing (and clear the LockStack). We copy the oops back to the
> LockStack of the next carrier when thawing for the first time (and clear them
> from the stackChunk). Note that we currently assume carriers don't hold
> monitors while mounting virtual threads.
>
> - For inflated monitors we now record the `java.lang.Thread.tid` of the owner
> in the ObjectMonitor's `_owner` field instead of a JavaThread*. This allows
> us to tie the owner of the monitor to a `java.lang.Thread` instance, rather
> than to a JavaThread which is only created per platform thread. The tid is
> already a 64 bit field so we can ignore issues of the counter wrapping around.
>
> General notes about this part:
>
> - Since virtual threads don't need to worry about holding monitors anymo...
test/jdk/java/lang/Thread/virtual/JfrEvents.java line 323:
> 321: var started2 = new AtomicBoolean();
> 322:
> 323: Thread vthread1 = Thread.ofVirtual().unstarted(() -> {
Suggestion:
Thread vthread1 = Thread.ofVirtual().unstarted(() -> {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1808287799
Re: RFR: 8341927: Remove hardcoded SunJCE provider
On Wed, 16 Oct 2024 18:47:44 GMT, Matthew Donovan wrote:
> In this PR, I removed hard-coded security providers and replaced them with a
> system property, test.provider.name. If the property is not specified, the
> provider originally used in the test is used:
>
> Cipher c = Cipher.getInstance("AES/GCM/NoPadding",
> System.getProperty("test.provider.name", "SunJCE"));
test/jdk/javax/security/auth/login/Configuration/GetInstance.java line 88:
> 86: // get an instance of JavaLoginConfig from SUN
> 87: Configuration c = Configuration.getInstance(JAVA_CONFIG, null,
> 88: System.getProperty("test.provider.name","SUN"));
Suggestion:
System.getProperty("test.provider.name", "SUN"));
test/jdk/javax/security/auth/login/Configuration/GetInstance.java line 94:
> 92: try {
> 93: c = Configuration.getInstance(JAVA_CONFIG, null,
> 94:
> System.getProperty("test.provider.name","SunRsaSign"));
Suggestion:
System.getProperty("test.provider.name", "SunRsaSign"));
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21551#discussion_r1808334456
PR Review Comment: https://git.openjdk.org/jdk/pull/21551#discussion_r1808334626
Re: RFR: 8341436: containers/docker/TestJcmdWithSideCar.java takes needlessly long to run [v3]
On Mon, 7 Oct 2024 19:37:51 GMT, Sebastian Lövdahl wrote:
>> The fix is twofold.
>>
>> 1. Stop the main container after an iteration is done. The main container is
>> started with its runtime defined as 120 seconds, which means that each
>> iteration takes 120 seconds. In reality, one iteration takes a few seconds
>> while 115 seconds is spent waiting on the main container exiting.
>>
>> 2. Change the name of the main container to be unique per iteration.
>> Containers are started with `--rm`, which means they are removed after
>> exiting. However, the removal is done asynchronously _after_ the `stop`
>> command has returned. This means that the second iteration may get an error
>> if the same container name is used if the removal was not done before the
>> container is started in the next iteration.
>>
>> On my machine, this cuts down the test runtime using Podman from 4m 13s to
>> 17s. Using Docker, the runtime goes from 4m 15s to 41s.
>>
>> Podman only runs half the test cases (since JDK-8341310) which explain some
>> of the difference. But there is also something strange going on in the
>> Docker case; every `docker stop` call takes 10 seconds, and I have not been
>> able to figure out what exactly causes it.
>>
>> Doing a manual `kill [container Java process PID]` gracefully terminates the
>> Java process and container, but `docker stop` never does. Instead, it blocks
>> for 10 seconds before abruptly killing the process using `SIGKILL`. I
>> confirmed this with a simplified case and both
>> `strace -e 'trace=!all' docker run --init eclipse-temurin:23 java ..` and
>> `strace -e 'trace=!all' docker run eclipse-temurin:23 java ..`, no signals
>> were ever visible when calling either `docker stop` or `docker kill`.
>>
>> https://www.docker.com/blog/docker-best-practices-choosing-between-run-cmd-and-entrypoint/
>> and "What is PID 1 and why does it matter?" talks about why
>> [`--init`](https://docs.docker.com/reference/cli/docker/container/run/#init)
>> is supposed to help.
>
> Sebastian Lövdahl has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Have EventGeneratorLoop end after a more predictable duration
test/hotspot/jtreg/containers/docker/TestJcmdWithSideCar.java line 176:
> 174: // 1. mount /tmp from the main container using --volumes-from.
> 175: // 2. access /tmp from the main container via /proc//root/tmp.
> 176: private static OutputAnalyzer runSideCar(MainContainer
> mainContainer, AttachStrategy attachStrategy, boolean elevated, String
> whatToRun, String... args) throws Exception {
Suggestion:
private static OutputAnalyzer runSideCar(MainContainer mainContainer,
AttachStrategy attachStrategy, boolean elevated, String whatToRun, String...
args) throws Exception {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21331#discussion_r1803124755
Re: RFR: 8339307: jhsdb jstack could not trace FFM upcall frame [v3]
On Sat, 31 Aug 2024 09:34:09 GMT, Yasumasa Suenaga wrote:
>> I attempted to check stack trace in the core generated by [SEGV example in
>> upcall](https://github.com/YaSuenag/garakuta/blob/841452d9176dab1ddbb552009c180530eb81190b/NativeSEGV/ffm/upcall/src/main/java/com/yasuenag/garakuta/nativesegv/upcall/Main.java)
>> with `jhsdb jstack`, however it failed with following exception.
>>
>>
>> Error occurred during stack walking:
>> java.lang.RuntimeException: Couldn't deduce type of CodeBlob
>> @0x7fa04c265990 for PC=0x7fa04c265aa6
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.code.CodeCache.findBlobUnsafe(CodeCache.java:124)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.code.CodeCache.findBlob(CodeCache.java:83)
>> at jdk.hotspot.agent/sun.jvm.hotspot.runtime.Frame.cb(Frame.java:119)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.runtime.x86.X86Frame.adjustUnextendedSP(X86Frame.java:334)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.runtime.x86.X86Frame.initFrame(X86Frame.java:137)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.runtime.x86.X86Frame.(X86Frame.java:163)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.runtime.x86.X86Frame.senderForInterpreterFrame(X86Frame.java:361)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.runtime.x86.X86Frame.sender(X86Frame.java:281)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.runtime.Frame.sender(Frame.java:207)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.runtime.Frame.realSender(Frame.java:212)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.runtime.VFrame.sender(VFrame.java:120)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.runtime.VFrame.javaSender(VFrame.java:144)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.tools.StackTrace.run(StackTrace.java:81)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.tools.StackTrace.run(StackTrace.java:45)
>> at jdk.hotspot.agent/sun.jvm.hotspot.tools.JStack.run(JStack.java:67)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:278)
>> at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.start(Tool.java:241)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.execute(Tool.java:134)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.tools.JStack.runWithArgs(JStack.java:90)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runJSTACK(SALauncher.java:302)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:500)
>> Caused by: sun.jvm.hotspot.types.WrongTypeException: No suitable match for
>> type of address 0x7fa04c265990 (nearest symbol is _ZTV10Upcall...
>
> Yasumasa Suenaga has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add frame size to all of UpcallStub::create() call
test/hotspot/jtreg/serviceability/sa/LingeredAppWithFFMUpcall.java line 45:
> 43: try {
> 44: Thread.sleep(60); // 10 min
> 45: } catch(InterruptedException e) {
Suggestion:
} catch (InterruptedException e) {
test/hotspot/jtreg/serviceability/sa/LingeredAppWithFFMUpcall.java line 61:
> 59:
> 60: public static void main(String[] args) {
> 61: try{
Suggestion:
try {
test/hotspot/jtreg/serviceability/sa/LingeredAppWithFFMUpcall.java line 65:
> 63: (new Thread(() -> callJNI(upcallAddr), THREAD_NAME)).start();
> 64: LingeredApp.main(args);
> 65: } catch(Exception e) {
Suggestion:
} catch (Exception e) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20789#discussion_r1740474940
PR Review Comment: https://git.openjdk.org/jdk/pull/20789#discussion_r1740475126
PR Review Comment: https://git.openjdk.org/jdk/pull/20789#discussion_r1740475258
Re: RFR: 8204681: Option to include timestamp in hprof filename
On Tue, 13 Aug 2024 15:07:17 GMT, Sonia Zaldana Calles
wrote:
> Hi all,
>
> This PR addresses [8204681](https://bugs.openjdk.org/browse/JDK-8204681)
> enabling support for timestamp expansion in filenames specified in
> `-XX:HeapDumpPath` using `%t`.
>
> As mentioned in this comments for this issue, this is somewhat related to
> [8334492](https://bugs.openjdk.org/browse/JDK-8334492) where we enabled
> support for `%p` for filenames specified in jcmd.
>
> With this patch, I propose:
> - Expanding the utility function `Arguments::copy_expand_pid` to
> `Arguments::copy_expand_arguments` to deal with `%p` expansions for pid and
> `%t` expansions for timestamps.
> - Leveraging the above utility function to enable argument expansion for both
> heap dump filenames and jcmd output commands.
> - Though the linked JBS issue only relates to heap dumps generated in case of
> OOM, I think we can edit it to more broadly support filename expansion to
> support `%t` for jcmd as well.
>
> Testing:
> - [x] Added test cases pass with all platforms (verified with a GHA job).
> - [x] Tier 1 passes with GHA.
>
> Looking forward to hearing your thoughts!
>
> Thanks,
> Sonia
test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpFilenameExpansion.java
line 53:
> 51: try {
> 52: Object[] oa = new Object[Integer.MAX_VALUE];
> 53: for(int i = 0; i < oa.length; i++) {
Suggestion:
for (int i = 0; i < oa.length; i++) {
test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpFilenameExpansion.java
line 90:
> 88: Pattern pattern =
> Pattern.compile("file\\d{4}-\\d{2}-\\d{2}_\\d{2}-\\d{2}-\\d{2}");
> 89: File[] files = new File(".").listFiles();
> 90: if(files != null) {
Suggestion:
if (files != null) {
test/jdk/sun/tools/jcmd/TestJcmdArgumentSubstitution.java line 87:
> 85: Pattern pattern =
> Pattern.compile("myfile\\d{4}-\\d{2}-\\d{2}_\\d{2}-\\d{2}-\\d{2}");
> 86: File[] files = new File(test_dir).listFiles();
> 87: if(files != null) {
Suggestion:
if (files != null) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20568#discussion_r1732794735
PR Review Comment: https://git.openjdk.org/jdk/pull/20568#discussion_r1732794999
PR Review Comment: https://git.openjdk.org/jdk/pull/20568#discussion_r1732795548
Re: RFR: 8337517: Redacted Heap Dumps
On Wed, 31 Jul 2024 19:10:41 GMT, Henry Lin wrote:
> Adds a command line option `-redact` to `jcmd`, `redact` to `jmap` and
> `-XX:+HeapDumpRedacted` enabling redacted heap dumps. When enabled, the
> output binary heap dump has zeroes written out in place of the original
> primitive values in the object fields. There is a new jtreg test
> `heapDumpRedactedTest.java` that tests that the fields are properly redacted.
test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpRedactedTest.java line 100:
> 98:
> 99: //static field
> 100: JavaThing staticInt = testClass.getStaticField("staticInt");
Suggestion:
JavaThing staticInt = testClass.getStaticField("staticInt");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20409#discussion_r1699665859
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with 10
> additional commits since the last revision:
>
> - Remove try_read
> - Add explicit to single parameter constructors
> - Remove superfluous access specifier
> - Remove unused include
> - Update assert message OMCache::set_monitor
> - Fix indentation
> - Remove outdated comment LightweightSynchronizer::exit
> - Remove logStream include
> - Remove strange comment
> - Fix javaThread include
test/hotspot/jtreg/runtime/Monitor/UseObjectMonitorTableTest.java line 126:
> 124: int count = getCount();
> 125: if (count != i * THREADS) {
> 126: throw new RuntimeException("WaitNotifyTest: Invalid
> Count " + count +
Suggestion:
throw new RuntimeException("WaitNotifyTest: Invalid Count "
+ count +
test/hotspot/jtreg/runtime/Monitor/UseObjectMonitorTableTest.java line 136:
> 134: int count = getCount();
> 135: if (count != ITERATIONS * THREADS) {
> 136: throw new RuntimeException("WaitNotifyTest: Invalid
> Count " + count);
Suggestion:
throw new RuntimeException("WaitNotifyTest: Invalid Count " +
count);
test/hotspot/jtreg/runtime/Monitor/UseObjectMonitorTableTest.java line 217:
> 215: int count = getCount();
> 216: if (count != THREADS * ITERATIONS) {
> 217: throw new RuntimeException("RandomDepthTest: Invalid
> Count " + count);
Suggestion:
throw new RuntimeException("RandomDepthTest: Invalid Count " +
count);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1684293578
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1684293811
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1684293954
Re: RFR: 8336254: Virtual thread implementation + test updates
On Thu, 11 Jul 2024 17:30:21 GMT, Alan Bateman wrote:
> Bringover some of the changes accumulated in the loom repo to the main line,
> most of these changes are test updates and have been baking in the loom repo
> for several months. The motive is partly to reduce the large set of changes
> that have accumulated in the loom repo, and partly to improve robustness and
> test coverage in the main line. The changes don't include any of the larger
> changes in the loom repo that are part of future JEPs.
>
> Implementation:
> - Robustness improvements to not throw OOME when unparking a virtual thread.
> - Robustness improvements to reduce class loading when a virtual thread parks
> or parks when pinned (jdk.internal.misc.VirtualThreads is removed,
> jdk.internal.event.VirtualThreadPinnedEvent is eagerly loaded)
> - VirtualThread changes to reduce contention on timer queues when doing
> timed-park
>
> Tests:
> - New tests for monitor enter/exit/wait/notify (this is a subset of the tests
> in the loom repo, we can't move many tests because they depend on on the
> changes to the object monitor implementation)
> - New test infrastructure to allow tests use a custom scheduler. This updates
> many tests to make use of this infrastructure, the "local" ThreadBuidlers is
> removed.
> - More test scenarios added to ThreadAPI and JVMTI GetThreadStateTest.java
> - New test for ThreadMXBean.getLockedMonitor with synchronized native methods
> - Reimplement of JVMTI VThreadEvent test to improve reliability
> - Rename some tests to get consistent naming
> - Diagnostic output in several stress tests to help trace progress in the
> event of a timeout
>
> Testing: tier1-6
test/jdk/java/lang/Thread/virtual/MonitorEnterExit.java line 216:
> 214: var started = new CountDownLatch(1);
> 215: var entered = new AtomicBoolean();
> 216: Thread vthread = Thread.ofVirtual().unstarted(() -> {
Suggestion:
Thread vthread = Thread.ofVirtual().unstarted(() -> {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20143#discussion_r1684290798
Re: RFR: 8072701: resume001 failed due to ERROR: timeout for waiting for a BreakpintEvent [v3]
On Thu, 11 Jul 2024 22:36:05 GMT, Chris Plummer wrote:
>> The test hits a breakpoint on thread2 with SUSPEND_EVENT_THREAD policy, so
>> only thread2 is suspended. It then does a vm.suspend(), which suspends all
>> threads and bumps the suspendCount of thread2 up to 2. It then does an
>> eventSet.resume(), which decrements the thread2 suspendCount to 1, so now
>> all threads are suspended with a suspendCount of 1. thread2 is then resumed
>> and we expect to hit the next thread2 breakpoint. The problem is that
>> thread2 can't hit the breakpoint until the main thread has proceeded far
>> enough, and if the vm.suspend() that suspended the main thread happens too
>> quickly, it won't have proceeded far enough, so thread2 never hits the
>> breakpoint.
>>
>> Essentially we need a vm.resume() to allow the main thread to run, but we
>> need to do it in a way that does nullify part of what the test is testing
>> for. So in order to allow the vm.resume() but not subvert the intent of the
>> test, we first do a thread2.suspend() so the vm.resume() won't resume
>> thread2.
>>
>> Testing in progress: tier1 and tier5 svc testing
>
> Chris Plummer has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix copyright and jcheck error
test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Debugee.java line 598:
> 596: * Print information about all threads in debuggee VM
> 597: */
> 598: public void printThreadsInfo(VirtualMachine vm) {
Suggestion:
public void printThreadsInfo(VirtualMachine vm) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20088#discussion_r1678148019
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v4]
On Mon, 3 Jun 2024 08:30:15 GMT, Inigo Mediavilla Saiz wrote:
>> Print the stack traces of mounted virtual threads when calling `jcmd
>> Thread.print`.
>
> Inigo Mediavilla Saiz has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add missing header
test/hotspot/jtreg/serviceability/dcmd/thread/PrintVirtualThreadTest.java line
84:
> 82: void compute() {
> 83: started.countDown();
> 84: while(true) {
Suggestion:
while (true) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1624070607
Re: RFR: 8330534: Update nsk/jdwp tests to use driver instead of othervm
On Wed, 17 Apr 2024 20:19:49 GMT, Leonid Mesnik wrote:
> The jdwp tests use debugger and debugee. There is no goal to execute debugger
> part with all VM flags, they are needed to be used with debugee VM only.
> The change is all tests is to don't use System.exit() and use 'driver'
> instead of othervm.
> test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java
> is updated to correctly set classpath for debugee
test/hotspot/jtreg/vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn001/forceEarlyReturn001.java
line 127:
> 125:
> 126: public static void main (String argv[]) {
> 127: int result = run(argv,System.out);
Suggestion:
int result = run(argv, System.out);
test/hotspot/jtreg/vmTestbase/nsk/jdwp/ThreadReference/Interrupt/interrupt001.java
line 93:
> 91: */
> 92: public static void main (String argv[]) {
> 93: int result = run(argv,System.out);
Suggestion:
int result = run(argv, System.out);
test/hotspot/jtreg/vmTestbase/nsk/jdwp/ThreadReference/Name/name001.java line
90:
> 88: */
> 89: public static void main (String argv[]) {
> 90: int result = run(argv,System.out);
Suggestion:
int result = run(argv, System.out);
test/hotspot/jtreg/vmTestbase/nsk/jdwp/ThreadReference/OwnedMonitors/ownmonitors001.java
line 101:
> 99: */
> 100: public static void main (String argv[]) {
> 101: int result = run(argv,System.out);
Suggestion:
int result = run(argv, System.out);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18826#discussion_r1574589699
PR Review Comment: https://git.openjdk.org/jdk/pull/18826#discussion_r1574590099
PR Review Comment: https://git.openjdk.org/jdk/pull/18826#discussion_r1574590307
PR Review Comment: https://git.openjdk.org/jdk/pull/18826#discussion_r1574590456
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v3]
On Thu, 21 Mar 2024 17:13:46 GMT, Bill Huang wrote:
>> This task addresses an essential aspect of our testing infrastructure: the
>> proper handling and cleanup of temporary files and socket files created
>> during test execution. The motivation behind these changes is to prevent the
>> accumulation of unnecessary files in the default temporary directory, which
>> can affect the system's storage and potentially influence subsequent test
>> runs.
>>
>> Our review identified that several tests create temporary files or socket
>> files without ensuring their removal post-execution.
>> - Direct calls to java.io.File.createTempFile and
>> java.nio.file.Files.createTempFile without adequate cleanup.
>> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not
>> explicitly removing socket files post-use.
>
> Bill Huang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed potential NPE issues.
test/jdk/java/nio/channels/unixdomain/Bind.java line 162:
> 160: // address with space should work
> 161: checkNormal(() -> {
> 162: UnixDomainSocketAddress usa =
> UnixDomainSocketAddress.of("with space");
Suggestion:
UnixDomainSocketAddress usa = UnixDomainSocketAddress.of("with
space");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1538701870
Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v2]
On Thu, 18 Jan 2024 16:19:38 GMT, Chris Plummer wrote:
>> I noticed that "clhsdb jstack" seemed to hang when I attached to process
>> with a somewhat large heap. It had taken over 10 minutes when I finally
>> decided to have a look at the SA process (using bin/jstack), which came up
>> with the following in the stack:
>>
>>
>> at
>> sun.jvm.hotspot.oops.ObjectHeap.iterateObjectsOfKlass([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ObjectHeap.java:117)
>> at
>> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.fillLocks([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:70)
>> at
>> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:36)
>> at
>> sun.jvm.hotspot.tools.StackTrace.run([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/StackTrace.java:71)
>>
>>
>> This code is meant to print information about j.u.c. locks. It it works by
>> searching the entire java heap for instances of AbstractOwnableSynchronizer.
>> This is a very expensive operation because it means not only does SA need to
>> read in the entire java heap, but it needs to create a Klass mirror for
>> every heap object so it can determine its type.
>>
>> Our testing doesn't seem to run into this slowness problem because "clhsdb
>> jstack" only iterates over the heap if AbstractOwnableSynchronizer has been
>> loaded, which it won't be if no j.u.c. locks have been created. This seems
>> to be the case wherever we use "clhsdb jstack" in testing. We do have some
>> tests that likely result in j.u.c locks, but they all use "jhsdb jstack",
>> which doesn't have this issue (it requires use of the --locks argument to
>> enable printing of j.u.c locks).
>>
>> This issue was already called out in
>> [JDK-8262098](https://bugs.openjdk.org/browse/JDK-8262098). For this CR I am
>> proposing that "clhsdb jstack" not include j.u.c lock scanning by default,
>> and add the -l argument to enable it. This will make it similar to
>> bin/jstack, which also has a -l argument, and to "clhsdb jstack", which has
>> a --locks argument (which maps internally to the Jstack.java -l argument).
>>
>> The same changes are also being made to "clhsdb pstack".
>>
>> Tested with all tier1, tier2, and tier5 svc tests.
>
> Chris Plummer has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - update clhsdb docs
> - update clhsdb docs
test/hotspot/jtreg/serviceability/sa/ClhsdbJstackWithConcurrentLock.java line
67:
> 65: String[] words = line.split(key + "|[, ]");
> 66: for (String word : words) {
> 67: word = word.replace("<","").replace(">","");
Suggestion:
word = word.replace("<", "").replace(">", "");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17479#discussion_r1468628161
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v3]
On Mon, 15 Jan 2024 05:37:02 GMT, Tom Rodriguez wrote:
>> The changes for JDK-8287061 didn't update the SA decoding logic and there
>> are other places where the decoding has gotten out of sync with HotSpot.
>> Some of them can't be tested because they are part of JVMCI but I've added a
>> directed test for the JDK-8287061 code and a more brute force test that
>> tries to decode everything.
>
> Tom Rodriguez has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Pass the proper options to the lingered app
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java line
1662:
> 1660: }
> 1661: if (blob instanceof NMethod) {
> 1662: NMethod nm = (NMethod) blob;
Suggestion:
NMethod nm = (NMethod) blob;
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java line 286:
> 284: for (Address p = scopesPCsBegin(); p.lessThan(scopesPCsEnd()); p =
> p.addOffsetTo(pcDescSize)) {
> 285: PCDesc pd = new PCDesc(p);
> 286: if (pd.getPCOffset() == -1) {
Suggestion:
if (pd.getPCOffset() == -1) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1452005032
PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1452004835
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread
On Fri, 23 Jun 2023 01:57:56 GMT, Alex Menkov wrote:
> This is follow-up JDK-8307153/JDK-8309612 (JVMTI GetThreadState on carrier
> should return STATE_WAITING)
> New test tests GetThreadState for different thread states.
> The test detected a bug in the implementation, new issue is created:
> JDK-8310584
> Corresponding testcases are commented now in the test, fix for JDK-8310584
> will uncomment them
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
line 85:
> 83: }
> 84: });
> 85: synchronized(syncObj) {
Suggestion:
synchronized (syncObj) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1242759252
Re: RFR: 8306446: java/lang/management/ThreadMXBean/Locks.java transient failures
On Thu, 15 Jun 2023 20:34:53 GMT, Kevin Walls wrote:
> This test iterates an array of ThreadInfos in a few places (e.g. in the
> method doCheck()), and needs to tolerate and ignore nulls, in case a thread
> finishes and the test hits an NPE.
>
> There are other calls like "TM.getThreadInfo(tid).getLockName()" which might
> often be risky, but if the threads are blocked as they are here, they can't
> be terminating, so this usage is safe.
>
>
> The test has additional problems when started in a virtual thread.
> ThreadMXBean.getThreadInfo() methods only return a ThreadInfo for platform
> threads. The test needs to avoid some checks if mainThread is virtual.
>
> In assertNoLock, it needs to not object to a thread holding a lock on a
> VirtualThread object is not relevant.
> Also the loop in doChecks which follows a chain of locks... This needs to
> recognise that ForkJoinPool thead is not worth pursuing. It's not one of the
> very narrow set of threads this test cares about.
>
> Despite these exclusions, the test does some reasonable verification work
> when MainThread is virtual. This test historically cam in with a general
> "JVM monitoring and management API" change, it is not testing a particular
> fix.
>
>
> There's a failure condition in doCheck() which will not make the test fail:
> if it logs "TEST FAILED" in its final for loop, there is no failure. Make
> the loop count the failures, and throw if there are any.
>
> Also, while looking into this... The variable names in some methods are
> confusing. In checkBlockedObject(), let's use "threadName" rather than
> "result" if we are finding a thread name, and let's not reuse the same result
> variable for a lockName later in the method.
>
> The logs from this test are hard to read and verify, I find it better if the
> lock objects OBJB and OBJC are of classes other than Object, so you get to
> read, e.g.:
> LockAThread blocked on Locks$ObjectB@4691fdfd
> (ObjectB, not just Object).
test/jdk/java/lang/management/ThreadMXBean/Locks.java line 72:
> 70: .filter(Objects::nonNull)
> 71: .filter(i ->
> name.equals(i.getLockOwnerName()))
> 72: .filter(i ->
> !i.getLockName().contains("java.lang.VirtualThread"))
Suggestion:
.filter(i ->
!i.getLockName().contains("java.lang.VirtualThread"))
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14501#discussion_r1236928027
Re: RFR: 8308237: add JDWP and JDI virtual thread support for ThreadReference.PopFrames [v4]
On Mon, 22 May 2023 06:35:47 GMT, Chris Plummer wrote:
>> This is a follow-on to
>> [JDK-8308000](https://bugs.openjdk.org/browse/JDK-8308000) which adds JVMTI
>> PopFrames support for virtual thread. For JDWP and JDI this is mostly a spec
>> update, although JDI needs minor changes to properly throw the correct
>> exception. Note this PR needs JDK-8264699 in order to function properly, so
>> there may be some GHA failures until JDK-8308000 is pushed.
>>
>> There are a large number of tests that can now be removed from the problem
>> list. Also, one test needs to be modified to no longer expect
>> OpaqueFrameException for virtual threads. It was just revereted back to it's
>> previous form before the OpaqueFrameException support was added for virtual
>> threads.
>>
>> As you can see from the problemlist update, there are quite a few tests for
>> popFrames() support. However, there are still two coverage gaps:
>>
>> - There is no test for throwing NativeMethodException (even for platform
>> threads)
>> - There is no test case for throwing OpaqueFrameException when the virtual
>> thread is suspended but not mounted.
>>
>> I may eventually add one or both tests to the PR, or I may just file
>> separate CRs for them for now.
>
> Chris Plummer has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix jcheck errors
test/jdk/com/sun/jdi/PopFramesTest.java line 115:
> 113: try {
> 114: Thread.sleep(1);
> 115: } catch (InterruptedException e) {
Suggestion:
} catch (InterruptedException e) {
test/jdk/com/sun/jdi/PopFramesTest.java line 192:
> 190: }
> 191:
> 192: public static void main(String[] args) throws Exception {
Suggestion:
public static void main(String[] args) throws Exception {
test/jdk/com/sun/jdi/PopFramesTest.java line 248:
> 246: BreakpointEvent bpe = startTo("PopFramesTestTarg", "popMethod",
> "()V");
> 247: ClassType targetClass =
> (ClassType)bpe.location().declaringType();
> 248: ThreadReference mainThread = bpe.thread();
Suggestion:
ThreadReference mainThread = bpe.thread();
test/jdk/com/sun/jdi/PopFramesTest.java line 254:
> 252: try {
> 253: Thread.sleep(1000); // give thread chance to get into
> Thread.sleep() or loop
> 254: } catch (InterruptedException e) {
Suggestion:
} catch (InterruptedException e) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14022#discussion_r1200225974
PR Review Comment: https://git.openjdk.org/jdk/pull/14022#discussion_r1200226261
PR Review Comment: https://git.openjdk.org/jdk/pull/14022#discussion_r1200227466
PR Review Comment: https://git.openjdk.org/jdk/pull/14022#discussion_r1200227660
Re: RFR: 8308400: add ForceEarlyReturn support for virtual threads [v2]
On Sat, 20 May 2023 00:21:04 GMT, Serguei Spitsyn wrote: >> This enhancement adds ForceEarlyReturnXXX support for virtual threads. The >> spec defines minimal support that the JVMTI ForceEarlyReturnXXX can be used >> for a virtual thread suspended an an event. >> Actually, the ForceEarlyReturnXXX can supports suspended and mounted virtual >> threads. >> >> CSR (approved): https://bugs.openjdk.org/browse/JDK-8308401 add >> ForceEarlyReturn support for virtual threads >> >> Testing: >> New test was developed: serviceability/vthread/ForceEarlyReturnTest. >> Submitted mach5 tiers 1-6 are good. >> TBD: rerun mach5 tiers 1-6 at the end of review again if necessary. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > minor tweak in libForceEarlyReturnTest.cpp test/hotspot/jtreg/serviceability/jvmti/vthread/ForceEarlyReturnTest/ForceEarlyReturnTest.java line 65: > 63: static final String expValB1 = "B1"; > 64: static final String expValB2 = "B2"; > 65: static final String expValB3 = "B3"; nit Suggestion: static final String expValB1 = "B1"; static final String expValB2 = "B2"; static final String expValB3 = "B3"; - PR Review Comment: https://git.openjdk.org/jdk/pull/14067#discussion_r1200212515
Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents
On Wed, 10 May 2023 11:12:49 GMT, Alan Bateman wrote:
> This is the implementation for JEP 451. There are two parts to this:
>
> 1. A multi-line warning is printed when a JVM TI or Java agent is loaded into
> a running VM. For JVM TI, the message is printed to stderr from
> JvmtiAgent::load. For Java agents, it is printed to System.err (as that may
> be redirected) in the JPLIS (j.l.instrumentation) implementation.
>
> 2. If running with -Djdk.instrument.traceUsage or
> -Djdk.instrument.traceUsage=true, the calls to the Instrumentation API print
> a trace message and stack trace.
src/java.instrument/share/classes/sun/instrument/InstrumentationImpl.java line
68:
> 66: public class InstrumentationImpl implements Instrumentation {
> 67: private static final String TRACE_USAGE_PROP_NAME =
> "jdk.instrument.traceUsage";
> 68: private final static boolean TRACE_USAGE;
Suggestion:
private static final boolean TRACE_USAGE;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1193098588
Re: RFR: 8307058: Implementation of Generational ZGC [v6]
On Thu, 4 May 2023 11:44:14 GMT, Stefan Karlsson wrote: >> Hi all, >> >> Please review the implementation of Generational ZGC, which can be turned on >> by adding -XX:+ZGenerational in addition to using -XX:+UseZGC. Generational >> ZGC is a major rewrite of the non-generational ZGC version that exists in >> the openjdk/jdk repository. It splits the heap into two generations; the >> young generation where newly allocated objects are born, and the old >> generation where long-lived objects get promoted to. The motivation for >> introducing generations is to allow ZGC to reclaim memory faster by not >> having to walk the entire object graph every time a garbage collection is >> run. This should make Generational ZGC suitable for more workloads. In >> particular workloads that previously hit allocation stalls because of high >> allocation rates, large live sets, or limited spare machine resources, have >> the potential to work better with Generational ZGC. For an in-depth >> description of Generational ZGC, see https://openjdk.org/jeps/439. >> >> The development of Generational ZGC started around the same time as the >> development of JDK 17. At that point we forked off the Generational ZGC >> development into its own branch and let non-generational live unaffected in >> openjdk/jdk. This safe-guarded non-generational ZGC and allowed Generational >> ZGC to move unhindered, without the shackles of having to fit into another >> GC implementation's design and quirks. Since then, almost all of the ZGC >> files have been changed. Moving forward to today, when it's ready for us to >> upstream Generational ZGC, we now need to deliver Generational ZGC without >> disrupting our current user-base. We have therefore opted to initially >> include both versions of ZGC in the code base, but with the intention to >> deprecate non-generational ZGC in a future release. Existing users running >> with only -XX:+UseZGC will get the non-generational ZGC, and users that want >> the new Generational ZGC need to run with -XX:+ZGenerational in addition to >> -XX:+UseZGC. The intention is to give the users time to validate and deploy their workloads with the new GC implementation. >> >> Including both the new evolution of a GC and its legacy predecessor poses a >> few challenges for us GC developers. The first reaction could be to try to >> mash the two implementations together and sprinkle the GC code with >> conditional statements or dynamic dispatches. We have done similar >> experiments before. When ZGC was first born, we started an experiment where >> we converted G1 into getting the same features as the evolving ZGC. It was >> quite clear to us how time consuming and complex things end up being when we >> tried to keep both the original G1 working, and at the same time implemented >> the ZGC-alike G1. Given this experience, we don't see that as a viable >> solution to deliver a maintainable and evolving Generational ZGC. Our >> pragmatic suggestion to these challenges is to let Generational ZGC live >> under the current gc/z directories and let the legacy, non-generational ZGC >> be completely separated in its own directories. This way we can continue to >> move quickly with the continued develo pment of Generational ZGC and let the non-generational ZGC be mostly untouched until it gets deprecated, and eventually removed. The non-generational ZGC directory will be gc/x and all the classes of non-generational have been prefixed with X instead of Z. An alternative to this rename could be to namespace out non-generational ZGC. We experimented with that, but it was too easy to accidentally cross-compile Generational ZGC code into non-generational ZGC, so we didn't like that approach. >> >> Most of the stand-alone cleanups and enhancements outside of the ZGC code >> have already been upstreamed to openjdk/jdk. There are still a few patches >> that could/should be pushed separately, but they will be easier to >> understand by also looking at the Generational ZGC code, so they will be >> sent out after this PR has been published. The patches that could be >> published separately are: >> >> * 59d1e96af6a UPSTREAM: Introduce check_oop infrastructure to check oops in >> the oop class >> * ca9edf8aa79 UPSTREAM: RISCV tmp reg cleanup resolve_jobject >> * 4bec9c69b67 CLEANUP: barrierSetNMethod_aarch64.cpp >> * b67d03a3f04 UPSTREAM: Add relaxed add&fetch for aarch64 atomics >> * a2824734d23 UPSTREAM: lir_xchg >> * 36cd39c0126 UPSTREAM: assembler_ppc CMPLI >> * 447259cea42 UPSTREAM: assembler_ppc ANDI >> * 9417323499a UPSTREAM: Add VMErrorCallback infrastructure >> >> Regarding all the changesets you see in this PR, they form the history of >> the development of Generational ZGC. It might look a bit unconventional to >> what you are used to see in openjdk development. What we have done is to use >> merges with the 'ours' strategy to ignore the previous Generational ZGC >> patche
Re: RFR: 8306471: Add virtual threads support to JDWP ThreadReference.Stop and JDI ThreadReference.stop()
On Wed, 19 Apr 2023 23:40:56 GMT, Chris Plummer wrote:
> Note this PR depends on the #13546 PR for the following:
>
> [JDK-8306434](https://bugs.openjdk.org/browse/JDK-8306434): add support of
> virtual threads to JVMTI StopThread
>
> So it can't be finalized and push until after JDK-8306434 is pushed. There
> will also be GHA failures until then. If JDK-8306434 results in any
> additional spec changes, they will likely impact this CR also.
>
>
> [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034) is adding some
> virtual thread support to JVMTI StopThread. This will allow JDWP
> ThreadReference.Stop and JDI ThreadReference.stop() to have the same level
> support for virtual threads.
>
> Mostly this is a spec update for JDWP and JDI, but there are also some minor
> changes needed to the ThreadReference.stop() handling of JDWP errors, and
> throwing the appropriate exceptions. Also some minor cleanup in jdb. The
> debug agent doesn't need changes since JVMTI errors are just passed through
> as the corresponding JDWP errors, and the code for this is already in place.
> These errors are not new nor need any special handling.
>
> Our existing testing for ThreadReference.stop() is fairly weak:
>
> - nsk/jdb/kill/kill001, which tests stop() when the thread is suspended at a
> breakpoint. It will get problem listed by
> [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034). I have fixes for
> it already working and will push it separately.
> - nsk/jdi/stop/stop001, which is problem listed and only tests when the
> thread is blocked in Object.wait()
> - nsk/jdi/stop/stop002, which only tests that throwing an invalid exception
> fails properly
>
> I decided to take stop002 and make it a more thorough test of
> ThreadReference.stop(). See the comment at the top for a list of what is
> tested. As for reviewing this test, it's probably best to look at it as a
> completely new test rather than looking at diffs since so much has been
> changed and added.
test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line
169:
> 167: thrRef.stop(throwableRef);
> 168: log.display("TEST #2 PASSED: stop() call succeeded.");
> 169: } catch(Exception ue) {
nit
Suggestion:
} catch (Exception ue) {
test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line
192:
> 190: log.display("TEST #3 PASSED: stop() call
> succeeded.");
> 191: }
> 192: } catch(Exception ue) {
nit
Suggestion:
} catch (Exception ue) {
test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line
223:
> 221: log.display("TEST #4 PASSED: stop() call
> succeeded.");
> 222: }
> 223: } catch(Throwable ue) {
Suggestion:
} catch (Throwable ue) {
test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line
255:
> 253: log.display("TEST #5 PASSED: stop() call for
> suspended thread succeeded");
> 254: }
> 255: } catch(Throwable ue) {
Suggestion:
} catch (Throwable ue) {
test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line
274:
> 272: } finally {
> 273: // Force the debuggee out of both loops
> 274: if (objRef != null && stopLoop1 != null && stopLoop2 !=
> null) {
Suggestion:
if (objRef != null && stopLoop1 != null && stopLoop2 != null) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1174633416
PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1174633435
PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1174633460
PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1174633464
PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1174633486
Re: RFR: 8304919: Implementation of Virtual Threads [v3]
On Wed, 29 Mar 2023 07:31:40 GMT, Alan Bateman wrote:
>> JEP 444 proposes to make virtual threads a permanent feature in Java 21. The
>> APIs that were preview APIs in Java 19/20 are changed to permanent and their
>> `@since`/equivalent are changed to 21 (as per the guidance in JEP 12). The
>> JNI and JVMTI versions are bumped as this is the first change in 21 to need
>> the new version number. A lot of tests are updated to drop `@enablePreview`
>> and --enable-preview.
>>
>> There is one API change from Java 19/20, the preview API
>> Thread.Builder.allowSetThreadLocals(boolean) is dropped. This requires an
>> update to the JVMTI GetThreadInfo implementation to read the TCCL
>> consistently.
>>
>> In addition, there are a small number of implementation changes to sync up
>> from the loom fibers branch:
>>
>> - A number of stack frames are `@Hidden` to reduce noise in the stack
>> traces. This exposed a few issues with the stack walker code. More
>> specifically, the cases where end of a continuation falls precisely at the
>> end of the batch, or where the remaining frames are hidden, weren't handled
>> correctly.
>> - The code to emit the JFR jdk.ThreadSleepEvent is refactored so it's in
>> Thread rather than in two classes.
>> - A few robustness improvements for OOME and SOE. There is more to do here,
>> for future PRs.
>> - New system property to print a stack trace when a virtual thread sets its
>> own value of a TL.
>> - ThreadPerTaskExecutor is changed to use FutureTask.
>>
>> Testing: tier1-6.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Test updates
test/hotspot/jtreg/serviceability/jvmti/vthread/premain/AgentWithVThreadTest.java
line 40:
> 38: public static void main(String[] args) throws Exception {
> 39:
> 40: ProcessBuilder pb =
> ProcessTools.createTestJvm("-javaagent:agent.jar", "-version");
Suggestion:
ProcessBuilder pb = ProcessTools.createTestJvm("-javaagent:agent.jar",
"-version");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151504517
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v6]
On Wed, 15 Mar 2023 18:45:00 GMT, Matias Saavedra Silva
wrote:
>> The current structure used to store the resolution information for
>> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its
>> ambigious fields f1 and f2. This structure can hold information for fields,
>> methods, and invokedynamics and each of its fields can hold different types
>> of values depending on the entry.
>>
>> This enhancement proposes a new structure to exclusively contain
>> invokedynamic information in a manner that is easy to interpret and easy to
>> extend. Resolved invokedynamic entries will be stored in an array in the
>> constant pool cache and the operand of the invokedynamic bytecode will be
>> rewritten to be the index into this array.
>>
>> Any areas that previously accessed invokedynamic data from
>> ConstantPoolCacheEntry will be replaced with accesses to this new array and
>> structure. Verified with tier1-9 tests.
>>
>> The PPC was provided by @reinrich and the RISCV port was provided by
>> @DingliZhang and @zifeihan.
>>
>> This change supports the following platforms: x86, aarch64, PPC, and RISCV
>
> Matias Saavedra Silva has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fixed aarch64 interpreter mistake
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyEntry.java
line 49:
> 47:
> 48: private static synchronized void initialize(TypeDataBase db) throws
> WrongTypeException {
> 49: Type type= db.lookupType("ResolvedIndyEntry");
Suggestion:
Type type = db.lookupType("ResolvedIndyEntry");
-
PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v6]
On Fri, 17 Mar 2023 13:51:05 GMT, Frederic Parain wrote:
>> Please review this change re-implementing the FieldInfo data structure.
>>
>> The FieldInfo array is an old data structure storing fields metadata. It has
>> poor extension capabilities, a complex management code because of lack of
>> strong typing and semantic overloading, and a poor memory efficiency.
>>
>> The new implementation uses a compressed stream to store those metadata,
>> achieving better memory density and providing flexible extensibility, while
>> exposing a strongly typed set of data when uncompressed. The stream is
>> compressed using the unsigned5 encoding, which alreay present in the JDK
>> (because of pack200) and the JVM (because JIT compulers use it to comrpess
>> debugging information).
>>
>> More technical details are available in the CR:
>> https://bugs.openjdk.org/browse/JDK-8292818
>>
>> Those changes include a re-organisation of fields' flags, splitting the
>> previous heterogeneous AccessFlags field into three distincts flag
>> categories: immutable flags from the class file, immutable fields defined by
>> the JVM, and finally mutable flags defined by the JVM.
>>
>> The SA, CI, and JVMCI, which all used to access the old FieldInfo array,
>> have been updated too to deal with the new FieldInfo format.
>>
>> Tested with mach5, tier 1 to 7.
>>
>> Thank you.
>
> Frederic Parain has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Style fixes
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
line 268:
> 266:
> 267: Field getField(int index) {
> 268: synchronized(this) {
nit
Suggestion:
synchronized (this) {
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/jcore/ClassWriter.java
line 380:
> 378: dos.writeShort(accessFlags & (short)
> JVM_RECOGNIZED_FIELD_MODIFIERS);
> 379:
> 380: int nameIndex= klass.getFieldNameIndex(index);
nit:
Suggestion:
int nameIndex = klass.getFieldNameIndex(index);
-
PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8294977: Convert test/jdk/java tests from ASM library to Classfile API [v5]
On Thu, 16 Mar 2023 15:34:58 GMT, Chen Liang wrote:
>> Summaries:
>> 1. A few recommendations about updating the constant API is made at
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000233.html
>> and I may update this patch shall the API changes be integrated before
>> 2. One ASM library-specific test, `LambdaAsm` is removed. Others have their
>> code generation infrastructure upgraded from ASM to Classfile API.
>> 3. Most tests are included in tier1, but some are not:
>> In `:jdk_io`: (tier2, part 2)
>>
>> test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java
>> test/jdk/java/io/Serializable/records/ProhibitedMethods.java
>> test/jdk/java/io/Serializable/records/BadCanonicalCtrTest.java
>>
>> In `:jdk_instrument`: (tier 3)
>>
>> test/jdk/java/lang/instrument/RetransformAgent.java
>> test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java
>> test/jdk/java/lang/instrument/asmlib/Instrumentor.java
>>
>>
>> @asotona Would you mind reviewing?
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix LambdaStackTrace after running
test/jdk/java/lang/invoke/8022701/MHIllegalAccess.java line 50:
> 48: public class MHIllegalAccess {
> 49:
> 50:public static void main(String[] args) throws Throwable {
Suggestion:
public static void main(String[] args) throws Throwable {
-
PR: https://git.openjdk.org/jdk/pull/13009
Integrated: 8303690: Prefer ArrayList to LinkedList in com.sun.jmx.mbeanserver.Introspector
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- On Thu, 2 Mar 2023 20:16:35 GMT, Andrey Turbanov wrote: > `LinkedList` is used as value in > `com.sun.jmx.mbeanserver.Introspector.SimpleIntrospector#cache` > It's created, filled (with `add`) and then iterated. No removes from the head > or something like this. `ArrayList` should be preferred as more efficient and > widely used collection. > Also I've done some related code cleaned: > 1. removed redundand `if` from `SoftReference` value check > 2. fixed a typo in javadoc This pull request has now been integrated. Changeset: 1d071d08 Author:Andrey Turbanov URL: https://git.openjdk.org/jdk/commit/1d071d0817714ee2f1bd2af5f9556f7d268dd0fa Stats: 8 lines in 1 file changed: 1 ins; 3 del; 4 mod 8303690: Prefer ArrayList to LinkedList in com.sun.jmx.mbeanserver.Introspector Reviewed-by: stsypanov, kevinw, cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/12839
RFR: 8303690: Prefer ArrayList to LinkedList in com.sun.jmx.mbeanserver.Introspector
`LinkedList` is used as value in `com.sun.jmx.mbeanserver.Introspector.SimpleIntrospector#cache` It's created, filled (with `add`) and then iterated. No removes from the head or something like this. `ArrayList` should be preferred as more efficient and widely used collection. Also I've done some related code cleaned: 1. removed redundand `if` from `SoftReference` value check 2. fixed a typo in javadoc - Commit messages: - [PATCH] Prefer ArrayList to LinkedList in Introspector Changes: https://git.openjdk.org/jdk/pull/12839/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12839&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303690 Stats: 8 lines in 1 file changed: 1 ins; 3 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12839.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12839/head:pull/12839 PR: https://git.openjdk.org/jdk/pull/12839
Integrated: 8303267: Prefer ArrayList to LinkedList in ConcurrentLocksPrinter
On Mon, 27 Feb 2023 12:54:24 GMT, Andrey Turbanov wrote: > LinkedList is used as value in > `sun.jvm.hotspot.runtime.ConcurrentLocksPrinter#locksMap` Map. > There is only add/iterator calls on this lists. No removes from the head or > something like this. Not sure why LinkedList was used, but ArrayList should > be preferred as more efficient and widely used collection. > > Also I've done some related code cleaned: > 1. Mark field `locksMap` as final > 2. Use Map.computeIfAbsent > 3. Use enhanced-for cycle instead of `for` with iterator This pull request has now been integrated. Changeset: d4dcba04 Author:Andrey Turbanov URL: https://git.openjdk.org/jdk/commit/d4dcba04632f07555e4fe5547ee39125935a03c6 Stats: 10 lines in 1 file changed: 0 ins; 5 del; 5 mod 8303267: Prefer ArrayList to LinkedList in ConcurrentLocksPrinter Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/12763
RFR: 8303267: Prefer ArrayList to LinkedList in ConcurrentLocksPrinter
LinkedList is used as value in `sun.jvm.hotspot.runtime.ConcurrentLocksPrinter#locksMap` Map. There is only add/iterator calls on this lists. No removes from the head or something like this. Not sure why LinkedList was used, but ArrayList should be preferred as more efficient and widely used collection. Also I've done some related code cleaned: 1. Mark field `locksMap` as final 2. Use Map.computeIfAbsent 3. Use enhanced-for cycle instead of `for` with iterator - Commit messages: - [PATCH] Prefer ArrayList to LinkedList in ConcurrentLocksPrinter Changes: https://git.openjdk.org/jdk/pull/12763/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12763&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303267 Stats: 10 lines in 1 file changed: 0 ins; 5 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/12763.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12763/head:pull/12763 PR: https://git.openjdk.org/jdk/pull/12763
Re: RFR: 8299518: HotSpotVirtualMachine shared code across different platforms [v4]
On Mon, 9 Jan 2023 03:29:04 GMT, Yi Yang wrote:
>> harmless refactor to share code across different platforms of
>> VirtualMachineImpl:
>> 1. Shared code to process command response after requesting a command
>> execution
>> 2. Read functionality in SocketInputStream can be reused
>
> Yi Yang has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - describe parameter
> - Revert "separate renaming"
>
>This reverts commit cb70ac62fdfa605ed4f9e415c4ab73bfc8a3.
src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java line
422:
> 420: * InputStream for the socket connection to get target VM
> 421: */
> 422: static abstract class SocketInputStream extends InputStream {
Use blessed modifiers order
Suggestion:
abstract static class SocketInputStream extends InputStream {
-
PR: https://git.openjdk.org/jdk/pull/11823
Integrated: 8298090: Use String.join() instead of manual loop in DescriptorSupport.toString
On Wed, 2 Nov 2022 21:30:51 GMT, Andrey Turbanov wrote: > There is opportunity to use String.join in DescriptorSupport.toString > implementation. Result code is much shorter and clearer. This pull request has now been integrated. Changeset: d646e32b Author:Andrey Turbanov URL: https://git.openjdk.org/jdk/commit/d646e32b7a0f8e4a66f06e15e289d4ed70b8250e Stats: 9 lines in 1 file changed: 0 ins; 7 del; 2 mod 8298090: Use String.join() instead of manual loop in DescriptorSupport.toString Reviewed-by: stsypanov, sspitsyn, lmesnik - PR: https://git.openjdk.org/jdk/pull/10960
Re: RFR: 8298073: gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java causes test task timeout on macosx
On Thu, 8 Dec 2022 03:06:31 GMT, Chris Plummer wrote: > This fixes two separate CRs: > > [JDK-8241293](https://bugs.openjdk.org/browse/JDK-8241293) > CompressedClassSpaceSizeInJmapHeap.java time out after 8 minutes > [JDK-8298073](https://bugs.openjdk.org/browse/JDK-8298073) > gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java causes test task timeout > on macosx > > For the first one the test was hitting an 8m timeout, but completing right > after that. The test log said PASSED, but jtreg failed it anyway because it > noticed the timeout. The fix is to double the default 120s timeout to 240s > (which is 16m when the 4x timeoutfactor is applied. > > For the second one the issue seemed to be with having the test process spawn > a jhsdb process that attached back to the test process. OSX didn't seem to be > too happy about this. The fix is to instead create a LingeredApp process to > attach to like all the other SA tests do. test/hotspot/jtreg/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java line 53: > 51: SATestUtils.skipIfCannotAttach(); // throws SkippedException if > attach not expected to work. > 52: > 53: LingeredApp theApp = new LingeredApp(); Nit Suggestion: LingeredApp theApp = new LingeredApp(); - PR: https://git.openjdk.org/jdk/pull/11576
RFR: 8298090: Use String.join() instead of manual loop in DescriptorSupport.toString
There is opportunity to use String.join in DescriptorSupport.toString implementation. Result code is much shorter and clearer. - Commit messages: - [PATCH] Use String.join() instead of manual loop in DescriptorSupport Changes: https://git.openjdk.org/jdk/pull/10960/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10960&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8298090 Stats: 9 lines in 1 file changed: 0 ins; 7 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/10960.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10960/head:pull/10960 PR: https://git.openjdk.org/jdk/pull/10960
Re: RFR: 8294241: Deprecate URL public constructors
On Wed, 26 Oct 2022 16:00:56 GMT, Daniel Fuchs wrote:
> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
> to parse or construct any URL.
>
> The `java.net.URL` class does not itself encode or decode any URL components
> according to the escaping mechanism defined in RFC2396. It is the
> responsibility of the caller to encode any fields, which need to be escaped
> prior to calling URL, and also to decode any escaped fields, that are
> returned from URL.
>
> This has lead to many issues in the past. Indeed, if used improperly, there
> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
> URL string that can be parsed back into the same URL. This can lead to
> constructing misleading URLs. Another issue is with `equals()` and
> `hashCode()` which may have to perform a lookup, and do not take
> encoding/escaping into account.
>
> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
> of the shortcoming of `java.net.URL`. Conversion methods to create a URL from
> a URI were also added. However, it was left up to the developers to use
> `java.net.URI`, or not. This RFE proposes to deprecate all public
> constructors of `java.net.URL`, in order to provide a stronger warning about
> their potential misuses. To construct a URL, using `URI::toURL` should be
> preferred.
>
> In order to provide an alternative to the constructors that take a stream
> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
> java.net.URLStreamHandler)` is provided as part of this change.
>
> Places in the JDK code base that were constructing `java.net.URL` have been
> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
> issues will be logged to revisit the calling code.
>
> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
test/jdk/java/net/URL/URLFromURITest.java line 268:
> 266: // - URL authority is null or empty depending on the protocol
> 267: // and on whether the URL is hierarchical or not.
> 268: if (isFileBased && authority == null) {
Suggestion:
if (isFileBased && authority == null) {
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]
On Wed, 18 May 2022 18:14:52 GMT, Severin Gehwolf wrote: >> Please review this change to the cgroup v1 subsystem which makes it more >> resilient on some of the stranger systems. Unfortunately, I wasn't able to >> re-create a similar system as the reporter. The idea of using the longest >> substring match for the cgroupv1 file paths was based on the fact that on >> systemd systems processes run in separate scopes and the maven forked test >> runner might exhibit this property. For that it makes sense to use the >> common ancestor path. Nothing changes in the common cases where the >> `cgroup_path` matches `_root` and when the `_root` is `/` (container the >> former, host system the latter). >> >> In addition, the code paths which are susceptible to throw NPE have been >> hardened to catch those situations. Should it happen in the future it makes >> more sense (to me) to not have accurate container detection in favor of >> continuing to keep running. >> >> Finally, with the added unit-tests a bug was uncovered on the "substring" >> match case of cgroup paths in hotspot. `p` returned from `strstr` can never >> point to `_root` as it's used as the "needle" to find in "haystack" >> `cgroup_path` (not the other way round). >> >> Testing: >> - [x] Added unit tests >> - [x] GHA >> - [x] Container tests on cgroups v1 Linux. Continue to pass > > Severin Gehwolf has updated the pull request incrementally with two > additional commits since the last revision: > > - Refactor hotspot gtest > - Separate into function. Fix comment. test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java line 224: > 222: > "1:name=systemd:/user.slice/user-1000.slice/user@1000.service/apps.slice/apps-org.gnome.Terminal.slice/vte-spawn-3c00b338-5b65-439f-8e97-135e183d135d.scope\n" > + > 223: > "0::/user.slice/user-1000.slice/user@1000.service/apps.slice/apps-org.gnome.Terminal.slice/vte-spawn-3c00b338-5b65-439f-8e97-135e183d135d.scope\n"; > 224: private String cgroupv1SelfPrefixContent = > "9:memory:/user.slice/user-1000.slice/session-3.scope\n"; Let's remove one redundant space Suggestion: private String cgroupv1SelfPrefixContent = "9:memory:/user.slice/user-1000.slice/session-3.scope\n"; - PR: https://git.openjdk.org/jdk/pull/8629
Re: RFR: 8288387: GetLocalXXX/SetLocalXXX spec should require suspending target thread [v2]
On Thu, 6 Oct 2022 17:31:00 GMT, Serguei Spitsyn wrote:
>> The spec of JVM TI GetLocalXXX/SetLocalXXX functions is updated to require
>> the target thread to be suspended. If not suspended then the
>> JVMTI_ERROR_THREAD_NOT_SUSPENDED error code is returned by the
>> implementation.
>>
>> The CSR is: https://bugs.openjdk.org/browse/JDK-8294690
>>
>> A few tests are impacted by this fix:
>>
>> test/hotspot/jtreg/serviceability/jvmti/vthread/GetSetLocalTest
>> test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadTest
>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM01/cm01t011
>>
>>
>> The following test has been removed as non-relevant any more:
>> `
>> test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java`
>>
>> New negative test has been added instead:
>> `
>> test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetSetLocalUnsuspended.java`
>>
>> All JVM TI and JPDA tests were used locally for verification.
>> They were also run in Loom repository with `JTREG_MAIN_WRAPPER=Virtual`.
>>
>> Mach5 test runs on all platforms are TBD.
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> addressed review comments about is_JavaThread_current and @enablePreview tag
test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetSetLocalUnsuspended.java
line 39:
> 37: static native void testUnsuspendedThread(Thread thread);
> 38:
> 39: static private volatile boolean doStop;
Let's use default modifiers order
Suggestion:
private static volatile boolean doStop;
test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetSetLocalUnsuspended.java
line 41:
> 39: static private volatile boolean doStop;
> 40:
> 41: static private void sleep(long millis) {
Let's use default modifiers order
Suggestion:
private static void sleep(long millis) {
-
PR: https://git.openjdk.org/jdk/pull/10586
Re: RFR: 8289711: Add container configuration data to mbeans [v10]
On Wed, 10 Aug 2022 02:10:25 GMT, xpbob wrote:
>> Container configuration information is useful for troubleshooting
>> problems,Exposing information in MBeans is ideal for monitoring, jConsole,
>> and other scenarios.
>> Results the following
>> 
>
> xpbob has updated the pull request incrementally with one additional commit
> since the last revision:
>
> add bean to list
src/jdk.management/share/classes/com/sun/management/ContainerInfoMXBean.java
line 31:
> 29: * This is a special bean , only available on Linux systems
> 30: */
> 31: public interface ContainerInfoMXBean extends PlatformManagedObject {
let's remove one redundant space before `extends`
-
PR: https://git.openjdk.org/jdk/pull/9372
Integrated: 8293879: Remove unnecessary castings in jdk.hotspot.agent
On Thu, 15 Sep 2022 20:49:14 GMT, Andrey Turbanov wrote: > Redundant castings make code harder to read. > Found them by IntelliJ IDEA. > I tried to choose only casts which are definitely safe to remove. > Most generification was done in > [JDK-8241618](https://bugs.openjdk.org/browse/JDK-8241618), but casts weren't > removed. This pull request has now been integrated. Changeset: 5725a93c Author:Andrey Turbanov URL: https://git.openjdk.org/jdk/commit/5725a93c078dac9775ccef04f3624647a8d38e83 Stats: 261 lines in 75 files changed: 0 ins; 8 del; 253 mod 8293879: Remove unnecessary castings in jdk.hotspot.agent Reviewed-by: lmesnik, cjplummer - PR: https://git.openjdk.org/jdk/pull/10295
Re: RFR: 8293879: Remove unnecessary castings in jdk.hotspot.agent
On Thu, 15 Sep 2022 20:49:14 GMT, Andrey Turbanov wrote: > Redundant castings make code harder to read. > Found them by IntelliJ IDEA. > I tried to choose only casts which are definitely safe to remove. > Most generification was done in > [JDK-8241618](https://bugs.openjdk.org/browse/JDK-8241618), but casts weren't > removed. Thank you for review! - PR: https://git.openjdk.org/jdk/pull/10295
Re: RFR: 8293879: Remove unnecessary castings in jdk.hotspot.agent
On Thu, 15 Sep 2022 22:27:25 GMT, Chris Plummer wrote:
>> Redundant castings make code harder to read.
>> Found them by IntelliJ IDEA.
>> I tried to choose only casts which are definitely safe to remove.
>> Most generification was done in
>> [JDK-8241618](https://bugs.openjdk.org/browse/JDK-8241618), but casts
>> weren't removed.
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/shared/GenCollectedHeap.java
> line 134:
>
>> 132: } else {
>> 133: return VMObjectFactory.newObject(GenerationSpec.class,
>> 134: oldGenSpecField.getAddress());
>
> Maybe make each of these one line instead of two.
Other lines in this file don't exceed 80 chars. So, I think it's better to
preserver line break here.
-
PR: https://git.openjdk.org/jdk/pull/10295
RFR: 8293879: Remove unnecessary castings in jdk.hotspot.agent
Redundant castings make code harder to read. Found them by IntelliJ IDEA. I tried to choose only casts which are definitely safe to remove. Most generification was done in [JDK-8241618](https://bugs.openjdk.org/browse/JDK-8241618), but casts weren't removed. - Commit messages: - [PATCH] Remove unnecessary castings in jdk.hotspot.agent Changes: https://git.openjdk.org/jdk/pull/10295/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10295&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8293879 Stats: 261 lines in 75 files changed: 0 ins; 8 del; 253 mod Patch: https://git.openjdk.org/jdk/pull/10295.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10295/head:pull/10295 PR: https://git.openjdk.org/jdk/pull/10295
Integrated: 8293647: Avoid unnecessary boxing in jdk.hotspot.agent
On Mon, 12 Sep 2022 12:35:30 GMT, Andrey Turbanov wrote: > Usages of methods `Integer.valueOf`, `Boolean.valueOf`, `Long.valueOf` often > can be simplified by using their pair methods > `parseInt`/`parseBoolean`/`parseLong`. This pull request has now been integrated. Changeset: 7e020398 Author: Andrey Turbanov URL: https://git.openjdk.org/jdk/commit/7e0203980582c47e53f8851998138e13913bd28a Stats: 16 lines in 3 files changed: 0 ins; 0 del; 16 mod 8293647: Avoid unnecessary boxing in jdk.hotspot.agent Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/10237
RFR: 8293647: Avoid unnecessary boxing in jdk.hotspot.agent
Usages of methods `Integer.valueOf`, `Boolean.valueOf`, `Long.valueOf` often can be simplified by using their pair methods `parseInt`/`parseBoolean`/`parseLong`. - Commit messages: - 8293647: Avoid unnecessary boxing in jdk.hotspot.agent - [PATCH] Avoid unnecessary boxing in jdk.hotspot.agent Changes: https://git.openjdk.org/jdk/pull/10237/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10237&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8293647 Stats: 16 lines in 3 files changed: 0 ins; 0 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/10237.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10237/head:pull/10237 PR: https://git.openjdk.org/jdk/pull/10237
Integrated: 8293432: Use diamond operator in java.management
On Mon, 5 Sep 2022 19:56:44 GMT, Andrey Turbanov wrote: > The diamond operator was introduced in Java 7. We can take advantage of this > language feature to make code easier to read. > Tested on Linux release x64. `make test TEST="jdk/java/lang/management > jdk/javax/management jdk/com/sun/jmx jdk/sun/management"` > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/java/lang/management 6969 0 0 > >jtreg:test/jdk/javax/management 246 246 0 0 > >jtreg:test/jdk/com/sun/jmx3 3 0 0 > >>> jtreg:test/jdk/sun/management3332 0 1 << > > Single failure - is a test `RmiBootstrapTest.java#id1`. It's known issue - > [JDK-8293335](https://bugs.openjdk.org/browse/JDK-8293335). This pull request has now been integrated. Changeset: 98da03af Author:Andrey Turbanov URL: https://git.openjdk.org/jdk/commit/98da03af50e2372817a7b5e381eea5ee6f2cb919 Stats: 363 lines in 61 files changed: 2 ins; 72 del; 289 mod 8293432: Use diamond operator in java.management Reviewed-by: rriggs, sspitsyn - PR: https://git.openjdk.org/jdk/pull/10173
Re: RFR: 8293432: Use diamond operator in java.management
On Mon, 5 Sep 2022 19:56:44 GMT, Andrey Turbanov wrote: > The diamond operator was introduced in Java 7. We can take advantage of this > language feature to make code easier to read. > Tested on Linux release x64. `make test TEST="jdk/java/lang/management > jdk/javax/management jdk/com/sun/jmx jdk/sun/management"` > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/java/lang/management 6969 0 0 > >jtreg:test/jdk/javax/management 246 246 0 0 > >jtreg:test/jdk/com/sun/jmx3 3 0 0 > >>> jtreg:test/jdk/sun/management3332 0 1 << > > Single failure - is a test `RmiBootstrapTest.java#id1`. It's known issue - > [JDK-8293335](https://bugs.openjdk.org/browse/JDK-8293335). Thank you for review! - PR: https://git.openjdk.org/jdk/pull/10173
Re: RFR: 8293432: Use diamond operator in java.management
On Mon, 5 Sep 2022 19:56:44 GMT, Andrey Turbanov wrote:
> The diamond operator was introduced in Java 7. We can take advantage of this
> language feature to make code easier to read.
> Tested on Linux release x64. `make test TEST="jdk/java/lang/management
> jdk/javax/management jdk/com/sun/jmx jdk/sun/management"`
>
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>jtreg:test/jdk/java/lang/management 6969 0 0
>
>jtreg:test/jdk/javax/management 246 246 0 0
>
>jtreg:test/jdk/com/sun/jmx3 3 0 0
>
>>> jtreg:test/jdk/sun/management3332 0 1 <<
>
> Single failure - is a test `RmiBootstrapTest.java#id1`. It's known issue -
> [JDK-8293335](https://bugs.openjdk.org/browse/JDK-8293335).
src/java.management/share/classes/com/sun/jmx/mbeanserver/ClassLoaderRepositorySupport.java
line 89:
> 87: **/
> 88: private synchronized boolean add(ObjectName name, ClassLoader cl) {
> 89: List l = new ArrayList<>(Arrays.asList(loaders));
I found it strange that ArrayList is used to just append one element to array.
I think it's better to just use Arrays.copyOf with length+1.
-
PR: https://git.openjdk.org/jdk/pull/10173
RFR: 8293432: Use diamond operator in java.management
The diamond operator was introduced in Java 7. We can take advantage of this language feature to make code easier to read. Tested on Linux release x64. `make test TEST="jdk/java/lang/management jdk/javax/management jdk/com/sun/jmx jdk/sun/management"` Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/jdk/java/lang/management 6969 0 0 jtreg:test/jdk/javax/management 246 246 0 0 jtreg:test/jdk/com/sun/jmx3 3 0 0 >> jtreg:test/jdk/sun/management3332 0 1 << Single failure - is a test `RmiBootstrapTest.java#id1`. It's known issue - [JDK-8293335](https://bugs.openjdk.org/browse/JDK-8293335). - Commit messages: - [PATCH] Use diamond operator in java.management Changes: https://git.openjdk.org/jdk/pull/10173/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10173&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8293432 Stats: 363 lines in 61 files changed: 2 ins; 72 del; 289 mod Patch: https://git.openjdk.org/jdk/pull/10173.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10173/head:pull/10173 PR: https://git.openjdk.org/jdk/pull/10173
Integrated: 8292350: Use static methods for hashCode/toString primitives
On Wed, 10 Aug 2022 08:01:41 GMT, Andrey Turbanov wrote: > It's a bit shorter and clearer. This pull request has now been integrated. Changeset: 37c0a136 Author: Andrey Turbanov URL: https://git.openjdk.org/jdk/commit/37c0a13647e74fd02823a3f621e986f96904b933 Stats: 8 lines in 4 files changed: 0 ins; 0 del; 8 mod 8292350: Use static methods for hashCode/toString primitives Reviewed-by: prr, rriggs, amenkov, jpai - PR: https://git.openjdk.org/jdk/pull/9816
RFR: 8292350: Use static methods for hashCode/toString primitives
It's a bit shorter and clearer. - Commit messages: - [PATCH] Use static methods for hashCode/toString primitives Changes: https://git.openjdk.org/jdk/pull/9816/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=9816&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8292350 Stats: 8 lines in 4 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/9816.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9816/head:pull/9816 PR: https://git.openjdk.org/jdk/pull/9816
Integrated: 8289126: Cleanup unnecessary null comparison before instanceof check in jdk.hotspot.agent
On Fri, 24 Jun 2022 09:19:33 GMT, Andrey Turbanov wrote:
> Update code checks both non-null and instance of a class in jdk.hotspot.agent
> module classes.
> The checks and explicit casts could also be replaced with pattern matching
> for the instanceof operator.
>
> For example, the following code:
>
> Object node = tree.getLastSelectedPathComponent();
> if (node != null && node instanceof SimpleTreeNode) {
> showInspector((SimpleTreeNode)node);
> }
>
> Can be simplified to:
>
> Object node = tree.getLastSelectedPathComponent();
> if (node instanceof SimpleTreeNode simpleNode) {
> showInspector(simpleNode);
> }
>
>
> See similar cleanup in java.base -
> [JDK-8258422](https://bugs.openjdk.java.net/browse/JDK-8258422)
This pull request has now been integrated.
Changeset: 7905788e
Author:Andrey Turbanov
URL:
https://git.openjdk.org/jdk/commit/7905788e969727c81eea4397f0d9b918cdb5286a
Stats: 62 lines in 18 files changed: 0 ins; 13 del; 49 mod
8289126: Cleanup unnecessary null comparison before instanceof check in
jdk.hotspot.agent
Reviewed-by: ayang, cjplummer
-
PR: https://git.openjdk.org/jdk/pull/9272
Re: RFR: 8289126: Cleanup unnecessary null comparison before instanceof check in jdk.hotspot.agent
On Fri, 24 Jun 2022 18:15:26 GMT, Chris Plummer wrote: >I noticed you have 3 tier1 JDI tests timing out on all platforms. I don't see >this in another PR I just reviewed. I know you didn't change JDI, but am >wondering why this is happening in your PR. See [JDK-8289129](https://bugs.openjdk.org/browse/JDK-8289129): [JDK-8287281](https://bugs.openjdk.org/browse/JDK-8287281) is the cause. - PR: https://git.openjdk.org/jdk/pull/9272
RFR: 8289126: Cleanup unnecessary null comparison before instanceof check in jdk.hotspot.agent
Update code checks both non-null and instance of a class in jdk.hotspot.agent
module classes.
The checks and explicit casts could also be replaced with pattern matching for
the instanceof operator.
For example, the following code:
Object node = tree.getLastSelectedPathComponent();
if (node != null && node instanceof SimpleTreeNode) {
showInspector((SimpleTreeNode)node);
}
Can be simplified to:
Object node = tree.getLastSelectedPathComponent();
if (node instanceof SimpleTreeNode simpleNode) {
showInspector(simpleNode);
}
See similar cleanup in java.base -
[JDK-8258422](https://bugs.openjdk.java.net/browse/JDK-8258422)
-
Commit messages:
- 8289126: Cleanup unnecessary null comparison before instanceof check in
jdk.hotspot.agent
Changes: https://git.openjdk.org/jdk/pull/9272/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=9272&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8289126
Stats: 62 lines in 18 files changed: 0 ins; 13 del; 49 mod
Patch: https://git.openjdk.org/jdk/pull/9272.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/9272/head:pull/9272
PR: https://git.openjdk.org/jdk/pull/9272
