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

Remove caffeine from instrumentation-api to make it work on Android #4532

Closed
mateuszrzeszutek opened this issue Oct 28, 2021 · 8 comments
Closed
Labels
bug Something isn't working

Comments

@mateuszrzeszutek
Copy link
Member

Is your feature request related to a problem? Please describe.
Caffeine doesn't work on Android, using it will break library instrumentations (like okhttp3) that are supposed to be used on Android.
Animalsniffer detects a lot of classes in caffeine2 that are not present in android: #4505

Describe the solution you'd like
The current idea to solve this issue is to vendor in https://github.com/ben-manes/concurrentlinkedhashmap and move caffeine 2 & 3 to agent bootstrap -- so that when the javaagent is used the optimal cache implementation is chosen.

@mateuszrzeszutek mateuszrzeszutek added the bug Something isn't working label Oct 28, 2021
@anuraaga
Copy link
Contributor

Realized that if we would switch implementations in the javaagent anyways how about just going with an SPI? It could default to non-caffeine and just require adding an artifact to activate, even for library instrumentation

@iNikem
Copy link
Contributor

iNikem commented Nov 14, 2021

The current idea to solve this issue is to vendor in https://github.com/ben-manes/concurrentlinkedhashmap and move caffeine 2 & 3 to agent bootstrap -- so that when the javaagent is used the optimal cache implementation is chosen.

But that cache does not support weak keys, is it ok for us? @anuraaga ?

@anuraaga
Copy link
Contributor

@iNikem I think the idea was to not use bounded caches with weak keys anymore, so always use the WeakLockFree map. Do we have any bounded ones where removing the bound seems like it would cause a problem?

@iNikem
Copy link
Contributor

iNikem commented Nov 15, 2021

Now I am even more confused :D This ticket says to use concurrentlinkedhashmap, you are saying to use existing WeakLockFree, meeting notes from the last week say use concurrentlinkedhashmap or cache2k.

If we move Caffeine behind SPI, what will be our default cache implementation for library instrumentations? Can we get that consensus? :)

@anuraaga
Copy link
Contributor

Think we might have missed it from the notes, but I think wanted to split the entrypoint into two

  • One for bounded caches without weak keys
  • One for weak keys without a bound

Always use WeakLockFree for latter, use either ConcurrentLinkedHashMap (or maybe cache2k) or Caffeine based on an SPI for former.

I guess the default for library instrumentation bounded cache would be ConcurrentLinkedHashMap since including is a bit simpler than excluding.

@iNikem
Copy link
Contributor

iNikem commented Nov 15, 2021

So we want 3 cache implementations??

@mateuszrzeszutek
Copy link
Member Author

I think we want two types of cache:

  1. An unbounded weak-keyed cache
  2. A bounded, strong keys & values cache

So 2 different interfaces/APIs basically.
Weaklockfree would be the only implementation for cache type 1., while ConcurrentLinkedHashMap/cache2k/caffeine would be implementations of type 2.

@mateuszrzeszutek
Copy link
Member Author

Implemented in #4667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants