Skip to content

Use garbage-free formatter for s and S patterns #3338

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

Merged
merged 15 commits into from
Dec 31, 2024
Merged

Use garbage-free formatter for s and S patterns #3338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
cba3b5f
Use garbage-free formatter for `s` and `S` patterns
ppkarwasz Dec 27, 2024
69fa9cc
Fix failing test
ppkarwasz Dec 28, 2024
bfce29a
Apply suggestions from code review
ppkarwasz Dec 28, 2024
7879316
Apply review suggestions(2)
ppkarwasz Dec 28, 2024
0779964
Improve Javadoc
ppkarwasz Dec 28, 2024
1483158
Miscellaneous fixes
ppkarwasz Dec 28, 2024
acaaa69
Make `AbstractFormatter` private
ppkarwasz Dec 28, 2024
60e9002
Remove Java 17 method calls
ppkarwasz Dec 28, 2024
a44e3c9
Final corrections
ppkarwasz Dec 28, 2024
0e8ddb3
Move `escapeLiteral` to `PatternSequence`
ppkarwasz Dec 28, 2024
2279c11
Fix precision detection
ppkarwasz Dec 30, 2024
756ee61
Improve Javadoc of `tryMerge`
ppkarwasz Dec 30, 2024
412baee
Make benchmark JAR executable again
ppkarwasz Dec 30, 2024
cb1a159
Document GC characteristics of instant formatters
ppkarwasz Dec 30, 2024
0b198fc
Improve performance
ppkarwasz Dec 31, 2024
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
175 changes: 101 additions & 74 deletions ...g/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@
import java.util.stream.IntStream;
import java.util.stream.Stream;
import org.apache.logging.log4j.core.time.MutableInstant;
import org.apache.logging.log4j.core.util.internal.instant.InstantPatternDynamicFormatter.CompositePatternSequence;
import org.apache.logging.log4j.core.util.internal.instant.InstantPatternDynamicFormatter.DynamicPatternSequence;
import org.apache.logging.log4j.core.util.internal.instant.InstantPatternDynamicFormatter.PatternSequence;
import org.apache.logging.log4j.core.util.internal.instant.InstantPatternDynamicFormatter.SecondPatternSequence;
import org.apache.logging.log4j.core.util.internal.instant.InstantPatternDynamicFormatter.StaticPatternSequence;
import org.apache.logging.log4j.util.Constants;
import org.junit.jupiter.params.ParameterizedTest;
Expand All @@ -55,104 +55,74 @@ void sequencing_should_work(
static List<Arguments> sequencingTestCases() {
final List<Arguments> testCases = new ArrayList<>();

// Merged constants
testCases.add(Arguments.of(":'foo',", ChronoUnit.DAYS, singletonList(new StaticPatternSequence(":foo,"))));

// `SSSX` should be treated constant for daily updates
testCases.add(Arguments.of("SSSX", ChronoUnit.DAYS, singletonList(pCom(pDyn("SSS"), pDyn("X")))));
testCases.add(Arguments.of("SSSX", ChronoUnit.DAYS, asList(pMilliSec(), pDyn("X"))));

// `yyyyMMddHHmmssSSSX` instant cache updated hourly
testCases.add(Arguments.of(
"yyyyMMddHHmmssSSSX",
ChronoUnit.HOURS,
asList(
pCom(pDyn("yyyy"), pDyn("MM"), pDyn("dd"), pDyn("HH")),
pCom(pDyn("mm"), pDyn("ss"), pDyn("SSS")),
pDyn("X"))));
asList(pDyn("yyyyMMddHH", ChronoUnit.HOURS), pDyn("mm"), pSec("", 3), pDyn("X"))));

// `yyyyMMddHHmmssSSSX` instant cache updated per minute
testCases.add(Arguments.of(
"yyyyMMddHHmmssSSSX",
ChronoUnit.MINUTES,
asList(
pCom(pDyn("yyyy"), pDyn("MM"), pDyn("dd"), pDyn("HH"), pDyn("mm")),
pCom(pDyn("ss"), pDyn("SSS")),
pDyn("X"))));
asList(pDyn("yyyyMMddHHmm", ChronoUnit.MINUTES), pSec("", 3), pDyn("X"))));

// ISO9601 instant cache updated daily
final String iso8601InstantPattern = "yyyy-MM-dd'T'HH:mm:ss.SSSX";
testCases.add(Arguments.of(
iso8601InstantPattern,
ChronoUnit.DAYS,
asList(
pCom(pDyn("yyyy"), pSta("-"), pDyn("MM"), pSta("-"), pDyn("dd"), pSta("T")),
pCom(
pDyn("HH"),
pSta(":"),
pDyn("mm"),
pSta(":"),
pDyn("ss"),
pSta("."),
pDyn("SSS"),
pDyn("X")))));
pDyn("yyyy'-'MM'-'dd'T'", ChronoUnit.DAYS),
pDyn("HH':'mm':'", ChronoUnit.MINUTES),
pSec(".", 3),
pDyn("X"))));

// ISO9601 instant cache updated per minute
testCases.add(Arguments.of(
iso8601InstantPattern,
ChronoUnit.MINUTES,
asList(
pCom(
pDyn("yyyy"),
pSta("-"),
pDyn("MM"),
pSta("-"),
pDyn("dd"),
pSta("T"),
pDyn("HH"),
pSta(":"),
pDyn("mm"),
pSta(":")),
pCom(pDyn("ss"), pSta("."), pDyn("SSS")),
pDyn("X"))));
asList(pDyn("yyyy'-'MM'-'dd'T'HH':'mm':'", ChronoUnit.MINUTES), pSec(".", 3), pDyn("X"))));

// ISO9601 instant cache updated per second
testCases.add(Arguments.of(
iso8601InstantPattern,
ChronoUnit.SECONDS,
asList(
pCom(
pDyn("yyyy"),
pSta("-"),
pDyn("MM"),
pSta("-"),
pDyn("dd"),
pSta("T"),
pDyn("HH"),
pSta(":"),
pDyn("mm"),
pSta(":"),
pDyn("ss"),
pSta(".")),
pDyn("SSS"),
pDyn("X"))));
asList(pDyn("yyyy'-'MM'-'dd'T'HH':'mm':'", ChronoUnit.MINUTES), pSec(".", 3), pDyn("X"))));

// Seconds and micros
testCases.add(Arguments.of(
"HH:mm:ss.SSSSSS", ChronoUnit.MINUTES, asList(pDyn("HH':'mm':'", ChronoUnit.MINUTES), pSec(".", 6))));

return testCases;
}

private static CompositePatternSequence pCom(final PatternSequence... sequences) {
return new CompositePatternSequence(asList(sequences));
private static DynamicPatternSequence pDyn(final String singlePattern) {
return new DynamicPatternSequence(singlePattern);
}

private static DynamicPatternSequence pDyn(final String pattern, final ChronoUnit precision) {
return new DynamicPatternSequence(pattern, precision);
}

private static DynamicPatternSequence pDyn(final String pattern) {
return new DynamicPatternSequence(pattern);
private static SecondPatternSequence pSec(String separator, int fractionalDigits) {
return new SecondPatternSequence(true, separator, fractionalDigits);
}

private static StaticPatternSequence pSta(final String literal) {
return new StaticPatternSequence(literal);
private static SecondPatternSequence pMilliSec() {
return new SecondPatternSequence(false, "", 3);
}

@ParameterizedTest
@ValueSource(
strings = {
// Basics
"S",
"SSSSSSS",
"SSSSSSSSS",
"n",
Expand All @@ -163,8 +133,7 @@ private static StaticPatternSequence pSta(final String literal) {
"yyyy-MM-dd HH:mm:ss,SSSSSSS",
"yyyy-MM-dd HH:mm:ss,SSSSSSSS",
"yyyy-MM-dd HH:mm:ss,SSSSSSSSS",
"yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSS",
"yyyy-MM-dd'T'HH:mm:ss.SXXX"
"yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSS"
})
void should_recognize_patterns_of_nano_precision(final String pattern) {
assertPatternPrecision(pattern, ChronoUnit.NANOS);
Expand Down Expand Up @@ -233,20 +202,31 @@ void should_recognize_patterns_of_second_precision(final String pattern) {
assertPatternPrecision(pattern, ChronoUnit.SECONDS);
}

@ParameterizedTest
@ValueSource(
strings = {
static Stream<String> should_recognize_patterns_of_minute_precision() {
Stream<String> stream = Stream.of(
// Basics
"m",
"mm",
"Z",
"x",
"X",
"O",
"z",
"VV",
// Mixed with other stuff
"yyyy-MM-dd HH:mm",
"yyyy-MM-dd'T'HH:mm",
"HH:mm",
"yyyy-MM-dd HH x",
"yyyy-MM-dd'T'HH XX",
// Single-quoted text containing nanosecond and millisecond directives
"yyyy-MM-dd'S'HH:mm",
"yyyy-MM-dd'n'HH:mm"
})
"yyyy-MM-dd'n'HH:mm");
return Constants.JAVA_MAJOR_VERSION > 8 ? Stream.concat(stream, Stream.of("v")) : stream;
}

@ParameterizedTest
@MethodSource
void should_recognize_patterns_of_minute_precision(final String pattern) {
assertPatternPrecision(pattern, ChronoUnit.MINUTES);
}
Expand All @@ -267,28 +247,71 @@ static List<String> hourPrecisionPatterns() {
"K",
"k",
"H",
"Z",
"x",
"X",
"O",
"z",
"VV",
// Mixed with other stuff
"yyyy-MM-dd HH",
"yyyy-MM-dd'T'HH",
"yyyy-MM-dd HH x",
"yyyy-MM-dd'T'HH XX",
"ddHH",
// Single-quoted text containing nanosecond and millisecond directives
"yyyy-MM-dd'S'HH",
"yyyy-MM-dd'n'HH"));
if (Constants.JAVA_MAJOR_VERSION > 8) {
java8Patterns.add("B");
java8Patterns.add("v");
}
return java8Patterns;
}

static Stream<Arguments> dynamic_pattern_should_correctly_determine_precision() {
// When no a precise unit is not available, uses the closest smaller unit.
return Stream.of(
Arguments.of("G", ChronoUnit.ERAS),
Arguments.of("u", ChronoUnit.YEARS),
Arguments.of("D", ChronoUnit.DAYS),
Arguments.of("M", ChronoUnit.MONTHS),
Arguments.of("L", ChronoUnit.MONTHS),
Arguments.of("d", ChronoUnit.DAYS),
Arguments.of("Q", ChronoUnit.MONTHS),
Arguments.of("q", ChronoUnit.MONTHS),
Arguments.of("Y", ChronoUnit.YEARS),
Arguments.of("w", ChronoUnit.WEEKS),
Arguments.of("W", ChronoUnit.DAYS), // The month can change in the middle of the week
Arguments.of("F", ChronoUnit.DAYS), // The month can change in the middle of the week
Arguments.of("E", ChronoUnit.DAYS),
Arguments.of("e", ChronoUnit.DAYS),
Arguments.of("c", ChronoUnit.DAYS),
Arguments.of("a", ChronoUnit.HOURS), // Let us round it down
Arguments.of("h", ChronoUnit.HOURS),
Arguments.of("K", ChronoUnit.HOURS),
Arguments.of("k", ChronoUnit.HOURS),
Arguments.of("H", ChronoUnit.HOURS),
Arguments.of("m", ChronoUnit.MINUTES),
Arguments.of("s", ChronoUnit.SECONDS),
Arguments.of("S", ChronoUnit.MILLIS),
Arguments.of("SS", ChronoUnit.MILLIS),
Arguments.of("SSS", ChronoUnit.MILLIS),
Arguments.of("SSSS", ChronoUnit.MICROS),
Arguments.of("SSSSS", ChronoUnit.MICROS),
Arguments.of("SSSSSS", ChronoUnit.MICROS),
Arguments.of("SSSSSSS", ChronoUnit.NANOS),
Arguments.of("SSSSSSSS", ChronoUnit.NANOS),
Arguments.of("SSSSSSSSS", ChronoUnit.NANOS),
Arguments.of("A", ChronoUnit.MILLIS),
Arguments.of("n", ChronoUnit.NANOS),
Arguments.of("N", ChronoUnit.NANOS),
// Time zones can change in the middle of a UTC hour (e.g. India)
Arguments.of("VV", ChronoUnit.MINUTES),
Arguments.of("z", ChronoUnit.MINUTES),
Arguments.of("O", ChronoUnit.MINUTES),
Arguments.of("X", ChronoUnit.MINUTES),
Arguments.of("x", ChronoUnit.MINUTES),
Arguments.of("Z", ChronoUnit.MINUTES));
}

@ParameterizedTest
@MethodSource
void dynamic_pattern_should_correctly_determine_precision(String singlePattern, ChronoUnit expectedPrecision) {
assertThat(pDyn(singlePattern).precision).isEqualTo(expectedPrecision);
}

private static void assertPatternPrecision(final String pattern, final ChronoUnit expectedPrecision) {
final InstantPatternFormatter formatter =
new InstantPatternDynamicFormatter(pattern, Locale.getDefault(), TimeZone.getDefault());
Expand Down Expand Up @@ -351,7 +374,11 @@ static Stream<Arguments> formatterInputs(final String pattern) {

private static MutableInstant randomInstant() {
final MutableInstant instant = new MutableInstant();
final long epochSecond = RANDOM.nextInt(1_621_280_470); // 2021-05-17 21:41:10
// In the 1970's some time zones had sub-minute offsets to UTC, e.g., Africa/Monrovia.
// We will exclude them for tests:
final int minEpochSecond = 315_532_800; // 1980-01-01 01:00:00
final int maxEpochSecond = 1_621_280_470; // 2021-05-17 21:41:10
final long epochSecond = minEpochSecond + RANDOM.nextInt(maxEpochSecond - minEpochSecond);
final int epochSecondNano = randomNanos();
instant.initFromEpochSecond(epochSecond, epochSecondNano);
return instant;
Expand Down
2 changes: 1 addition & 1 deletion ...ogging/log4j/core/util/internal/instant/InstantPatternThreadLocalCachedFormatterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ static Object[][] getterTestCases() {
}

@ParameterizedTest
@ValueSource(strings = {"S", "SSSS", "SSSSS", "SSSSSS", "SSSSSSS", "SSSSSSSS", "SSSSSSSSS", "n", "N"})
@ValueSource(strings = {"SSSS", "SSSSS", "SSSSSS", "SSSSSSS", "SSSSSSSS", "SSSSSSSSS", "n", "N"})
void ofMilliPrecision_should_fail_on_inconsistent_precision(final String subMilliPattern) {
final InstantPatternDynamicFormatter dynamicFormatter =
new InstantPatternDynamicFormatter(subMilliPattern, LOCALE, TIME_ZONE);
Expand Down
Loading
Loading