This is an automated email from the ASF dual-hosted git repository. pkarwasz pushed a commit to branch fix/2238_fix_jdk_string_map in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit f83d1926738ba958e3ecce1715f5c5755b3aa6e3 Author: Piotr P. Karwasz <[email protected]> AuthorDate: Mon Jan 29 19:56:29 2024 +0100 Fix regression in `JdkMapAdapterStringMap` performance We introduce a new constructor for `JdkMapAdapterStringMap` to prevent the performance penalty of an exception if the map is immutable. --- .../log4j/core/impl/JdkMapAdapterStringMapTest.java | 1 + .../logging/log4j/core/impl/JdkMapAdapterStringMap.java | 16 +++++++++++++++- .../log4j/core/impl/ThreadContextDataInjector.java | 14 +++++++------- .../logging/log4j/core/util/ContextDataProvider.java | 2 +- .../log4j/layout/template/json/util/JsonWriterTest.java | 4 ++-- src/changelog/.3.x.x/2238_fix_jdk_string_map.xml | 10 ++++++++++ 6 files changed, 36 insertions(+), 11 deletions(-) diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMapTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMapTest.java index 65e894814d..39953f0474 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMapTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMapTest.java @@ -44,6 +44,7 @@ public class JdkMapAdapterStringMapTest { @Test public void testConstructorDisallowsNull() { assertThrows(NullPointerException.class, () -> new JdkMapAdapterStringMap(null)); + assertThrows(NullPointerException.class, () -> new JdkMapAdapterStringMap(null, false)); } @Test diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMap.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMap.java index 8da3952cd8..b158143093 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMap.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMap.java @@ -47,9 +47,14 @@ public class JdkMapAdapterStringMap implements StringMap { private transient String[] sortedKeys; public JdkMapAdapterStringMap() { - this(new HashMap<String, String>()); + this(new HashMap<String, String>(), false); } + /** + * @deprecated for performance reasons since 2.23. + * Use {@link #JdkMapAdapterStringMap(Map, boolean)} instead. + */ + @Deprecated public JdkMapAdapterStringMap(final Map<String, String> map) { this.map = Objects.requireNonNull(map, "map"); try { @@ -59,6 +64,15 @@ public class JdkMapAdapterStringMap implements StringMap { } } + /** + * @param map a JDK map, + * @param immutable must be {@code true} if the map is immutable or it should not be modified. + */ + public JdkMapAdapterStringMap(final Map<String, String> map, final boolean immutable) { + this.map = Objects.requireNonNull(map, "map"); + this.immutable = immutable; + } + @Override public Map<String, String> toMap() { return map; diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThreadContextDataInjector.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThreadContextDataInjector.java index 4df68ce64a..24d8604bca 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThreadContextDataInjector.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThreadContextDataInjector.java @@ -121,7 +121,7 @@ public class ThreadContextDataInjector { // data. Note that we cannot reuse the specified StringMap: some Loggers may have properties defined // and others not, so the LogEvent's context data may have been replaced with an immutable copy from // the ThreadContext - this will throw an UnsupportedOperationException if we try to modify it. - final StringMap result = new JdkMapAdapterStringMap(new HashMap<>(copy)); + final StringMap result = new JdkMapAdapterStringMap(new HashMap<>(copy), false); for (int i = 0; i < props.size(); i++) { final Property prop = props.get(i); if (!copy.containsKey(prop.getName())) { @@ -133,20 +133,20 @@ public class ThreadContextDataInjector { } private static JdkMapAdapterStringMap frozenStringMap(final Map<String, String> copy) { - final JdkMapAdapterStringMap result = new JdkMapAdapterStringMap(copy); - result.freeze(); - return result; + return new JdkMapAdapterStringMap(copy, true); } @Override public ReadOnlyStringMap rawContextData() { final ReadOnlyThreadContextMap map = ThreadContext.getThreadContextMap(); - if (map instanceof ReadOnlyStringMap) { - return (ReadOnlyStringMap) map; + if (map != null) { + return map.getReadOnlyContextData(); } // note: default ThreadContextMap is null final Map<String, String> copy = ThreadContext.getImmutableContext(); - return copy.isEmpty() ? ContextDataFactory.emptyFrozenContextData() : new JdkMapAdapterStringMap(copy); + return copy.isEmpty() + ? ContextDataFactory.emptyFrozenContextData() + : new JdkMapAdapterStringMap(copy, true); } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/ContextDataProvider.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/ContextDataProvider.java index b96967d87f..f4351a9d79 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/ContextDataProvider.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/ContextDataProvider.java @@ -36,6 +36,6 @@ public interface ContextDataProvider { * @return the context data in a StringMap. */ default StringMap supplyStringMap() { - return new JdkMapAdapterStringMap(supplyContextData()); + return new JdkMapAdapterStringMap(supplyContextData(), true); } } diff --git a/log4j-layout-template-json-test/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java b/log4j-layout-template-json-test/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java index c97ec50b9e..9f672f226a 100644 --- a/log4j-layout-template-json-test/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java +++ b/log4j-layout-template-json-test/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java @@ -149,7 +149,7 @@ class JsonWriterTest { @Test void test_writeObject_StringMap() { - final StringMap map = new JdkMapAdapterStringMap(Collections.singletonMap("a", "b")); + final StringMap map = new JdkMapAdapterStringMap(Collections.singletonMap("a", "b"), true); final String expectedJson = "{'a':'b'}".replace('\'', '"'); final String actualJson = withLockedWriterReturning(writer -> writer.use(() -> writer.writeObject(map))); Assertions.assertThat(actualJson).isEqualTo(expectedJson); @@ -224,7 +224,7 @@ class JsonWriterTest { put("foo", "bar"); } }), - new JdkMapAdapterStringMap(Collections.singletonMap("a", "b")))); + new JdkMapAdapterStringMap(Collections.singletonMap("a", "b"), true))); put("key7", (StringBuilderFormattable) buffer -> buffer.append(7.7777777777777D)); } }; diff --git a/src/changelog/.3.x.x/2238_fix_jdk_string_map.xml b/src/changelog/.3.x.x/2238_fix_jdk_string_map.xml new file mode 100644 index 0000000000..6c4dd7e4dc --- /dev/null +++ b/src/changelog/.3.x.x/2238_fix_jdk_string_map.xml @@ -0,0 +1,10 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xmlns="http://logging.apache.org/log4j/changelog"; + xsi:schemaLocation="http://logging.apache.org/log4j/changelog https://logging.apache.org/log4j/changelog-0.1.2.xsd"; + type="fixed"> + <issue id="2238" link="https://github.com/apache/logging-log4j2/issues/2238"/> + <description format="asciidoc"> + Fix regression in `JdkMapAdapterStringMap` performance. + </description> +</entry>
