Use alternative weak-keyed map implementations to reduce work done in CommonTaskExecutor #1812
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tylerbenson
reviewed
Aug 28, 2020
Contributor
tylerbenson
left a comment
There was a problem hiding this comment.
Nice job. I think we should merge the ClassValue changes even if we decide to skip the inlining.
Contributor
Author
|
Well, we really need to get the amount of background "work" down, whether it's by expunging inline or some other mechanism. |
f094977 to
f427454
Compare
…d background cleaning of WeakConcurrentHashMap which persists beyond the usage of the map, use ClassValue instead of WeakMap in JAX-RS instrumentations
f427454 to
ead09a5
Compare
Contributor
Author
|
I think there's another solution - don't use |
Contributor
|
I'll admit I don't find WeakMap terribly helpful. I understand the idea of substituting out different WeakMaps, but in practice, I don't really want to do that. WeakMaps aren't really Maps per se but Caches. For me when writing a Cache, it is crucial to consider the eviction policy including cache size in context. WeakMap provides little control over the eviction policy which I find to be a major failure in its design. |
tylerbenson
reviewed
Aug 31, 2020
|
|
||
| private final WeakMap<ClassLoader, Boolean> injectedClassLoaders = newWeakMap(); | ||
| private final Map<ClassLoader, Boolean> injectedClassLoaders = | ||
| Collections.synchronizedMap(new WeakHashMap<ClassLoader, Boolean>()); |
Contributor
There was a problem hiding this comment.
I believe that WeakHashMap is still doing expungeStaleEntries() inline. If that's the case, is it still better to use that instead of WeakConcurrentMap?
Contributor
Author
There was a problem hiding this comment.
Yes, it does, whenever getTable() is called.
However, this question suggests that you may not have understood what I'm trying to achieve here. We have hundreds of background tasks executed once per second, because we use a WeakMap implementation with a "background" cleanup policy. These background tasks continue until the application shuts down, but the injectedClassLoaders map may never actually be used, and even if it is, its usefulness is short-lived and concentrated at the start of the application's life. To reduce the total cost of these maps' existence, increasing the costs of access (if the map is ever used) would make sense, but there's no evidence a synchronized wrapper around a WeakHashMap incurs significant (any?) cost over WeakConcurrentMap.
Contributor
There was a problem hiding this comment.
I understand what you're trying to achieve here. I just didn't understand why you chose this option vs the inline WeakConcurrentMap. That said, I don't disagree with your choice.
As for why the tests are failing, I'm not sure. I can look into it. Is it easy to reproduce if I run locally?
Contributor
Author
There was a problem hiding this comment.
The motivation is for this to be completely independent of the (quite complex) provision mechanism. I don't expect there to be any contention on injectedClassLoaders and the wrapper is there as an insurance policy against multithreaded use, so I don't expect to get anything in return for using WeakConcurrentMap here. I found the changes necessary to support multiple WeakMap implementations broke the test suite mysteriously, in exactly the same way this commit does. (This is my second attempt at getting rid of these background tasks).
To reproduce, run
./gradlew :dd-java-agent:test --stacktrace
It will time out after 120s and probably give you as much information about why as can be seen here: none whatsoever :(
Contributor
Author
There was a problem hiding this comment.
I was reduced to putting System.err.printlns into CoreTracer's constructor - PendingTrace.initialize() hangs. It seems PendingTrace relies on HelperInjector to initialise the background clean up, I'd need to take a thread dump to check if it's deadlocked somehow, but that's a task for tomorrow.
I wonder why the author of this comment in CoreTracer just before initializing PendingTrace wrote this comment:
// Ensure that PendingTrace.SPAN_CLEANER is initialized in this thread:
// FIXME: add test to verify the span cleaner thread is started with this call.
PendingTrace.initialize();
Contributor
Author
There was a problem hiding this comment.
Check out my latest commit
Contributor
There was a problem hiding this comment.
Hmm, seems like we have a lot of brittleness in our initialization order.
I'd expect the first job to always start-up the background thread regardless of which job it is & order. From a maintenance perspective, it seems like a pretty serious design flaw to be relying on something else to start the pool.
This looks a nice improvement. Although, I'm wondering if we don't need to just take a hard look at our initialization in general.
Contributor
Author
|
I agree that the What's very interesting about this change is that certain tests time out as a result of this change: private final WeakMap<ClassLoader, Boolean> injectedClassLoaders = newWeakMap();to private final Map<ClassLoader, Boolean> injectedClassLoaders =
Collections.synchronizedMap(new WeakHashMap<ClassLoader, Boolean>());I previously found that tests would time out when I made it possible for |
Contributor
Author
|
Here's a thread dump of the hanging task, I can't see a dead lock here, but making sure the |
dougqh
approved these changes
Aug 31, 2020
bantonsson
approved these changes
Sep 1, 2020
Contributor
Author
|
I hadn't realised how many |
Contributor
Author
|
As justification for the change, I put some tracing in to the With the change, after applying load, I see this (I included 2 duty cycles to draw attention to the variation in time of one of the tasks): The very slow task appears when the load starts - the map's purpose is for |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.