Skip to content
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

Move NumberFormat to thread-local variable #387

Merged
merged 4 commits into from
Nov 22, 2022
Merged

Move NumberFormat to thread-local variable #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d95f15e
Move NumberFormat to thread-local variable
erik Nov 18, 2022
8277036
Add supression for ThreadLocal use
erik Nov 21, 2022
a55669d
Use ThreadLocal for NumberFormat objects in `Format`
erik Nov 21, 2022
c9d0ede
Wrap `NumberFormat` instances rather than hashmap
erik Nov 21, 2022
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
50 changes: 32 additions & 18 deletions planetiler-core/src/main/java/com/onthegomap/planetiler/util/Format.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,40 @@
public class Format {

public static final Locale DEFAULT_LOCALE = Locale.getDefault(Locale.Category.FORMAT);
public static final ConcurrentMap<Locale, Format> instances = new ConcurrentHashMap<>();
private final NumberFormat pf;
private final NumberFormat nf;
private final NumberFormat intF;

private static final ConcurrentMap<Locale, Format> instances = new ConcurrentHashMap<>();

// `NumberFormat` instances are not thread safe, so we need to wrap them inside a `ThreadLocal`.
//
// Ignore warnings about not removing thread local values since planetiler uses dedicated worker threads that release
// values when a task is finished and are not re-used.
@SuppressWarnings("java:S5164")
private final ThreadLocal<NumberFormat> pf;
@SuppressWarnings("java:S5164")
private final ThreadLocal<NumberFormat> nf;
@SuppressWarnings("java:S5164")
private final ThreadLocal<NumberFormat> intF;

private Format(Locale locale) {
pf = NumberFormat.getPercentInstance(locale);
pf.setMaximumFractionDigits(0);
nf = NumberFormat.getNumberInstance(locale);
nf.setMaximumFractionDigits(1);
intF = NumberFormat.getNumberInstance(locale);
intF.setMaximumFractionDigits(0);
pf = ThreadLocal.withInitial(() -> {
var f = NumberFormat.getPercentInstance(locale);
f.setMaximumFractionDigits(0);
return f;
});
nf = ThreadLocal.withInitial(() -> {
var f = NumberFormat.getNumberInstance(locale);
f.setMaximumFractionDigits(1);
return f;
});
intF = ThreadLocal.withInitial(() -> {
var f = NumberFormat.getNumberInstance(locale);
f.setMaximumFractionDigits(0);
return f;
});
Comment on lines -26 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

minor thing - you could probably just inline these in the field definitions since they don't need anything outside the initializer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that possible if they need access to the locale parameter passed to the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah, good point!

}

public static Format forLocale(Locale locale) {
Format format = instances.get(locale);
if (format == null) {
format = instances.computeIfAbsent(locale, Format::new);
}
return format;
return instances.computeIfAbsent(locale, Format::new);
}

public static Format defaultInstance() {
Expand Down Expand Up @@ -117,17 +131,17 @@ private String format(Number num, boolean pad, NavigableMap<Long, String> suffix

/** Returns 0.0-1.0 as a "0%" - "100%" with no decimal points. */
public String percent(double value) {
return pf.format(value);
return pf.get().format(value);
}

/** Returns a number formatted with 1 decimal point. */
public String decimal(double value) {
return nf.format(value);
return nf.get().format(value);
}

/** Returns a number formatted with 0 decimal points. */
public String integer(Number value) {
return intF.format(value);
return intF.get().format(value);
}

/** Returns a duration formatted as fractional seconds with 1 decimal point. */
Expand Down
8 changes: 6 additions & 2 deletions planetiler-core/src/main/java/com/onthegomap/planetiler/util/Parse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ public class Parse {
Pattern.compile(
"(?<value>-?[\\d.]+)\\s*((?<mi>mi)|(?<m>m|$)|(?<km>km|kilom)|(?<ft>ft|')|(?<in>in|\")|(?<nmi>nmi|international nautical mile|nautical))",
Pattern.CASE_INSENSITIVE);
private static final NumberFormat PARSER = NumberFormat.getNumberInstance(Locale.ROOT);
// Ignore warnings about not removing thread local values since planetiler uses dedicated worker threads that release
// values when a task is finished and are not re-used.
@SuppressWarnings("java:S5164")
private static final ThreadLocal<NumberFormat> PARSER =
ThreadLocal.withInitial(() -> NumberFormat.getNumberInstance(Locale.ROOT));

private Parse() {}

Expand All @@ -41,7 +45,7 @@ public static Long parseLongOrNull(String tag) {
private static <T> T retryParseNumber(Object obj, Function<Number, T> getter, T backup) {
// more expensive parser in case simple valueOf parse fails
try {
return getter.apply(PARSER.parse(obj.toString()));
return getter.apply(PARSER.get().parse(obj.toString()));
} catch (ParseException e) {
return backup;
}
Expand Down