Skip to content
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.

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

Avoid a slow exception catch in JdkMapAdapterStringMap constructor #2238

Closed
ppkarwasz opened this issue Jan 24, 2024 Discussed in #2235 · 5 comments
Closed

Avoid a slow exception catch in JdkMapAdapterStringMap constructor #2238

ppkarwasz opened this issue Jan 24, 2024 Discussed in #2235 · 5 comments
Assignees
Milestone

Comments

@ppkarwasz
Copy link
Contributor

Discussed in #2235

Originally posted by jdgenerix January 24, 2024
The org.apache.logging.log4j.core.impl.JdkMapAdapterStringMap constructor is called from org.apache.logging.log4j.core.impl.ThreadContextDataInjector with an UnmodiableMap (copy) and the constructor is trying to modify and catch an exception to detect if this is an immutable Map.
This code is triggered a lot of time and could be more efficient.
Adding a package protected constructor which take the immutable boolean flag as an argument and using it from ThreadContextDataInjector would avoid this slow try catch.

        @Override
        public ReadOnlyStringMap rawContextData() {
            final ReadOnlyThreadContextMap map = ThreadContext.getThreadContextMap();
            if (map instanceof ReadOnlyStringMap) {
                return (ReadOnlyStringMap) map;
            }
            // note: default ThreadContextMap is null
            final Map<String, String> copy = ThreadContext.getImmutableContext();
            return copy.isEmpty() ? ContextDataFactory.emptyFrozenContextData() : new JdkMapAdapterStringMap(copy);
        }


        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) {
			immutable = true;
		}
	}
```</div>
@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Jan 24, 2024

@jdgenerix,

Thank you for this report. Can you supply a PR to correct this?

Looking at the code you are pointing at, there is another optimization possible: ReadOnlyThreadContextMap can return a frozen StringMap without the need to convert it to and from a Java Map.

@jdgenerix
Copy link

@ppkarwasz I'm sorry, i don't really see the optimzation you saw with ReadOnlyThreadContextMap.

@ppkarwasz
Copy link
Contributor Author

@jdgenerix,

After looking better at the code the native storage of DefaultThreadContextMap is JDK's Map, so the only optimization possible is to create an additional constructor to JdkMapAdapterStringMap that takes an additional boolean frozen parameter.

The:

            final ReadOnlyThreadContextMap map = ThreadContext.getThreadContextMap();
            if (map instanceof ReadOnlyStringMap) {
                return (ReadOnlyStringMap) map;
            }

is also wrong (a ReadOnlyThreadContextMap is a factory of ReadOnlyStringMap not a ReadOnlyStringMap itself) and should be:

            final ReadOnlyThreadContextMap map = ThreadContext.getThreadContextMap();
            if (map != null) {
                return map.getReadOnlyContextData();
            }

@ppkarwasz
Copy link
Contributor Author

@jdgenerix,

Can you check if the latest snapshot in the Maven repo https://repository.apache.org/snapshots solves your problem?

@jdgenerix
Copy link

@ppkarwasz Thank you.
I did a test and it looks fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants