You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by pk...@apache.org on 2024/02/05 21:55:11 UTC
(logging-log4j2) branch 2.x updated (249a4f18dd -> d992d4d4ce)
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a change to branch 2.x
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
from 249a4f18dd Apply review suggestions
new e5d1c024c3 Fix regression in `JdkMapAdapterStringMap` performance
new 329fa64a05 Improve deprecated constructor and javadoc
new d992d4d4ce Remove test of deprecated constructor
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../core/impl/JdkMapAdapterStringMapTest.java | 2 +-
.../log4j/core/impl/JdkMapAdapterStringMap.java | 44 +++++++++++++++++++---
.../log4j/core/impl/ThreadContextDataInjector.java | 14 +++----
.../log4j/core/util/ContextDataProvider.java | 10 ++++-
.../layout/template/json/util/JsonWriterTest.java | 4 +-
..._kubernetes.xml => 2238_fix_jdk_string_map.xml} | 3 +-
6 files changed, 60 insertions(+), 17 deletions(-)
copy src/changelog/.2.x.x/{fix_typo_kubernetes.xml => 2238_fix_jdk_string_map.xml} (71%)
(logging-log4j2) 03/03: Remove test of deprecated constructor
Posted by pk...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a commit to branch 2.x
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit d992d4d4ceee37b5759cf57b5ce69438a43efe5f
Author: Piotr P. Karwasz <pi...@karwasz.org>
AuthorDate: Tue Jan 30 22:48:52 2024 +0100
Remove test of deprecated constructor
---
.../org/apache/logging/log4j/core/impl/JdkMapAdapterStringMapTest.java | 1 -
1 file changed, 1 deletion(-)
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 b827ba6cb9..81ff33dd26 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
@@ -48,7 +48,6 @@ public class JdkMapAdapterStringMapTest {
@Test
public void testConstructorDisallowsNull() {
- assertThrows(NullPointerException.class, () -> new JdkMapAdapterStringMap(null));
assertThrows(NullPointerException.class, () -> new JdkMapAdapterStringMap(null, false));
}
(logging-log4j2) 01/03: Fix regression in `JdkMapAdapterStringMap` performance
Posted by pk...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a commit to branch 2.x
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit e5d1c024c3e5cf20a0418f2d07767686603f5ab4
Author: Piotr P. Karwasz <pi...@karwasz.org>
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/.2.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 9f387c1388..b827ba6cb9 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
@@ -49,6 +49,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 e8426e55dc..fc03cb6fe1 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
@@ -48,9 +48,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 {
@@ -60,6 +65,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 8b8d3f9bfa..24f34936e6 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
@@ -133,7 +133,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())) {
@@ -145,20 +145,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 c285217446..bdada9f49a 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
@@ -150,7 +150,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);
@@ -225,7 +225,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/.2.x.x/2238_fix_jdk_string_map.xml b/src/changelog/.2.x.x/2238_fix_jdk_string_map.xml
new file mode 100644
index 0000000000..6c4dd7e4dc
--- /dev/null
+++ b/src/changelog/.2.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>
(logging-log4j2) 02/03: Improve deprecated constructor and javadoc
Posted by pk...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a commit to branch 2.x
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 329fa64a0525c8153b95bf13ca5a516ac9ad8263
Author: Piotr P. Karwasz <pi...@karwasz.org>
AuthorDate: Tue Jan 30 22:47:00 2024 +0100
Improve deprecated constructor and javadoc
---
.../log4j/core/impl/JdkMapAdapterStringMap.java | 36 +++++++++++++++++-----
.../log4j/core/util/ContextDataProvider.java | 8 +++++
2 files changed, 36 insertions(+), 8 deletions(-)
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 fc03cb6fe1..09c4441394 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
@@ -21,6 +21,7 @@ import java.util.Comparator;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
+import java.util.WeakHashMap;
import org.apache.logging.log4j.util.BiConsumer;
import org.apache.logging.log4j.util.ReadOnlyStringMap;
import org.apache.logging.log4j.util.StringMap;
@@ -42,13 +43,16 @@ public class JdkMapAdapterStringMap implements StringMap {
}
return left.compareTo(right);
};
+ // Cache of known unmodifiable map implementations.
+ // It is a cache, no need to synchronise it between threads.
+ private static Map<Class<?>, Void> UNMODIFIABLE_MAPS_CACHE = new WeakHashMap<>();
private final Map<String, String> map;
private boolean immutable = false;
private transient String[] sortedKeys;
public JdkMapAdapterStringMap() {
- this(new HashMap<String, String>(), false);
+ this(new HashMap<>(), false);
}
/**
@@ -58,25 +62,41 @@ public class JdkMapAdapterStringMap implements StringMap {
@Deprecated
public JdkMapAdapterStringMap(final Map<String, String> map) {
this.map = Objects.requireNonNull(map, "map");
- try {
- map.replace(Strings.EMPTY, Strings.EMPTY, Strings.EMPTY);
- } catch (final UnsupportedOperationException ignored) {
+ // Known immutable implementations
+ if (UNMODIFIABLE_MAPS_CACHE.containsKey(map.getClass())) {
immutable = true;
+ } else {
+ // Check with a NO-OP replacement
+ try {
+ map.replace(Strings.EMPTY, Strings.EMPTY, Strings.EMPTY);
+ } catch (final UnsupportedOperationException ignored) {
+ final WeakHashMap<Class<?>, Void> cache = new WeakHashMap<>(UNMODIFIABLE_MAPS_CACHE);
+ cache.put(map.getClass(), null);
+ UNMODIFIABLE_MAPS_CACHE = cache;
+ immutable = true;
+ }
}
}
/**
+ * Constructs a new {@link StringMap}, based on a JDK map.
+ * <p>
+ * The underlying map should not be modified after this call.
+ * </p>
+ * <p>
+ * If the {@link Map} implementation does not allow modifications, {@code frozen} should be set to {@code true}.
+ * </p>
* @param map a JDK map,
- * @param immutable must be {@code true} if the map is immutable or it should not be modified.
+ * @param frozen if {@code true} this collection will be immutable.
*/
- public JdkMapAdapterStringMap(final Map<String, String> map, final boolean immutable) {
+ public JdkMapAdapterStringMap(final Map<String, String> map, final boolean frozen) {
this.map = Objects.requireNonNull(map, "map");
- this.immutable = immutable;
+ this.immutable = frozen;
}
@Override
public Map<String, String> toMap() {
- return map;
+ return new HashMap<>(map);
}
private void assertNotFrozen() {
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 f4351a9d79..8ac63b6858 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
@@ -27,12 +27,20 @@ public interface ContextDataProvider {
/**
* Returns a Map containing context data to be injected into the event or null if no context data is to be added.
+ * <p>
+ * Thread-safety note: The returned object can safely be passed off to another thread: future changes in the
+ * underlying context data will not be reflected in the returned object.
+ * </p>
* @return A Map containing the context data or null.
*/
Map<String, String> supplyContextData();
/**
* Returns the context data as a StringMap.
+ * <p>
+ * Thread-safety note: The returned object can safely be passed off to another thread: future changes in the
+ * underlying context data will not be reflected in the returned object.
+ * </p>
* @return the context data in a StringMap.
*/
default StringMap supplyStringMap() {