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.

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

Replace Guava caches #2044

Merged
merged 12 commits into from
Nov 24, 2020
Merged

Replace Guava caches #2044

merged 12 commits into from
Nov 24, 2020

Conversation

randomanderson
Copy link
Contributor

@randomanderson randomanderson commented Oct 30, 2020 •
edited
Loading

This pull request removes the last Guava dependency for non-testing code 🎉

For weak caches:

Java 7: Use a combination of WeakLockFree and ConcurrentLinkedHashMap (CLHM was written by the same guy that went on to write Caffeine and is the recommended solution for < jdk8). Entries are expunged when put or computeIfAbsent might hit capacity
Java 8+: Use Caffeine

For other FIFO caches

Use ConcurrentLinkedHashMap. I looked at our DDCache implementations and they don't support this use case.

Additionally, I did a small cleanup on the WeakCache and WeakMap interfaces

For reference, the maps/caches in use after this PR:

Unbounded Bounded (random removal) Bounded (FIFO)
Weak Keys WeakLockFree or synchronized java.util.WeakHashMap -- WeakLockFree+CLHM (jdk7) / Caffeine (jdk8+)
Strong Keys datadog.trace.api.CHMCache datadog.trace.api.FixedSizeCache ConcurrentLinkedHashMap

@randomanderson randomanderson requested a review from a team as a code owner October 30, 2020 16:57
randomanderson
randomanderson commented Oct 30, 2020
View reviewed changes
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentTooling.java
}

private static final long DEFAULT_CACHE_CAPACITY = 32;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no basis for this number. The alternative is removing newWeakCache() and forcing callers to specify a capacity.

richardstartin
richardstartin reviewed Oct 30, 2020
View reviewed changes
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentTooling.java Outdated

String version = System.getProperty("java.version");
try {
if (version == null || version.startsWith("1.7")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@devinsba mentioned encapsulating this in a Platform class before, I think there are probably enough instances of this to do it now

tylerbenson
tylerbenson approved these changes Nov 11, 2020
View reviewed changes
...agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java
final Cache<TypeCacheKey, TypePool.Resolution> sharedResolutionCache =
CacheBuilder.newBuilder()
.softValues()
final ConcurrentMap<TypeCacheKey, TypePool.Resolution> sharedResolutionCache =
new ConcurrentLinkedHashMap.Builder<TypeCacheKey, TypePool.Resolution>()
.maximumWeightedCapacity(TYPE_CAPACITY)
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth doing a perf test before/after this change?

@devinsba devinsba mentioned this pull request Nov 13, 2020
Closed
@devinsba
Copy link
Contributor

Fixes #2073

@devinsba devinsba linked an issue Nov 13, 2020 that may be closed by this pull request
Upgrading Guava to resolve CVE-2018-10237 #2073
Closed
@mcculls mcculls mentioned this pull request Nov 23, 2020
Closed
@mcculls
Copy link
Contributor

mcculls commented Nov 23, 2020

Hi @randomanderson - I think everything should now be in place to switch Caffeine over to our own scheduler.

I've been testing this in #2109 - if you rebase this PR on latest master and cherry-pick these two commits, CI should pass:

  • faa701b use AgentTaskScheduler.INSTANCE as Caffeine's executor for background cleanup
  • ddd9991 patch Caffeine's BoundedLocalCache.PerformCleanupTask
    (to avoid loading ForkJoinTask before instrumentation is ready)

The last commit is the smallest way I found to safely patch the offending task that extends ForkJoinTask and causes ForkJoinTask to load before instrumentation is ready. An alternative approach would be to compile a complete copy of the Caffeine source in a separate module (with the necessary source patch) and use that module instead of the inlined dependency.

richardstartin
richardstartin approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

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

I'm ok with the patch

@randomanderson randomanderson removed a link to an issue Nov 23, 2020
Upgrading Guava to resolve CVE-2018-10237 #2073
Closed
@randomanderson randomanderson merged commit f76fd6b into master Nov 24, 2020
@randomanderson randomanderson deleted the landerson/weak-cache branch November 24, 2020 20:03
@github-actions github-actions bot added this to the 0.69.0 milestone Nov 24, 2020
@anuraaga anuraaga mentioned this pull request Mar 19, 2021
Merged
@PerfectSlayer PerfectSlayer added comp: tooling Build & Tooling and removed remove guava labels Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerbenson tylerbenson tylerbenson approved these changes

@bantonsson bantonsson bantonsson approved these changes

@richardstartin richardstartin richardstartin approved these changes

Assignees
No one assigned
Labels
comp: tooling Build & Tooling
Projects
None yet
Milestone
0.69.0
Development

Successfully merging this pull request may close these issues.
7 participants
@randomanderson @devinsba @mcculls @tylerbenson @bantonsson @richardstartin @PerfectSlayer