8359119: Change Charset to use StableValue #25727

minborg · 2025-06-10T14:09:03Z

Fields and methods can better leverage the Stable Value API compared to using DCL and holder classes. There are also some fields that can be marked @Stable.

This PR passes tier1, tier2, and tier3 tests on multiple platforms.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8359119: Change Charset to use StableValue (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25727/head:pull/25727
$ git checkout pull/25727

Update a local copy of the PR:
$ git checkout pull/25727
$ git pull https://git.openjdk.org/jdk.git pull/25727/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25727

View PR using the GUI difftool:
$ git pr show -t 25727

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25727.diff

Using Webrev

Link to Webrev Comment

src/java.base/share/classes/java/nio/charset/Charset.java

bridgekeeper · 2025-06-10T14:10:22Z

👋 Welcome back pminborg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-06-10T14:10:53Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-06-10T14:11:29Z

@minborg The following label will be automatically applied to this pull request:

nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

src/java.base/share/classes/java/nio/charset/Charset.java

mlbridge · 2025-06-10T14:17:58Z

Webrevs

01: Full - Incremental (21a43faa)
00: Full (57edd190)

minborg · 2025-06-10T14:51:16Z

Current startup performance on an M1 macOS using brr -b perfstartup-noop.

Base:

Wall clock:       29.5 ms/op
  :.cycles:       100702748 average cycles elapsed
  :.instructions: 196311919 average instructions retired
  :.maxrss:       39100416 bytes maximum resident set size
  :.taskclock:    23.0 ms/op

Patch:

Wall clock:       29.0 ms/op
  :.cycles:       101696210 average cycles elapsed
  :.instructions: 196761541 average instructions retired
  :.maxrss:       39067648 bytes maximum resident set size
  :.taskclock:    22.5 ms/op

AlanBateman · 2025-06-10T15:04:44Z

src/java.base/share/classes/java/nio/charset/Charset.java

-        static final ThreadTracker TRACKER = new ThreadTracker();
-    }
+    private static final Supplier<ThreadTracker> TRACKER = StableValue.supplier(
+            new Supplier<>() { public ThreadTracker get() { return new ThreadTracker(); }});


ThreadTracker pre-dates ScopedValue. We could potentially use a ScopedValue<Boolean> IN_LOOKUP here, or StableValue<ScopedValue<Boolean>> if there are issues running the class initializer in early VM startup. Separate discussion but we don't need ThreadTracker now.

So, revert the changes here?

I think it's okay, it's just that the change is a reminder that all ThreadTracker usages can be replaced with a ScopedValue but may require a bit of work to allow using during early VM startup.

I chatted with Andrew Haley yesterday about allowing ScopedValue be used in early startup. Charset is initialized very early in initPhase1, when initializing the system properties, so easy to get recursive initialization issues that surface or NPE or other exceptions. There are several ways we can fix this and it means that TRACKER is not needed. I don't mind if we change it to use a StableValue now but I expect we will change it very soon to remove ThreadTracker and just replace with ScopedValue.

The issue with using ScopedValue early in startup is resolved so we can use private static final ScopedValue<Boolean> IN_LOOKUP = ScopedValue.newInstance(); and get rid of the use of ThreadTracker from this class.

AlanBateman · 2025-06-12T11:07:48Z

src/java.base/share/classes/java/nio/charset/Charset.java

-            int n = 0;
-            ServiceLoader<CharsetProvider> sl =
+    private static final Supplier<List<CharsetProvider>> EXTENDED_PROVIDERS = StableValue.supplier(
+            new Supplier<>() { public List<CharsetProvider> get() { return extendedProviders0(); }});


This one looks okay.

AlanBateman · 2025-06-12T11:08:59Z

src/java.base/share/classes/java/nio/charset/Charset.java

+    private static Charset defaultCharset0() {
+        return Objects.requireNonNullElse(
+                standardProvider.charsetForName(StaticProperty.fileEncoding()),
+                sun.nio.cs.UTF_8.INSTANCE);


I think it would more readable to keep the if-then-else rather than using requireNonNullElse here.

AlanBateman · 2025-06-12T11:10:20Z

src/java.base/share/classes/java/nio/charset/Charset.java

    private final String name;          // tickles a bug in oldjavac
+    @Stable


If we touching these then we can remove the "tickles a bug in oldjavac" comment as I think that goes back to before JDK 5.

AlanBateman · 2025-06-12T11:11:44Z

Overall I think this is okay but will need to run startup benchmarks due to this class being used in early startup.

wenshao · 2025-07-20T22:14:37Z

After this proposal, the startup will load 9 more classes

Java Code

public class Startup {
    public static void main(String[] args) throws Exception {
        Thread.sleep(1000);
    }
}

./build/macosx-aarch64-server-release/images/jdk/bin/java -verbose:class Startup

By comparing the output before and after this proposal, we can see that 9 more classes are loaded, as follows:

[info][class,load] java.nio.charset.Charset$2
[info][class,load] java.lang.StableValue
[info][class,load] java.util.Objects
[info][class,load] jdk.internal.lang.stable.StableSupplier
[info][class,load] jdk.internal.lang.stable.StableValueImpl
[info][class,load] java.nio.charset.Charset$3
[info][class,load] java.nio.charset.Charset$4
[info][class,load] java.nio.charset.Charset$5
[info][class,load] jdk.internal.ref.Cleaner

Add stable value functionality

57edd19

minborg commented Jun 10, 2025

View reviewed changes

src/java.base/share/classes/java/nio/charset/Charset.java Outdated Show resolved Hide resolved

openjdk bot added the nio nio-dev@openjdk.org label Jun 10, 2025

minborg commented Jun 10, 2025

View reviewed changes

src/java.base/share/classes/java/nio/charset/Charset.java Outdated Show resolved Hide resolved

minborg commented Jun 10, 2025

View reviewed changes

src/java.base/share/classes/java/nio/charset/Charset.java Outdated Show resolved Hide resolved

minborg marked this pull request as ready for review June 10, 2025 14:13

openjdk bot added the rfr Pull request is ready for review label Jun 10, 2025

minborg changed the title ~~8359119: There are several opportunities to use Stable Values in j.n.c.Charset~~ 8359119: Change Charset to use StableValue Jun 10, 2025

AlanBateman reviewed Jun 10, 2025

View reviewed changes

minborg added 3 commits June 10, 2025 17:10

Revert changes

f89e3f6

Revert more

c9ce11d

Revert for loop

21a43fa

AlanBateman reviewed Jun 12, 2025

View reviewed changes

wenshao mentioned this pull request Jul 20, 2025

Add @Stable to LocaleProviderAdapter #26354

Draft

3 tasks

		private final String name; // tickles a bug in oldjavac
		@Stable

8359119: Change Charset to use StableValue #25727

Are you sure you want to change the base?

8359119: Change Charset to use StableValue #25727

Conversation

minborg commented Jun 10, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

Uh oh!

bridgekeeper bot commented Jun 10, 2025

Uh oh!

openjdk bot commented Jun 10, 2025

Uh oh!

openjdk bot commented Jun 10, 2025

Uh oh!

Uh oh!

Uh oh!

mlbridge bot commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

minborg commented Jun 10, 2025

Uh oh!

AlanBateman Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minborg Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

AlanBateman Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

AlanBateman Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

AlanBateman Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

AlanBateman Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

AlanBateman Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

AlanBateman Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented Jun 12, 2025

Uh oh!

wenshao commented Jul 20, 2025

Uh oh!

Uh oh!

minborg commented Jun 10, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 10, 2025 •

edited

Loading

AlanBateman Jun 10, 2025 •

edited

Loading