WeakCache instead of WeakMap

[lpriima]

The intention of this change is to replace (not change) our concurrent WeakMap abstraction.
Our concurrent WeakMap abstraction has a few problems… :

• unbounded size
• primary eviction policy isn’t size
• always uses synchronized even around ConcurrentMaps like Guava
• encourages computation under a lock
• needs better setting for concurrency level

New abstraction should be a “WeakCache” not for a “WeakMap”. Maps may need to be unbounded, but caches should be bounded.

Abstraction should be quite minimal.
It doesn’t need to be as complex as our current WeakMap.Provider scheme.
Just a simple factory function that returns a Guava Cache object with reasonable defaults.

[tylerbenson]

WeakCache should be in the bootstrap project since it's referenced by instrumentation, but Guava should not be in the bootstrap.

[tylerbenson]

did you figure this out?

[lpriima]

didn't figure out why. But definitely logic is expected to be like this in tests. If I use direct cache.get(key, valueSuppler) then tests are failing

[dougqh]

I was thinking about another detail related to critical sections yesterday.

I think doing the computation inside the critical section might be a bad idea in general.
My thinking is that it increases the amount of activity under lock including likely taking other locks. In other words, it increases our potential for deadlocks. I'm not sure how much it increases our exposure, but it is strictly an increase.

[dougqh]

Why Object key rather than K key?

[lpriima]

it's pretty much was a copy from guava Cache:

https://github.com/google/guava/blob/0cd4e9faa1360da4a343f84cb275d6eda0c5e732/guava/src/com/google/common/cache/Cache.java#L49

I was trying to find why it's done like this in guava and didn't find any reason, why they not using K there. They doesn't seems to care about compatibility with JDK versions before 5. Like in JDK There is always a some form of get method accepting Object in all types of map interfaces:

https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#get-java.lang.Object-
https://docs.oracle.com/javase/8/docs/api/java/util/AbstractMap.html#get-java.lang.Object-

[dougqh]

Yes, I'm aware that Java maps do something similar. I think we can leave it as is. I was mostly just curious.

changes addressed

[dougqh]

I like the moving of the factory onto AgentTooling. I think that's a good call given the inherent coupling.

[dougqh]

I see removed the duplication. Nice.

[dougqh]

I think I'd prefer that encapsulated a bit more. As is, that ServiceLoader look-up is a lot to process.

[dougqh]

Yes, I'd prefer to create a static factory to wrap the look-up, so we don't have to repeat this.

[lpriima]

Right now we already have several (e.g.: netty-4.0 and netty-4.1) no-dependency artifacts with a lot of copy-paste inside. If we try to move common code to separate module, the muzzle tests will fail. Do we want to make this changes inside this PR ?

[dougqh]

I see.

Yes, I think part of our problem is that helper class system doesn't transitively handle parents, etc. automatically. I think that ends up discouraging reuse, but we shouldn't solve that now.

I was more wondering whether we could put a static look-up method on WeakCache or WeakCache.Provider that handles the ServiceLoader logic. I suppose that has the problem of needing to find the right ClassLoader. Hmm?

[dougqh]

Looks like this is shaping up nicely, but I'd like to clean-up the ServiceLoader look-ups just a bit.

[tylerbenson]

Netty AttributeKeys can't recompute.

[tylerbenson]

I don't think this class can use a cache semantic. The reason this exists is because netty will throw an error if an attribute of the same name is created more than once. (See the comment in the class.)

[lpriima]

Maybe just cache without time eviction ? only by weak reference expiration

[dougqh]

Conceptually, I don't think of WeakReference expiration as eviction per se. From the standpoint of retrieving something from the Map/Cache, WeakReference expiration isn't an observable difference. Only the side effect on memory is observable.

[dougqh]

I don't think this class can use a cache semantic. The reason this exists is because netty will throw an error if an attribute of the same name is created more than once. (See the comment in the class.)

That doesn't necessarily mean that cache won't work, but it depends on the nature of the error. If Netty fails fast and in recoverable way, then we could simply handle the exception rather than introducing a side structure.

[tylerbenson]

Same problem here...

[tylerbenson]

Only minor/optional items remaining...

[tylerbenson]

This is implied for fields on an interface, so this doesn't change anything.

[lpriima]

that's true. Do you want me to revert it back ? it's just a style question

[dougqh]

I'm fine with dropping public, but I'm less keen on dropping static. And final is a useful implied addition.

[tylerbenson]

How expensive is loadWeakCacheProvider()? Is this something we should be concerned about?

[dougqh]

Yes, I was pondering that. I think it would be good to have the Provider captured by a static final, so we don't keep reloading it.

[tylerbenson]

since you're returning a boolean and not a count, maybe doesMatch/matches/evaluateMatch instead?

[dougqh]

Is that a side effect of our prior change in this area? But yes, we should clean-up the naming.