Skip to content

Clean up thread context map implementations #2690

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 28, 2024
Merged

Clean up thread context map implementations #2690

merged 1 commit into from
Aug 28, 2024

Conversation

@ppkarwasz
Copy link
Contributor

We bring some order in the current variety of ThreadContextMap implementations by:

  • replacing DefaultThreadContextMap with the more performant StringArrayThreadContextMap introduced in Implement faster ThreadContextMap #2330,
  • removing the CopyOnWriteSortedArrayThreadContextMap, since it has no advantages over the default one. It doesn't even guarantee garbage-free logging.
  • moving the GarbageFreeSortedArrayThreadContextMap to log4j-core, since no other logging implementation can profit from it.

The documentation has been updated to include the fact that the configuration options for ThreadContext actually depend on the underlying implementation:

  • Log4j Core users have access to all the ThreadContext configuration options,
  • Simple Logger users have a more restricted list of config options (they can not use the garbage free map any more). We could even restrict this further by allowing only the log4j2.simplelogShowContextMap and log4j2.isThreadContextMapInheritable options,
  • Logback users have no configuration options, since the thread context map is hardcoded to use MDC,
  • JUL users have no configuration options, since the thread context map is hardcoded to no-op.

This PR deprecates #2593.

@ppkarwasz ppkarwasz mentioned this pull request Jun 24, 2024
Closed
@rgoers
Copy link
Member

rgoers commented Jun 25, 2024

Before merging this please ensure you look into the issues I had with the performance tests and StringArrayThreadContextMap.

@ppkarwasz ppkarwasz added this to the 2.24.0 milestone Aug 13, 2024
@ppkarwasz ppkarwasz force-pushed the feature/move-thread-context branch from 570a1fa to fc51ccd Compare August 14, 2024 11:28
@ppkarwasz
Copy link
Contributor Author

@rgoers,

I can not reproduce the issues in this branch. I suspect that they were due to the changes in the ThreadContextDataInjector (which I reverted).

The performance is similar to the GcFreeSortedArrayThreadContextMap, which seems reasonable:

Benchmark                            (count)  (threadContextMapAlias)  Mode  Cnt     Score    Error  Units
ThreadContextBenchmark.putAndRemove        5                  Default  avgt   25    40,337 ±  2,409  ns/op
ThreadContextBenchmark.putAndRemove        5             NoGcOpenHash  avgt   25    10,500 ±  0,446  ns/op
ThreadContextBenchmark.putAndRemove        5          NoGcSortedArray  avgt   25    46,233 ±  0,484  ns/op
ThreadContextBenchmark.putAndRemove       50                  Default  avgt   25   129,873 ±  2,131  ns/op
ThreadContextBenchmark.putAndRemove       50             NoGcOpenHash  avgt   25    12,334 ±  3,297  ns/op
ThreadContextBenchmark.putAndRemove       50          NoGcSortedArray  avgt   25    65,971 ±  0,742  ns/op
ThreadContextBenchmark.putAndRemove      500                  Default  avgt   25  1192,296 ± 21,985  ns/op
ThreadContextBenchmark.putAndRemove      500             NoGcOpenHash  avgt   25    15,351 ±  2,341  ns/op
ThreadContextBenchmark.putAndRemove      500          NoGcSortedArray  avgt   25   117,499 ±  1,333  ns/op

The poor performance when the context map has 50 or more entries is due to the remove operation, since the array is not sorted and the element was added at the end.

Large context maps and the remove operation itself are rather rare, but I can optimize it by reverting the order of the search.

@ppkarwasz
Copy link
Contributor Author

After reversing the lookup order in remove() the benchmark improves slightly:

Benchmark                            (count)  (threadContextMapAlias)  Mode  Cnt    Score    Error  Units
ThreadContextBenchmark.putAndRemove        5                  Default  avgt   25   37,141 ±  2,795  ns/op
ThreadContextBenchmark.putAndRemove        5             NoGcOpenHash  avgt   25   12,097 ±  2,007  ns/op
ThreadContextBenchmark.putAndRemove        5          NoGcSortedArray  avgt   25   46,352 ±  0,285  ns/op
ThreadContextBenchmark.putAndRemove       50                  Default  avgt   25   82,210 ±  1,496  ns/op
ThreadContextBenchmark.putAndRemove       50             NoGcOpenHash  avgt   25   15,974 ±  2,184  ns/op
ThreadContextBenchmark.putAndRemove       50          NoGcSortedArray  avgt   25   67,429 ±  1,369  ns/op
ThreadContextBenchmark.putAndRemove      500                  Default  avgt   25  731,671 ± 13,308  ns/op
ThreadContextBenchmark.putAndRemove      500             NoGcOpenHash  avgt   25   18,260 ±  5,095  ns/op
ThreadContextBenchmark.putAndRemove      500          NoGcSortedArray  avgt   25  117,670 ±  1,649  ns/op

@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Aug 16, 2024 •
edited
Loading

@rgoers,

Unless we find some problems, I will merge this PR next week. A reduced version of the ThreadContextvsScopedContextBenchmark is in the ThreadContextBenchmark2 class.

@ppkarwasz ppkarwasz self-assigned this Aug 18, 2024
vy
vy reviewed Aug 27, 2024
View reviewed changes
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Submitting my initial review. I liked the simplification in general.

@ppkarwasz
Clean up ThreadContextMap implementations
f27f6bc 
- Make `StringArray` the default thread context map
- Remove `CopyOnWriteSortedArrayThreadContextMap`
- Move `GarbageFreeSortedArrayThreadContextMap` to `log4j-core`
vy
vy approved these changes Aug 28, 2024
View reviewed changes
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only reviewed functional and documentation changes, they LGTM. (Did not have time to check the tests.)

@ppkarwasz ppkarwasz force-pushed the feature/move-thread-context branch from 8a43fa3 to f27f6bc Compare August 28, 2024 13:48
@ppkarwasz ppkarwasz merged commit f27f6bc into 2.x Aug 28, 2024
@ppkarwasz ppkarwasz deleted the feature/move-thread-context branch August 28, 2024 13:49
@ppkarwasz
Copy link
Contributor Author

I've only reviewed functional and documentation changes, they LGTM. (Did not have time to check the tests.)

Thanks, I am merging it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vy vy vy approved these changes

Assignees

@ppkarwasz ppkarwasz

Labels

None yet

Projects

None yet

Milestone

2.24.0

Development

Successfully merging this pull request may close these issues.

3 participants

@ppkarwasz @rgoers @vy