Skip to content

ClassLoaderMatcher put to Guava Cache outside critical section#1244

Merged
lpriima merged 1 commit into
masterfrom
lpriima/ClassLoaderMatcherGuavaCache
Feb 28, 2020
Merged

ClassLoaderMatcher put to Guava Cache outside critical section#1244
lpriima merged 1 commit into
masterfrom
lpriima/ClassLoaderMatcherGuavaCache

Conversation

@lpriima
Copy link
Copy Markdown
Contributor

@lpriima lpriima commented Feb 21, 2020

  1. move to Guava Cache with weak references
  2. split cache supplier(loader) and matcher

petclinic 100 runs avg start time:
before: 51ca429 avgTime : 18.49126000000000000000
after: 01b32d814fdcf1de9df46a7185ed78daf981434d avgTime : 17.60960000000000000000

@lpriima lpriima requested a review from a team as a code owner February 21, 2020 07:05
@lpriima lpriima force-pushed the lpriima/ClassLoaderMatcherGuavaCache branch 2 times, most recently from e8a916e to 37bcb4e Compare February 21, 2020 11:58
private final LoadingCache<ClassLoader, Boolean> cache =
CacheBuilder.newBuilder()
.weakKeys()
.build(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We might also want to set the concurrency level. I picked 8 for the type cache.
Although, we should add a configuration option in a separate PR -- and maybe auto-select by availableProcessors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Be careful with availableProcessors - it may be a lie in k8s land...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, that is true. We probably want to a cap on it. Although, I suspect for concurrency level we're better off err-ing slightly on the high-side.

Copy link
Copy Markdown
Contributor Author

@lpriima lpriima Feb 25, 2020

Choose a reason for hiding this comment

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

capped on 8.

@lpriima lpriima force-pushed the lpriima/ClassLoaderMatcherGuavaCache branch 7 times, most recently from fc04446 to 464d71e Compare February 25, 2020 21:54
@lpriima
Copy link
Copy Markdown
Contributor Author

lpriima commented Feb 25, 2020

Did we measure a difference by making the critical section smaller?

I suspect we could make this lighter by eliminating the Supplier which will cost an extra allocation at least while still running in the interpreter. We could either create a single instance and reuse the surrounding matcher -- or just eliminate the Supplier altogether and inline the code.

I'd previously tried the inline approach, but I'll admit the gains were small. Maybe, this just isn't worth the effort?
However, I think the switch away from WeakMap/WeakHashMap is still worthwhile.

image

Yes, I've tried w/o Supplier and it slightly better. But when we move to generic WeakCache abstraction, it will have the supplier (some anonymous class to load the cache) anyway: https://github.com/DataDog/dd-trace-java/blob/lpriima/WeakCache/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakCache.java#L38

Updated this PR without Supplier anyway.

@dougqh
Copy link
Copy Markdown
Contributor

dougqh commented Feb 25, 2020

Updated this PR without Supplier anyway.
Thanks for the nice summary of the intermediate progress.

Yes, I've tried w/o Supplier and it slightly better. But when we move to generic WeakCache abstraction, it will have the supplier (some anonymous class to load the cache) anyway: https://github.com/DataDog/dd-trace-java/blob/lpriima/WeakCache/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakCache.java#L38

Looks like this gives us a nice win right now, so I'm satisfied with this as is.
However, we will want to revisit this once WeakCache is in place.

We'll also want to look at any places using WeakMap and see if they are candidates for using WeakCache instead.

@lpriima
Copy link
Copy Markdown
Contributor Author

lpriima commented Feb 25, 2020

We'll also want to look at any places using WeakMap and see if they are candidates for using WeakCache instead.

it will be in 2 places here in this PR plus in all other places here: https://github.com/DataDog/dd-trace-java/pull/1256/files

@lpriima lpriima force-pushed the lpriima/ClassLoaderMatcherGuavaCache branch from 464d71e to 885212e Compare February 27, 2020 21:13
@lpriima lpriima changed the title ClassLoaderMatcher switch to Guava Cache ClassLoaderMatcher put to Guava Cache outside critical section Feb 27, 2020
@dougqh
Copy link
Copy Markdown
Contributor

dougqh commented Feb 27, 2020

While performing a perf integration test, I accidentally merged this prematurely to master. Since this has passed reviews, we should get this merged through the normal means.

@lpriima
Copy link
Copy Markdown
Contributor Author

lpriima commented Feb 28, 2020

ok. Merging

@lpriima lpriima merged commit 5317597 into master Feb 28, 2020
@lpriima lpriima deleted the lpriima/ClassLoaderMatcherGuavaCache branch February 28, 2020 00:04
@tylerbenson tylerbenson added this to the 0.45.0 milestone Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants