On Mon, 19 Aug 2024 15:39:02 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:

> The `ClassLoadingMXBean` and `MemoryMXBean` APIs have `setVerbose` methods to 
> control verbose mode and `isVerbose` methods to query it. Some JCK tests 
> expect `setVerbose(false)` to disable verbose mode and, subsequently, 
> `isVerbose()` to return false. However, if logging to a file is enabled by 
> using -Xlog on the java launcher command line then `isVerbose()` returns true 
> even after calling `setVerbose(false)`.
> 
> The proposed patch solves this by introducing two changes:
> 
> 1) The previous implementation used the `log_is_enabled` functionality to 
> check if logging was enabled for the given tag set. This returns true if 
> logging has been turned on for *any* output. The patch changes this so that 
> `isVerbose` only checks what has been configured for stdout, which is the 
> output that `setVerbose` configures.
> 
> 2) The previous implementation of `setVerbose` turned on `class+load*` 
> (notice the star) but then `isVerbose` only checked `class+load` (without the 
> star). The patch changes this so that the `isVerbose` in-effect checks 
> `class+load*`. (The `gc` part of the patch did not have this problem)
> 
> The main focus on this patch is to fix the JCK failure, with an 
> implementation that follows the API documentation. While looking at this area 
> of the code it is clear that there are other problems that we might want to 
> addressed in the future, but we're intentionally keeping this patch limited 
> in scope so that it can be backported to JDK 23.
> 
> A CSR for this change has been created.
> 
> Testing:
> * The newly implemented tests
> * The failing JCK tests with the corresponding -Xlog lines
> * Tier1-7 (running)
> 
> The patch is co-authored by me and David Holmes

Thumbs up.

src/hotspot/share/services/memoryService.cpp line 218:

> 216: 
> 217:   return false;
> 218: }

If we have `-Xlog:gc` and `-Xlog:gc+foo` set I think this version of
`MemoryService::get_verbose()` will return `false`. Is that really
what we want?

test/jdk/java/lang/management/ClassLoadingMXBean/TestVerboseClassLoading.java 
line 36:

> 34:  * @run main/othervm -Xlog:class+load=trace TestVerboseClassLoading false
> 35:  * @run main/othervm -Xlog:class+load=debug TestVerboseClassLoading false
> 36:  * @run main/othervm -Xlog:class+load=info TestVerboseClassLoading false

Hmm... I was expecting these to be `true`. What am I missing?

test/jdk/java/lang/management/ClassLoadingMXBean/TestVerboseClassLoading.java 
line 56:

> 54:  */
> 55: 
> 56: import java.lang.management.*;

I thought we tried to avoid wild-card imports.

test/jdk/java/lang/management/MemoryMXBean/TestVerboseMemory.java line 50:

> 48:  */
> 49: 
> 50: import java.lang.management.*;

I thought we tried to avoid wild-card imports.

-------------

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20628#pullrequestreview-2245990507
PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722060516
PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722064641
PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722072308
PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722072710

Reply via email to