Avoid potential overhead of ThreadLocal.remove in LogbackMetrics #3952
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In one of our apps we're seeing that over time the size of the ThreadLocalMap underpinning the ThreadLocal class gets large (due to other libraries overusing/abusing ThreadLocals). When this happens
ThreadLocal.remove
can start to become a performance bottleneck as it does someexpungeStaleEntry
calls that iterate over the whole map.Because logger calls can be so frequent (even if they're suppressed this code is still called) the
TurboFilter
gets called a lot, and thus so doesThreadLocal.remove
. Therefore Micrometer ends up slowing down all our code.I agree that technically this isn't Micrometer's fault, but the potential for it to happen in any application is still there and MM should really do what it can to avoid becoming a bottleneck.
So this PR changes
LogbackMetrics
to remove the need for it to callThreadLocal.remove
and thus the potential for this bug. It does this by not clearing the ThreadLocal after use and instead setting it to a 'holder' class that has its own boolean flag that gets flipped on/off insideignoreMetrics
.Running the LogbackBenchmarks this seems to be possibly faster than the regular code even when ThreadLocals aren't overloaded, so it seems like a decent improvement overall
Of course the tradeoff is that now micrometer 'pollutes' all threads with an extraneous ThreadLocal value that it never cleans up. However:
In developing this fix I did try a couple of other solutions:
ThreadLocal.set(false)
instead ofremove
. Unfortunatelyset
does some similar cleanup toremove
so the issue would still be thereAtomicBoolean
instead of a custom class to hold the flag. This wasn't performant enough asAtomicBoolean
does a load of locking etc as it's meant to be used across threads - as these flags are explicitly only used on a single thread that locking is unnecessary overheadNote that there is a similar issue to this one that might have helped this situation (#3891) as most of the time this is happening for me on some suppressed
logger.debug
calls. However even without that fix this would still be an issue for every non-suppressed logger call that my app made.