Migrate Guava cache usage to Caffeine #295

Stephan202 · 2021-05-05T17:54:52Z

This PR is a replacement of #286, which was closed as a side effect of the deletion of its base branch. For the original context, see #258. This migration from Guava to Caffeine was unlocked by the migration to Java 8 in the context of #276.

I'll re-apply the comments from the other PR, so as to ease the review effort.

Stephan202

Re-applied the comments from #286.

Stephan202 · 2021-05-05T17:55:11Z

agent-bridge/src/main/java/com/newrelic/agent/bridge/ExtensionHolder.java

@@ -17,7 +18,7 @@
     * Return the value associated with the instance in this ExtensionHolder. If the value is not present it will be
     * initialized (in a thread-safe manner) using valueLoader.
     */
-    public T getExtension(Object instance, Callable<T> valueLoader);
+    public T getExtension(Object instance, Supplier<T> valueLoader);


I found only one implementation of this interface. If it is used elsewhere we should revert this change and introduce a try/catch construct in the implementation.

keep this change.

newrelic-agent/build.gradle

newrelic-agent/src/main/java/com/newrelic/agent/ThreadService.java

Stephan202 · 2021-05-05T17:57:30Z

newrelic-agent/src/main/java/com/newrelic/agent/TimedTokenSet.java

-        activeTokens = CacheBuilder.newBuilder()
-                .concurrencyLevel(8)
+        activeTokens = Caffeine.newBuilder()
+                .initialCapacity(8)


For context see the discussion @ #286 (comment). We should consider dropping this line.

We are going to keep this change as is with capacity set to 8. I'm creating a research spike to unblock getting this merged and investigate whether resizing makes sense.

Stephan202 · 2021-05-05T17:57:39Z

newrelic-agent/src/main/java/com/newrelic/agent/TimedTokenSet.java

                .expireAfterAccess(timeOutMilli, TimeUnit.MILLISECONDS)
+                .executor(Runnable::run)


This Runnable::run is here because one of the tests expects to see the effects of the removal notification in a timely manner. See the wiki on how Caffeine may schedule this kind of work on another thread. Alternatively the test could be tweaked. If the current behavior is desired, perhaps .executor(Runnable::run) should also be added in AsyncTransactionService.

newrelic-agent/src/main/java/com/newrelic/agent/extension/ExtensionRewriter.java

Stephan202 · 2021-05-05T17:57:57Z

...ent/src/main/java/com/newrelic/agent/instrumentation/weaver/ClassLoaderClassTransformer.java

@@ -128,7 +128,7 @@ public ClassLoaderClassTransformer(InstrumentationProxy instrumentation, Set<Str
        // Try to use a custom Guava-based extension template to make NewField access better
        try {
            extensionTemplate = WeaveUtils.convertToClassNode(WeaveUtils.getClassBytesFromClassLoaderResource(
-                    GuavaBackedExtensionClass.class.getName(), GuavaBackedExtensionClass.class.getClassLoader()));
+                    CaffeineBackedExtensionClass.class.getName(), CaffeineBackedExtensionClass.class.getClassLoader()));


GuavaBackedExtensionClass is now Caffeine-backend, so a rename seemed in order.

Stephan202 · 2021-05-05T17:58:08Z

newrelic-agent/src/main/java/com/newrelic/agent/tracers/metricname/MetricNameFormats.java

+                key,
+                k -> metricName == null
+                        ? sig.getMetricNameFormat(key.invocationTargetClassName, flags)
+                        : new SimpleMetricNameFormat(getTracerMetricName(key.invocationTargetClassName, sig.getClassName(), metricName)));


Note that the if condition is now only evaluated on cache misses.

Stephan202 · 2021-05-05T17:58:18Z

newrelic-agent/src/test/java/com/newrelic/agent/extension/DependencyRemapperTest.java

+            // Make sure we pick up this Caffeine reference.
+            Caffeine<Object, Object> newBuilder = Caffeine.newBuilder();
+            // Make sure we pick up this Guava reference.
+            ImmutableSet<Object> set = ImmutableSet.of();


Related to my question above: no idea whether these test changes make sense.

Stephan202 · 2021-05-05T17:58:29Z

newrelic-weaver/src/test/java/com/newrelic/weave/weavepackage/WeavePackageManagerTest.java

@@ -239,7 +239,8 @@ public void testMaxInvalidClassLoaders() throws IOException {
            wpm.weave(cl, "com/newrelic/weave/weavepackage/WeavePackageManagerTest$NewNoMatchClass",
                    WeaveTestUtils.getClassBytes("com.newrelic.weave.weavepackage.WeavePackageManagerTest$NewNoMatchClass"));
        }
-        Assert.assertTrue(WeavePackageManager.MAX_INVALID_PACKAGE_CACHE >= wpm.invalidPackages.size());
+        wpm.invalidPackages.cleanUp();


See the wiki; Caffeine's size-based eviction may cause the cache to grow slightly larger than configured. Here we trigger an explicit cleanup to make sure the test passes. Under normal operation this kind of cleanup is performed in the context of subsequent cache operations.

This one .cleanUp() call wasn't enough; added a commit with a more robust fix.

XiXiaPdx · 2021-05-17T17:59:45Z

@Stephan202 Just a heads up that we are planning to review this after releasing agent version 7.0.0, which we are targeting for June 7th. Thank you for your contribution and patience!

Stephan202 · 2021-05-25T14:45:06Z

No hurry :)

For when the time comes: google/guava#5571 provides additional background.

tbradellis · 2021-06-02T02:41:28Z

@Stephan202 I've started doing some testing for this PR. I'm curious if this build resolves the deadlock we were seeing in the support ticket where discussing this change to Caffeine originally came up. Are you able to run this agent build in that environment?

Stephan202 · 2021-06-02T06:53:42Z

@tbradellis so far I did not attempt to deploy this code. Given our setup this wouldn't be impossible per se, but also a bit more work than I can commit to right now. I'll make a note in case I do find some spare time. 🤞

(For the time being we're still running with -Dnewrelic.config.class_transformer.com.newrelic.instrumentation.java.completable-future-jdk8u40.enabled=false. Haven't tried dropping it to see whether we're still hitting the issue in the first place. Will make a note to schedule such a test.)

tbradellis · 2021-08-10T00:49:20Z

Hey @Stephan202 . I'm working to get this merged. I need to rebase and I also have a few minor changes for some of the comments.
Can you give permission on the fork for me to push changes?

Stephan202 · 2021-08-10T05:05:04Z

Hi @tbradellis. It turns out that the "Allow edits from maintainers" feature isn't available for PRs created from organization accounts (such as PicnicSupermarket). Instead I have invited you as an "outside collaborator" on the repository itself.

If you have any questions about this or the code, let me know. Tnx for picking this up!

Stephan202 · 2021-08-10T05:24:48Z

I've rebased the branch on latest main; there were no conflicts.

W.r.t. open tasks, cursory inspection surfaces two more:

There are a few hits for git grep -i 'guava cache'; some/all of those should now probably reference Caffeine.
If THIRD_PARTY_NOTICES.md is manually maintained (seems so 👀) it should also be updated.

Stephan202 · 2021-08-10T06:07:49Z

It's not clear to me whether/how the Scala test failures relate to the changes in this PR.

tbradellis · 2021-08-10T17:13:02Z

Scala test failures

We have an issue with out scala tests that we are working to fix. It's also a race condition so, once it reruns a few times it'll pass.

CLAassistant · 2021-08-17T16:17:29Z

All committers have signed the CLA.

tbradellis · 2021-08-19T19:39:14Z

@Stephan202 It looks like I stepped on your commits when I had to --reset-author to get my license/cla accepted. I'll correct that.

kford-newrelic · 2021-08-23T23:52:15Z

Hi @Stephan202 check your email when you have a moment (and your junk folder just in case) I tried to send you a message

Fixes newrelic#258.

Stephan202 · 2021-08-24T06:00:53Z

@kford-newrelic it took me a while to find that email, as a GMail filter got hold of it; replied :)

@tbradellis I took the liberty to fix the author situation in this PR. (I still had an old version of the branch, so simply cherry-picked your commit on top of it and then rebased; there were no conflicts and the final diff is identical.)

…grate-to-caffeine" This reverts commit 015220c, reversing changes made to 0668807.

Stephan202 commented May 5, 2021

View reviewed changes

Stephan202 mentioned this pull request May 5, 2021

Migrate Guava cache usage to Caffeine #286

Closed

jasonjkeller linked an issue May 10, 2021 that may be closed by this pull request

Consider Caffeine for Caching to Replace Guava After EOL of Java 7 Support #258

Closed

kford-newrelic requested review from XiXiaPdx and jasonjkeller May 10, 2021 17:59

kford-newrelic assigned tbradellis Jun 1, 2021

Stephan202 force-pushed the improvement/migrate-to-caffeine branch from 648f3c1 to c25ab73 Compare August 10, 2021 05:22

tbradellis force-pushed the improvement/migrate-to-caffeine branch 2 times, most recently from bc93cf3 to 08997f0 Compare August 17, 2021 19:39

Stephan202 mentioned this pull request Aug 18, 2021

Have FluxOnBackpressureBufferStrategy reject sizes <= 0 reactor/reactor-core#2757

Merged

Stephan202 and others added 3 commits August 24, 2021 07:36

Migrate Guava cache usage to Caffeine

4cd9ab2

Fixes newrelic#258.

Fix all WeavePackageManagerTest cache size assertions

dfc24f3

change comments to caffeine. add 3rd party notice

7e82d80

Stephan202 force-pushed the improvement/migrate-to-caffeine branch from 08997f0 to 7e82d80 Compare August 24, 2021 05:55

kford-newrelic added the Q3 SC label Aug 24, 2021

tbradellis added 2 commits September 15, 2021 15:48

Merge branch 'newrelic:main' into improvement/migrate-to-caffeine

25016d8

Merge branch 'newrelic:main' into improvement/migrate-to-caffeine

42d0f4e

tbradellis requested review from tbradellis and removed request for jasonjkeller and XiXiaPdx September 21, 2021 20:56

tbradellis approved these changes Sep 21, 2021

View reviewed changes

tbradellis closed this Sep 21, 2021

tbradellis reopened this Sep 21, 2021

tbradellis merged commit 015220c into newrelic:main Sep 21, 2021

tbradellis deleted the improvement/migrate-to-caffeine branch September 21, 2021 21:02

meiao added a commit that referenced this pull request Sep 24, 2021

Revert "Merge pull request #295 from PicnicSupermarket/improvement/mi…

cac911f

…grate-to-caffeine" This reverts commit 015220c, reversing changes made to 0668807.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Guava cache usage to Caffeine #295

Migrate Guava cache usage to Caffeine #295

Stephan202 commented May 5, 2021

Stephan202 left a comment

Stephan202 May 5, 2021

tbradellis Sep 15, 2021

Stephan202 May 5, 2021

tbradellis Sep 15, 2021

Stephan202 May 5, 2021

Stephan202 May 5, 2021

Stephan202 May 5, 2021

Stephan202 May 5, 2021

Stephan202 May 5, 2021

Stephan202 May 5, 2021

XiXiaPdx commented May 17, 2021

Stephan202 commented May 25, 2021

tbradellis commented Jun 2, 2021 •

edited

Loading

Stephan202 commented Jun 2, 2021

tbradellis commented Aug 10, 2021

Stephan202 commented Aug 10, 2021

Stephan202 commented Aug 10, 2021

Stephan202 commented Aug 10, 2021

tbradellis commented Aug 10, 2021

CLAassistant commented Aug 17, 2021 •

edited

Loading

tbradellis commented Aug 19, 2021

kford-newrelic commented Aug 23, 2021

Stephan202 commented Aug 24, 2021

		.expireAfterAccess(timeOutMilli, TimeUnit.MILLISECONDS)
		.executor(Runnable::run)

Migrate Guava cache usage to Caffeine #295

Migrate Guava cache usage to Caffeine #295

Conversation

Stephan202 commented May 5, 2021

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiXiaPdx commented May 17, 2021

Stephan202 commented May 25, 2021

tbradellis commented Jun 2, 2021 • edited Loading

Stephan202 commented Jun 2, 2021

tbradellis commented Aug 10, 2021

Stephan202 commented Aug 10, 2021

Stephan202 commented Aug 10, 2021

Stephan202 commented Aug 10, 2021

tbradellis commented Aug 10, 2021

CLAassistant commented Aug 17, 2021 • edited Loading

tbradellis commented Aug 19, 2021

kford-newrelic commented Aug 23, 2021

Stephan202 commented Aug 24, 2021

tbradellis commented Jun 2, 2021 •

edited

Loading

CLAassistant commented Aug 17, 2021 •

edited

Loading