vy commented on code in PR #3939: URL: https://github.com/apache/logging-log4j2/pull/3939#discussion_r2381532157
########## src/changelog/.2.x.x/3935_optimize_DefaultThreadContextMap_getCopy.xml: ########## @@ -0,0 +1,22 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns="https://logging.apache.org/xml/ns"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation=" + https://logging.apache.org/xml/ns + https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"; + type="changed"> + <issue id="3935" link="https://github.com/apache/logging-log4j2/issues/3935"/> + <description format="asciidoc"> + Optimize `DefaultThreadContextMap.getCopy()` performance by avoiding megamorphic calls in HashMap constructor. + + The optimization replaces `new HashMap<>(map)` with manual iteration to avoid JDK-8368292 performance issues. + This provides significant performance improvements: + + * 37% faster for maps with 5 elements (90.9ns → 57.5ns) + * 47% faster for maps with 75 elements (1248ns → 667ns) + * 39% faster for maps with 1000 elements (18625ns → 11452ns) + + The change maintains identical functional behavior while eliminating megamorphic virtual calls that prevent JIT optimization. + This optimization particularly benefits applications with heavy ThreadContext usage such as web applications and microservices. + </description> Review Comment: ```suggestion <issue id="3935" link="https://github.com/apache/logging-log4j2/issues/3935"/> <issue id="3939" link="https://github.com/apache/logging-log4j2/pull/3939"/> <description format="asciidoc"> Optimize `DefaultThreadContextMap.getCopy()` performance by avoiding megamorphic calls in `HashMap` constructor </description> ``` ########## log4j-api/src/main/java/org/apache/logging/log4j/spi/DefaultThreadContextMap.java: ########## @@ -147,13 +147,53 @@ public <V> V getValue(final String key) { return (V) get(key); } + /** + * Returns a mutable copy of the current thread context map. + * <p> + * This method has been optimized to avoid performance issues with the HashMap(Map) constructor + * that suffers from megamorphic call overhead (see JDK-8368292 and GitHub issue #3935). + * The optimization provides 30-50% performance improvement for non-empty maps by using + * manual iteration instead of the HashMap constructor. + * </p> + * <p> + * Benchmark results show significant improvements: + * <ul> + * <li>Map size 5: 37% faster (90.9ns → 57.5ns)</li> + * <li>Map size 75: 47% faster (1248ns → 667ns)</li> + * <li>Map size 1000: 39% faster (18625ns → 11452ns)</li> + * </ul> + * </p> + * + * @return a mutable copy of the current thread context map, never {@code null} + */ @Override public Map<String, String> getCopy() { final Object[] state = localState.get(); if (state == null) { return new HashMap<>(0); } - return new HashMap<>(getMap(state)); + + final Map<String, String> map = getMap(state); + + // Handle empty map case efficiently - constructor is faster for empty maps + // (manual iteration shows 10-23% regression for empty maps) + if (map.isEmpty()) { + return new HashMap<>(); + } + + // Pre-size HashMap to minimize rehashing operations + // Factor 1.35 accounts for HashMap's 0.75 load factor (1/0.75 ≈ 1.33) + final HashMap<String, String> copy = new HashMap<>((int) (map.size() * 1.35)); + + // Manual iteration avoids megamorphic virtual calls that prevent JIT optimization. + // The HashMap(Map) constructor requires (3 + 4n) virtual method calls that become + // megamorphic when used with different map types, leading to 24-136% performance + // degradation. Manual iteration creates monomorphic call sites that JIT can optimize. Review Comment: ```suggestion // degradation. Manual iteration creates monomorphic call sites that JIT can optimize. // See https://bugs.openjdk.org/browse/JDK-8368292 ``` ########## log4j-api-test/src/test/java/org/apache/logging/log4j/spi/DefaultThreadContextMapTest.java: ########## @@ -130,4 +132,144 @@ void threadLocalNotInheritableByDefault() { void threadLocalInheritableIfConfigured() { threadLocalInheritableIfConfigured(createInheritableThreadContextMap()); } + + /** + * Test getCopy() with empty map + */ + @Test + void testGetCopyWithEmptyMap() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Verify map is empty + assertTrue(contextMap.isEmpty()); + assertEquals(0, contextMap.size()); + + // Get copy of empty map + final Map<String, String> copy = contextMap.getCopy(); + + // Verify copy is empty HashMap + assertThat(copy).isInstanceOf(HashMap.class); + assertTrue(copy.isEmpty()); + assertEquals(0, copy.size()); + + // Verify copy is independent + copy.put("test", "value"); + assertTrue(contextMap.isEmpty()); + } + + /** + * Test getCopy() with single-element map + */ + @Test + void testGetCopyWithSingleElement() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Add single element + contextMap.put("key1", "value1"); + assertEquals(1, contextMap.size()); + assertEquals("value1", contextMap.get("key1")); + + // Get copy + final Map<String, String> copy = contextMap.getCopy(); + + // Verify copy contains identical data + assertThat(copy).isInstanceOf(HashMap.class); + assertEquals(1, copy.size()); + assertEquals("value1", copy.get("key1")); + assertTrue(copy.containsKey("key1")); + + // Verify copy is independent + assertNotSame(copy, contextMap.toMap()); + copy.put("key2", "value2"); + assertEquals(1, contextMap.size()); + assertFalse(contextMap.containsKey("key2")); + } + + /** + * Test getCopy() with multiple elements + */ + @Test + void testGetCopyWithMultipleElements() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Add multiple elements + final Map<String, String> testData = new HashMap<>(); + testData.put("key1", "value1"); + testData.put("key2", "value2"); + testData.put("key3", "value3"); + testData.put("key4", "value4"); + testData.put("key5", "value5"); + + contextMap.putAll(testData); + assertEquals(5, contextMap.size()); + + // Get copy + final Map<String, String> copy = contextMap.getCopy(); + + // Verify copy contains identical data + assertThat(copy).isInstanceOf(HashMap.class); + assertEquals(5, copy.size()); + + for (Map.Entry<String, String> entry : testData.entrySet()) { + assertTrue(copy.containsKey(entry.getKey())); + assertEquals(entry.getValue(), copy.get(entry.getKey())); + } + + // Verify all entries match + assertEquals(testData, copy); + + // Verify copy is independent + copy.clear(); + assertEquals(5, contextMap.size()); + } + + /** + * Test getCopy() returns proper HashMap type + */ + @Test + void testGetCopyReturnsHashMap() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Test with empty map + Map<String, String> copy = contextMap.getCopy(); + assertThat(copy).isInstanceOf(HashMap.class); + + // Test with populated map + contextMap.put("key", "value"); + copy = contextMap.getCopy(); + assertThat(copy).isInstanceOf(HashMap.class); + } + + /** + * Test getCopy() independence from original map + */ Review Comment: ```suggestion ``` ########## log4j-api-test/src/test/java/org/apache/logging/log4j/spi/DefaultThreadContextMapTest.java: ########## @@ -130,4 +132,144 @@ void threadLocalNotInheritableByDefault() { void threadLocalInheritableIfConfigured() { threadLocalInheritableIfConfigured(createInheritableThreadContextMap()); } + + /** + * Test getCopy() with empty map + */ + @Test + void testGetCopyWithEmptyMap() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Verify map is empty + assertTrue(contextMap.isEmpty()); + assertEquals(0, contextMap.size()); + + // Get copy of empty map + final Map<String, String> copy = contextMap.getCopy(); + + // Verify copy is empty HashMap + assertThat(copy).isInstanceOf(HashMap.class); + assertTrue(copy.isEmpty()); + assertEquals(0, copy.size()); + + // Verify copy is independent + copy.put("test", "value"); + assertTrue(contextMap.isEmpty()); + } + + /** + * Test getCopy() with single-element map + */ + @Test + void testGetCopyWithSingleElement() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Add single element + contextMap.put("key1", "value1"); + assertEquals(1, contextMap.size()); + assertEquals("value1", contextMap.get("key1")); + + // Get copy + final Map<String, String> copy = contextMap.getCopy(); + + // Verify copy contains identical data + assertThat(copy).isInstanceOf(HashMap.class); + assertEquals(1, copy.size()); + assertEquals("value1", copy.get("key1")); + assertTrue(copy.containsKey("key1")); + + // Verify copy is independent + assertNotSame(copy, contextMap.toMap()); + copy.put("key2", "value2"); + assertEquals(1, contextMap.size()); + assertFalse(contextMap.containsKey("key2")); + } + + /** + * Test getCopy() with multiple elements + */ + @Test + void testGetCopyWithMultipleElements() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Add multiple elements + final Map<String, String> testData = new HashMap<>(); + testData.put("key1", "value1"); + testData.put("key2", "value2"); + testData.put("key3", "value3"); + testData.put("key4", "value4"); + testData.put("key5", "value5"); + + contextMap.putAll(testData); + assertEquals(5, contextMap.size()); + + // Get copy + final Map<String, String> copy = contextMap.getCopy(); + + // Verify copy contains identical data + assertThat(copy).isInstanceOf(HashMap.class); + assertEquals(5, copy.size()); + + for (Map.Entry<String, String> entry : testData.entrySet()) { + assertTrue(copy.containsKey(entry.getKey())); + assertEquals(entry.getValue(), copy.get(entry.getKey())); + } + + // Verify all entries match + assertEquals(testData, copy); + + // Verify copy is independent + copy.clear(); + assertEquals(5, contextMap.size()); + } + + /** + * Test getCopy() returns proper HashMap type + */ Review Comment: ```suggestion ``` ########## log4j-api/src/main/java/org/apache/logging/log4j/spi/DefaultThreadContextMap.java: ########## @@ -147,13 +147,53 @@ public <V> V getValue(final String key) { return (V) get(key); } + /** + * Returns a mutable copy of the current thread context map. + * <p> + * This method has been optimized to avoid performance issues with the HashMap(Map) constructor + * that suffers from megamorphic call overhead (see JDK-8368292 and GitHub issue #3935). + * The optimization provides 30-50% performance improvement for non-empty maps by using + * manual iteration instead of the HashMap constructor. + * </p> + * <p> + * Benchmark results show significant improvements: + * <ul> + * <li>Map size 5: 37% faster (90.9ns → 57.5ns)</li> + * <li>Map size 75: 47% faster (1248ns → 667ns)</li> + * <li>Map size 1000: 39% faster (18625ns → 11452ns)</li> + * </ul> + * </p> + * + * @return a mutable copy of the current thread context map, never {@code null} Review Comment: ```suggestion * {@return a mutable copy of the current thread context map} ``` ########## log4j-api-test/src/test/java/org/apache/logging/log4j/spi/DefaultThreadContextMapTest.java: ########## @@ -130,4 +132,144 @@ void threadLocalNotInheritableByDefault() { void threadLocalInheritableIfConfigured() { threadLocalInheritableIfConfigured(createInheritableThreadContextMap()); } + + /** + * Test getCopy() with empty map + */ + @Test + void testGetCopyWithEmptyMap() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Verify map is empty + assertTrue(contextMap.isEmpty()); + assertEquals(0, contextMap.size()); + + // Get copy of empty map + final Map<String, String> copy = contextMap.getCopy(); + + // Verify copy is empty HashMap + assertThat(copy).isInstanceOf(HashMap.class); + assertTrue(copy.isEmpty()); + assertEquals(0, copy.size()); + + // Verify copy is independent + copy.put("test", "value"); + assertTrue(contextMap.isEmpty()); + } + + /** + * Test getCopy() with single-element map + */ Review Comment: ```suggestion ``` ########## log4j-api-test/src/test/java/org/apache/logging/log4j/spi/DefaultThreadContextMapTest.java: ########## @@ -130,4 +132,144 @@ void threadLocalNotInheritableByDefault() { void threadLocalInheritableIfConfigured() { threadLocalInheritableIfConfigured(createInheritableThreadContextMap()); } + + /** + * Test getCopy() with empty map + */ Review Comment: ```suggestion ``` ########## log4j-api-test/src/test/java/org/apache/logging/log4j/spi/DefaultThreadContextMapTest.java: ########## @@ -130,4 +132,144 @@ void threadLocalNotInheritableByDefault() { void threadLocalInheritableIfConfigured() { threadLocalInheritableIfConfigured(createInheritableThreadContextMap()); } + + /** + * Test getCopy() with empty map + */ + @Test + void testGetCopyWithEmptyMap() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Verify map is empty + assertTrue(contextMap.isEmpty()); + assertEquals(0, contextMap.size()); + + // Get copy of empty map + final Map<String, String> copy = contextMap.getCopy(); + + // Verify copy is empty HashMap + assertThat(copy).isInstanceOf(HashMap.class); + assertTrue(copy.isEmpty()); + assertEquals(0, copy.size()); + + // Verify copy is independent + copy.put("test", "value"); + assertTrue(contextMap.isEmpty()); + } + + /** + * Test getCopy() with single-element map + */ + @Test + void testGetCopyWithSingleElement() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Add single element + contextMap.put("key1", "value1"); + assertEquals(1, contextMap.size()); + assertEquals("value1", contextMap.get("key1")); + + // Get copy + final Map<String, String> copy = contextMap.getCopy(); + + // Verify copy contains identical data + assertThat(copy).isInstanceOf(HashMap.class); + assertEquals(1, copy.size()); + assertEquals("value1", copy.get("key1")); + assertTrue(copy.containsKey("key1")); + + // Verify copy is independent + assertNotSame(copy, contextMap.toMap()); + copy.put("key2", "value2"); + assertEquals(1, contextMap.size()); + assertFalse(contextMap.containsKey("key2")); + } + + /** + * Test getCopy() with multiple elements + */ + @Test + void testGetCopyWithMultipleElements() { + final DefaultThreadContextMap contextMap = new DefaultThreadContextMap(); + + // Add multiple elements + final Map<String, String> testData = new HashMap<>(); + testData.put("key1", "value1"); + testData.put("key2", "value2"); + testData.put("key3", "value3"); + testData.put("key4", "value4"); + testData.put("key5", "value5"); + + contextMap.putAll(testData); + assertEquals(5, contextMap.size()); + + // Get copy + final Map<String, String> copy = contextMap.getCopy(); + + // Verify copy contains identical data + assertThat(copy).isInstanceOf(HashMap.class); + assertEquals(5, copy.size()); + + for (Map.Entry<String, String> entry : testData.entrySet()) { + assertTrue(copy.containsKey(entry.getKey())); + assertEquals(entry.getValue(), copy.get(entry.getKey())); + } + Review Comment: Doesn't this duplicate `assertEquals(testData, copy)`? ```suggestion ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
