Fix regression in JdkMapAdapterStringMap performance

[ppkarwasz]

We introduce a new constructor for JdkMapAdapterStringMap to prevent the performance penalty of an exception if the map is immutable. The regression was introduced in #2101.

Closes #2238.

[vy]

Thanks so much for promptly handling this. I guess this warrants a quick 2.23.0 release.

Changes LGTM. But I am not feeling confident enough to say we addressed the problem. I don't see a test triggering the code path users reported. Can you add such a test, please? Maybe we can craft/mock a custom map throwing an [unexpected] exception on replace() and verify that replace() is not called?

[ppkarwasz]

This change is required by the toMap contract, which requires a "mutable snapshot of the underlying data".

[rgoers]

This seems strange to me. Why wouldn't you create an ImmutableJdkMapAdapterStringMap that extends JdkMapAdapterStringMap (or vice versa) instead of using a boolean parameter? It seems like it would be cleaner.

[ppkarwasz]

@rgoers,

This seems strange to me. Why wouldn't you create an ImmutableJdkMapAdapterStringMap that extends JdkMapAdapterStringMap (or vice versa) instead of using a boolean parameter? It seems like it would be cleaner.

The original problem in #2098 was that a user implemented ContextDataProvider#supplyContextData using Map#of. This caused the generation of a StringMap that wasn't frozen, but wasn't modifiable either, since the underlying map was immutable.

I tried auto-detecting the problem which caused the performance issue we are facing today. By adding a new constructor I want to give users clear directions that:

• if the provided map is not immutable, Log4j might reuse it to prevent the allocation of new instances,
• if the provided map is immutable, it must be marked so.

I can create an ImmutableJdkMapAdapterStringMap, but then the contract of the original JdkMapAdapterStringMap constructor must be changed. What do you think?

[ppkarwasz]

@rgoers,

I think that an entire ImmutableJdkMapAdapterStringMap class is a little bit overkill for this, but maybe a pair of factory methods would be more appropriate (IDEA complains that boolean in a constructor is bad practice).
Any idea how to call them? createMutable and createFrozen?