Skip to content

8359119: Change Charset to use StableValue #25727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+36 −41
Open

8359119: Change Charset to use StableValue #25727

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57edd19
Add stable value functionality
minborg Jun 10, 2025
f89e3f6
Revert changes
minborg Jun 10, 2025
c9ce11d
Revert more
minborg Jun 10, 2025
21a43fa
Revert for loop
minborg Jun 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 36 additions & 41 deletions src/java.base/share/classes/java/nio/charset/Charset.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,16 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Objects;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.function.Supplier;


/**
Expand Down Expand Up @@ -381,16 +384,15 @@ public void remove() {
};
}

private static class ThreadTrackHolder {
static final ThreadTracker TRACKER = new ThreadTracker();
}
private static final Supplier<ThreadTracker> TRACKER = StableValue.supplier(
new Supplier<>() { public ThreadTracker get() { return new ThreadTracker(); }});
Copy link
Contributor

@AlanBateman AlanBateman Jun 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, revert the changes here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private static Object tryBeginLookup() {
return ThreadTrackHolder.TRACKER.tryBegin();
return TRACKER.get().tryBegin();
}

private static void endLookup(Object key) {
ThreadTrackHolder.TRACKER.end(key);
TRACKER.get().end(key);
}

private static Charset lookupViaProviders(final String charsetName) {
Expand Down Expand Up @@ -425,29 +427,27 @@ private static Charset lookupViaProviders(final String charsetName) {
}

/* The extended set of charsets */
private static class ExtendedProviderHolder {
static final CharsetProvider[] extendedProviders = extendedProviders();
// returns ExtendedProvider, if installed
private static CharsetProvider[] extendedProviders() {
CharsetProvider[] cps = new CharsetProvider[1];
int n = 0;
ServiceLoader<CharsetProvider> sl =
private static final Supplier<List<CharsetProvider>> EXTENDED_PROVIDERS = StableValue.supplier(
new Supplier<>() { public List<CharsetProvider> get() { return extendedProviders0(); }});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one looks okay.


private static List<CharsetProvider> extendedProviders0() {
CharsetProvider[] cps = new CharsetProvider[1];
int n = 0;
final ServiceLoader<CharsetProvider> sl =
ServiceLoader.loadInstalled(CharsetProvider.class);
for (CharsetProvider cp : sl) {
if (n + 1 > cps.length) {
cps = Arrays.copyOf(cps, cps.length << 1);
}
cps[n++] = cp;
for (CharsetProvider cp : sl) {
if (n + 1 > cps.length) {
cps = Arrays.copyOf(cps, cps.length << 1);
}
return n == cps.length ? cps : Arrays.copyOf(cps, n);
cps[n++] = cp;
}
return List.of(n == cps.length ? cps : Arrays.copyOf(cps, n));
}

private static Charset lookupExtendedCharset(String charsetName) {
if (!VM.isBooted()) // see lookupViaProviders()
return null;
CharsetProvider[] ecps = ExtendedProviderHolder.extendedProviders;
for (CharsetProvider cp : ecps) {
for (CharsetProvider cp : EXTENDED_PROVIDERS.get()) {
Charset cs = cp.charsetForName(charsetName);
if (cs != null)
return cs;
Expand Down Expand Up @@ -608,8 +608,7 @@ public static SortedMap<String,Charset> availableCharsets() {
new TreeMap<>(
String.CASE_INSENSITIVE_ORDER);
put(standardProvider.charsets(), m);
CharsetProvider[] ecps = ExtendedProviderHolder.extendedProviders;
for (CharsetProvider ecp :ecps) {
for (CharsetProvider ecp : EXTENDED_PROVIDERS.get()) {
put(ecp.charsets(), m);
}
for (Iterator<CharsetProvider> i = providers(); i.hasNext();) {
Expand All @@ -619,7 +618,14 @@ public static SortedMap<String,Charset> availableCharsets() {
return Collections.unmodifiableSortedMap(m);
}

private @Stable static Charset defaultCharset;
private static final Supplier<Charset> defaultCharset = StableValue.supplier(
new Supplier<>() { public Charset get() { return defaultCharset0(); }});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* Returns the default charset of this Java virtual machine.
Expand All @@ -640,25 +646,19 @@ public static SortedMap<String,Charset> availableCharsets() {
* @since 1.5
*/
public static Charset defaultCharset() {
if (defaultCharset == null) {
synchronized (Charset.class) {
// do not look for providers other than the standard one
Charset cs = standardProvider.charsetForName(StaticProperty.fileEncoding());
if (cs != null)
defaultCharset = cs;
else
defaultCharset = sun.nio.cs.UTF_8.INSTANCE;
}
}
return defaultCharset;
return defaultCharset.get();
}


/* -- Instance fields and methods -- */

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private final String[] aliases; // tickles a bug in oldjavac
private Set<String> aliasSet;
@Stable
private final Supplier<Set<String>> aliasSet = StableValue.supplier(
new Supplier<>() { public Set<String> get() { return Set.of(aliases); }});

/**
* Initializes a new charset with the given canonical name and alias
Expand Down Expand Up @@ -710,12 +710,7 @@ public final String name() {
* @return An immutable set of this charset's aliases
*/
public final Set<String> aliases() {
Set<String> set = this.aliasSet;
if (set == null) {
set = Set.of(aliases);
this.aliasSet = set;
}
return set;
return aliasSet.get();
}

/**
Expand Down