Immutable leak detection level #13459

franz1981 · 2023-06-22T10:06:15Z

Motivation:

Unstrusted fields from the JDK perspective are more costy and subsequent decisions cannot be constant folded

Modifications:

Create a new sys property to set it up

Result:

Better performance and likely smaller native images for DISABLED cases

franz1981 · 2023-06-22T10:08:36Z

I've created a small benchmark to show the benefits (not transitive ones eg constant fold of decisions) of this at franz1981/java-puzzles@20a9ceb

which got on

VM version: JDK 20.0.1, OpenJDK 64-Bit Server VM, 20.0.1+9

Benchmark                             Mode  Cnt           Score          Error  Units
EnumSwitch.decorateStable            thrpt   10   930543469.666 ± 13829443.763  ops/s
EnumSwitch.decorateStableFastPath    thrpt   10  1276226193.380 ± 13865208.113  ops/s
EnumSwitch.decorateUnstable          thrpt   10   734376175.307 ±  3692811.792  ops/s
EnumSwitch.decorateUnstableFastPath  thrpt   10   857378668.982 ±  8328131.819  ops/s

which show that the changes to allow a fast path on DISABLE always pay off but in the "stable" ie immutable level case it pays a lot.

normanmaurer · 2023-06-22T10:08:22Z

common/src/main/java/io/netty/util/ResourceLeakDetector.java

@@ -150,13 +158,19 @@ public static boolean isEnabled() {
     * Sets the resource leak detection level.
     */
    public static void setLevel(Level level) {
+        if (stableLevel != null) {
+            return;


I think we should better throw in this case.

a stackless Unsupported Ex?
I'm saying that because maybe users don't want to have some special paths in their code able to set the level at runtime to handle the case when the level is "immutable"

not sure what is better... At least it wouldn't be surprising in a sense that while they think they use paranoid (for example) it not actually does.

Me neither, be silent doesn't seem right, I agree...a warn?

I have noticed https://github.com/eclipse-vertx/vert.x/blob/6a8aefb6ec84278774d2c03fdae83fdf1deafded/src/main/java/io/vertx/core/impl/VertxImpl.java#L104: it means that if I won't expose something public to check if the immutable sys prop is there, with an exception, this one will crash the application while starting

or...

I just add a warn?

Any suggestion @normanmaurer ? do you want me to add an exception for this?

I am not sure ... @chrisvest @trustin thoughts ?

Thinking out loud, what about:

Throwing an unsupported exception if the new level is different than the immutable one.

Always require io.netty.leakDetection.level to be set when setting io.netty.immutableResourceLeakDetection. This way we avoid the vert.x case.

buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.java

common/src/main/java/io/netty/util/ResourceLeakDetector.java

franz1981 · 2023-06-22T11:22:41Z

Note on the DISABLE fast path: why don't let the JIT do it?
Current JIT would profile the switch and extract (via a guard) the branch that's always met, adding an uncommon trap
in case something different is observed.
So, although the level is stable/immutable:

JIT would run in C1/interpreted profiling the frequencies of matched conditions
the final fast path would include an unwelcome guard, absent otherwise (by extracting manually the fastest path we care the most)

common/src/main/java/io/netty/util/ResourceLeakDetector.java

Motivation: Unstrusted fields from the JDK perspective are more costy and subsequent decisions cannot be constant folded Modifications: Create a new sys property to set it up Result: Better performance and likely smaller native images for DISABLED cases

franz1981 · 2023-06-22T12:16:20Z

@normanmaurer I've added a new hint isImmutableDisabled (bad name I know) which can be used to manually propagate the static final decision at instance field level (by not allocating specific types and make obvious the recheability of methods and types, no needed in case of disabled level known at compile time).

This lead me to modify the default channel pipeline to do the same: instead of using an internal field touch it relies on the runtime decision, but it means that, if the level is not stable at runtime, the same pipeline can start not tracking anymore buffer(s) (no idea it wasn't like that before, maybe you have some historical data point or @trustin)

geoand · 2023-06-22T13:02:04Z

Just wanted to add, that I've talked with Franz about how he can check what kind of effect this has on a GraalVM native-image for Quarkus - I do expect it to have some small effect since the Graal compiler will theoretically be able to do more folding of the static part of the class(es) (which we do enable in Quarkus)

transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java

hyperxpro · 2023-07-10T17:21:41Z

With the growing number of props, should we create a dedicated wiki page for props and their definition?

franz1981 · 2024-01-23T11:03:51Z

@zakkak I would like your opinion on this, related potential improvements we could get on native image or if we have other options which doesn't require it

zakkak · 2024-01-23T12:38:00Z

common/src/main/java/io/netty/util/ResourceLeakDetector.java


    private static final InternalLogger logger = InternalLoggerFactory.getInstance(ResourceLeakDetector.class);

    static {
+        final boolean immutableLevel = SystemPropertyUtil.get("io.netty.immutableResourceLeakDetection") != null;


I suggest changing the property name and explicitly treating it as a boolean:

Suggested change

final boolean immutableLevel = SystemPropertyUtil.get("io.netty.immutableResourceLeakDetection") != null;

final boolean immutableLevel = Boolean.getBoolean("io.netty.leakDetection.level.immutable");

but use SystemPropertyUtil.getBoolean() instead of Boolean.getBoolean()

zakkak · 2024-01-23T12:38:34Z

common/src/main/java/io/netty/util/ResourceLeakDetector.java

@@ -97,10 +97,12 @@ static Level parseLevel(String levelStr) {
    }

    private static Level level;
+    private static final Level stableLevel;


Maybe call it immutableLevel?

zakkak · 2024-01-23T12:41:30Z

common/src/main/java/io/netty/util/ResourceLeakDetector.java

@@ -139,33 +147,43 @@ public static void setEnabled(boolean enabled) {
        setLevel(enabled? Level.SIMPLE : Level.DISABLED);
    }

+    private static boolean isImmutableDisabled() {


Might be slightly better:

Suggested change

private static boolean isImmutableDisabled() {

private static boolean isImmutablyDisabled() {

zakkak · 2024-01-23T12:47:46Z

@zakkak I would like your opinion on this, related potential improvements we could get on native image

Currently Quarkus sets io.netty.leakDetection.level to DISABLED when building a native-image (AFAIK there is no way to override this). So I would expect it to also set io.netty.immutableResourceLeakDetection if this PR eventually gets merged.

In theory (you could confirm it by setting app a small Quarkus test and observing the generated binary size, as well as the number of reachable classes/methods) this should suffice, and would give you the benefits you expect (i.e. the static analysis will understand that you are disabling the leak detection and throw away all relevant parts as dead code).

or if we have other options which doesn't require it

AFAIK this (i.e., guarding dead code inside if-else statements with conditions that are final and initialized at build time) is one of the best ways to go.

zakkak · 2024-01-23T13:02:56Z

common/src/main/java/io/netty/util/ResourceLeakDetector.java

-    private final Set<DefaultResourceLeak<?>> allLeaks =
+    private final Set<DefaultResourceLeak<?>> allLeaks = isImmutableDisabled()? null :
            Collections.newSetFromMap(new ConcurrentHashMap<DefaultResourceLeak<?>, Boolean>());

-    private final ReferenceQueue<Object> refQueue = new ReferenceQueue<Object>();
-    private final Set<String> reportedLeaks =
+    private final ReferenceQueue<Object> refQueue = isImmutableDisabled()? null : new ReferenceQueue<Object>();
+    private final Set<String> reportedLeaks = isImmutableDisabled()? null :


Don't these changes break trackForcibly?

normanmaurer · 2024-02-21T09:36:50Z

@franz1981 I am still not convinced we need to add this extra complexity for the relativ small win... wdyt ?

franz1981 · 2024-02-25T20:27:18Z

@normanmaurer

I have yet to address the changes as suggested by @zakkak and verify what happen on native image; I have some gut feeling that for native image this could reduce memory footprint (which depends on recheability) quite a bit.

franz1981 self-assigned this Jun 22, 2023

franz1981 requested review from normanmaurer and chrisvest June 22, 2023 10:06

normanmaurer requested changes Jun 22, 2023

View reviewed changes

franz1981 force-pushed the 4.1_stable_leak_detector branch from a9ba703 to d30af65 Compare June 22, 2023 11:57

franz1981 commented Jun 22, 2023

View reviewed changes

common/src/main/java/io/netty/util/ResourceLeakDetector.java Show resolved Hide resolved

Immutable leak detection level

cd0f03b

Motivation: Unstrusted fields from the JDK perspective are more costy and subsequent decisions cannot be constant folded Modifications: Create a new sys property to set it up Result: Better performance and likely smaller native images for DISABLED cases

franz1981 force-pushed the 4.1_stable_leak_detector branch from 4b00b09 to cd0f03b Compare June 22, 2023 12:13

franz1981 mentioned this pull request Jun 22, 2023

Make Netty leak detection level immutable eclipse-vertx/vert.x#4753

Open

chrisvest reviewed Jun 22, 2023

View reviewed changes

transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java Show resolved Hide resolved

zakkak reviewed Jan 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Immutable leak detection level #13459

Immutable leak detection level #13459

franz1981 commented Jun 22, 2023

franz1981 commented Jun 22, 2023 •

edited

normanmaurer Jun 22, 2023

franz1981 Jun 22, 2023 •

edited

normanmaurer Jun 22, 2023

franz1981 Jun 22, 2023

franz1981 Jun 22, 2023 •

edited

franz1981 Jun 29, 2023

normanmaurer Jul 24, 2023

zakkak Jan 23, 2024 •

edited

franz1981 commented Jun 22, 2023

franz1981 commented Jun 22, 2023

geoand commented Jun 22, 2023

hyperxpro commented Jul 10, 2023

franz1981 commented Jan 23, 2024

zakkak Jan 23, 2024

chrisvest Jan 23, 2024

zakkak Jan 23, 2024

zakkak Jan 23, 2024

zakkak commented Jan 23, 2024 •

edited

zakkak Jan 23, 2024

normanmaurer commented Feb 21, 2024

franz1981 commented Feb 25, 2024 •

edited

	final boolean immutableLevel = SystemPropertyUtil.get("io.netty.immutableResourceLeakDetection") != null;
	final boolean immutableLevel = Boolean.getBoolean("io.netty.leakDetection.level.immutable");

	private static boolean isImmutableDisabled() {
	private static boolean isImmutablyDisabled() {

Immutable leak detection level #13459

Are you sure you want to change the base?

Immutable leak detection level #13459

Conversation

franz1981 commented Jun 22, 2023

franz1981 commented Jun 22, 2023 • edited

Choose a reason for hiding this comment

franz1981 Jun 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franz1981 Jun 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakkak Jan 23, 2024 • edited

Choose a reason for hiding this comment

franz1981 commented Jun 22, 2023

franz1981 commented Jun 22, 2023

geoand commented Jun 22, 2023

hyperxpro commented Jul 10, 2023

franz1981 commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakkak commented Jan 23, 2024 • edited

Choose a reason for hiding this comment

normanmaurer commented Feb 21, 2024

franz1981 commented Feb 25, 2024 • edited

franz1981 commented Jun 22, 2023 •

edited

franz1981 Jun 22, 2023 •

edited

franz1981 Jun 22, 2023 •

edited

zakkak Jan 23, 2024 •

edited

zakkak commented Jan 23, 2024 •

edited

franz1981 commented Feb 25, 2024 •

edited