Faster type matching #5724

laurit · 2022-03-31T21:32:50Z

Nice matchers match based on the class that is being loaded, very nice matchers just match the class name.
Naughty matcher match based on the class hierarch (does this class extend X, does it implement Y).
Evil matchers match based on the complex criteria that involves other classes (does this class contain a method that overrides a method annotated with A in super class or interfaces). We have 4 such evil matchers: jax-rs 1, jax-rs 2, jax-ws and external annotations. The last one can hopefully be fixed by removing hasSuperType (I can't see a reason why it is there in the first place), the rest should be disable by default, deleted or written in some other way.
The problem with naughty matchers is that they need to find bytes for super types, parse them and cache them to avoid repeating this. If there are many classes this can be quite costly. This PR introduces an alternative strategy. When a class is loaded, first ensure its super types are loaded. During matching find super type with ClassLoder.findLoadedClass and use the already loaded class for navigating type hierarchy. If just the name or the modifiers of the super type are needed then there is no need to find the actual bytes for the class. Additionally using this strategy fixes a shortcoming of current strategy. Based on just the class loader of class being loaded it is not always possible to find bytes for all of its super types as in OSGI environments super type of super type need not be visible to current class. There should be less stack traces for net.bytebuddy.pool.TypePool$Resolution$NoSuchTypeException in smoke tests for server thats use OSGI.

mateuszrzeszutek · 2022-04-01T11:26:43Z

We have 4 such evil matchers: jax-rs 1, jax-rs 2, jax-ws and external annotations. The last one can hopefully be fixed by removing hasSuperType (I can't see a reason why it is there in the first place), the rest should be disable by default, deleted or written in some other way.

I think we could probably disable them by default and explicitly state in our docs that they're very expensive to apply, if you use JAX-RS then you have to enable it manually.

laurit · 2022-04-01T11:58:52Z

I think we could probably disable them by default and explicitly state in our docs that they're very expensive to apply, if you use JAX-RS then you have to enable it manually.

These expensive instrumentations are only used to create a span for the annotated method. We already have other spans for both JAX-RS and JAX-WS create by framework specific instrumentations that are probably more useful than the expensive one.

trask · 2022-04-01T18:35:49Z

💯

anuraaga · 2022-04-05T03:51:08Z

If the jax instrumentations are missing classloader matchers, can we add them and still keep the instrumentation enabled by default?

laurit · 2022-04-05T18:49:29Z

If the jax instrumentations are missing classloader matchers, can we add them and still keep the instrumentation enabled by default?

They have the class loader matchers. The problem is that these apis are present in a lot application servers which will enable these instrumentation even when they aren't really used. I have enable them for now, we can deal with this later.

trask

❤️❤️❤️

These expensive instrumentations are only used to create a span for the annotated method. We already have other spans for both JAX-RS and JAX-WS create by framework specific instrumentations that are probably more useful than the expensive one.

I'd be interested in seeing the impact to the tests of disabling these expensive ones in a follow-up. I think it would depend on what exactly we lose by having these off-by-default, but I'm very open to it, as startup performance is a big deal.

...t-tooling/src/main/java/io/opentelemetry/javaagent/tooling/util/IgnoreFailedTypeMatcher.java

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/DefineClassHandler.java

trask · 2022-04-06T02:19:31Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/DefineClassHandler.java

+      ClassReader cr = new ClassReader(classBytes, offset, length);
+      String superName = cr.getSuperName();
+      if (superName != null) {
+        Class.forName(superName.replace('/', '.'), false, classLoader);
+      }
+      String[] interfaces = cr.getInterfaces();
+      for (String interfaceName : interfaces) {
+        Class.forName(interfaceName.replace('/', '.'), false, classLoader);
+      }


oh! for some reason I didn't think it was so cheap to get the superclass and interfaces!

I had a similar optimization to preload super types in glowroot: https://github.com/glowroot/glowroot/blob/main/agent/core/src/main/java/org/glowroot/agent/impl/PreloadSomeSuperTypesCache.java

but it stored the superclass/interfaces into a file cache during loading, and then used that to speed up subsequent startups

(this is so much better 🤩)

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/DefineClassHandler.java

muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/AgentCachingPoolStrategy.java

laurit · 2022-04-06T12:32:09Z

I'd be interested in seeing the impact to the tests of disabling these expensive ones in a follow-up. I think it would depend on what exactly we lose by having these off-by-default, but I'm very open to it, as startup performance is a big deal.

I did some rough measurements starting liferay. I measured invocation count & time spent for 2 point

locating the class. I wrapped the locator in

opentelemetry-java-instrumentation/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/AgentLocationStrategy.java

Line 36 in 77dae8d

locators.add(ClassFileLocator.ForClassLoader.WeaklyReferenced.of(classLoader));
type matching. I wrapped the matcher from type instrumentation in

opentelemetry-java-instrumentation/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/InstrumentationModuleInstaller.java

Line 105 in 77dae8d

new IgnoreFailedTypeMatcher(typeInstrumentation.typeMatcher()));

This pr.

match count=5409259, time=5247ms
locate count=10559, time=3333ms

Class loader optimization matcher disabled, this ensures that the jax-rs/ws annotation matchers have something to match.

match count=10154036, time=16974ms
locate count=32589, time=9313ms

Class loader optimization matcher disabled, jax-rs/ws annotation matchers disabled.

match count=9965431, time=6738ms
locate count=9279, time=2094ms

Class loader optimization matcher disabled, jax-rs/ws annotation matchers disabled, matchers that match based on annotation (external annotations, withspan, spring-ws) disabled.

match count=9368871, time=4852ms
locate count=3258, time=1008ms

Byte-buddy locates bytes for all annotations on all methods if you have a matcher that matches based on annotation. I hope I can convince byte-buddy author to change this or give us some way to just match based on the name without locating the annotation type.

In retrospect I realized that I probably should have counted only the classes located during matching. Then the last test would give

locate count=45, time=19ms

which looks good.

mateuszrzeszutek · 2022-04-06T13:54:12Z

muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/AgentTooling.java

+    private static final AgentBuilder.PoolStrategy POOL_STRATEGY =
+        new AgentCachingPoolStrategy(locationStrategy(bootstrapProxy));


I know that this will currently work, but the combination of volatile globals and static class initializers makes me worried - WDYT about constructing the PoolStrategy in the AgentInstaller and passing it along as constructor parameter? (this'd require making InstrumentationLoader a "normal" class, not an SPI implementation)

This is also used from muzzle. WDYT if I add an SPI for getting the bootstrap proxy?

That'd probably solve the problem, but wouldn't just creating a bootstrap proxy CL in the muzzle module be a bit simpler?

It depends on whether muzzle code needs the same bootstrap proxy as agent (as agent jar is appended to bootclasspath getting access to resources in agent needs a bit of work). Having multiple pool strategy instances could complicate this a bit as then they wouldn't share the cache unless some stuff is made static.

muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/AgentCachingPoolStrategy.java

mateuszrzeszutek · 2022-04-06T14:42:54Z

muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/AgentCachingPoolStrategy.java

+        // unlike byte-buddy implementation we cache the descriptor to avoid having to find
+        // super class and interfaces multiple times


trask · 2022-04-07T04:16:10Z

jax-rs/ws annotation matchers disabled

wow, those matchers are slow!! (esp. now that the inheritance hierarchy matchers have been optimized in this PR 👍)

…orms

Co-authored-by: Trask Stalnaker <[email protected]>

trask · 2022-04-08T17:39:12Z

🎉

* Faster type matching * make findLoadedClass accessible on java17 * enable jaxrs instrumentation for quarkus test * fix websphere * fix muzzle * javadoc formating * ignore classes that are know to fail to load for virtual field transforms * add back jaxrs and jaxws annotation instrumentations * Apply suggestions from code review Co-authored-by: Trask Stalnaker <[email protected]> * fix compile error * comments * replace deprecated method usage * add comment * add an spi to get access to bootstrap proxy from muzzle module Co-authored-by: Trask Stalnaker <[email protected]>

laurit force-pushed the faster-type-matching branch 2 times, most recently from dd252f8 to bf1c29f Compare April 1, 2022 10:11

laurit marked this pull request as ready for review April 5, 2022 18:49

laurit requested a review from a team April 5, 2022 18:49

trask approved these changes Apr 6, 2022

View reviewed changes

laurit force-pushed the faster-type-matching branch from c789962 to 77dae8d Compare April 6, 2022 10:36

mateuszrzeszutek approved these changes Apr 6, 2022

View reviewed changes

trask mentioned this pull request Apr 7, 2022

Logback instrumentation doesn't get applied when mismatching slf4j version is used #5761

Closed

trask approved these changes Apr 7, 2022

View reviewed changes

anuraaga approved these changes Apr 7, 2022

View reviewed changes

laurit and others added 13 commits April 7, 2022 19:45

Faster type matching

6271192

make findLoadedClass accessible on java17

75a1af4

enable jaxrs instrumentation for quarkus test

8ee20b5

fix websphere

09ecfb2

fix muzzle

452013b

javadoc formating

0ca911a

ignore classes that are know to fail to load for virtual field transf…

c4c77c8

…orms

add back jaxrs and jaxws annotation instrumentations

b1aa471

Apply suggestions from code review

850942b

Co-authored-by: Trask Stalnaker <[email protected]>

fix compile error

bae2644

comments

bdaa8db

replace deprecated method usage

335eb51

add comment

a2fecda

add an spi to get access to bootstrap proxy from muzzle module

8af1e6a

laurit force-pushed the faster-type-matching branch from 2311d71 to 8af1e6a Compare April 7, 2022 17:06

trask merged commit 4ad4490 into open-telemetry:main Apr 8, 2022

This was referenced Apr 10, 2022

Startup performance #5791

Closed

Fix missing class errors #5795

Merged

trask mentioned this pull request Apr 18, 2022

Startup impact of application insight on microservices startup microsoft/ApplicationInsights-Java#2000

Open

gangxie112 mentioned this pull request Jun 1, 2023

startup performance is downgraded significantly when add opentelemetry agent in spring project #8629

Closed

laurit deleted the faster-type-matching branch June 1, 2023 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster type matching #5724

Faster type matching #5724

laurit commented Mar 31, 2022

mateuszrzeszutek commented Apr 1, 2022

laurit commented Apr 1, 2022

trask commented Apr 1, 2022

anuraaga commented Apr 5, 2022

laurit commented Apr 5, 2022

trask left a comment

trask Apr 6, 2022

laurit commented Apr 6, 2022

mateuszrzeszutek Apr 6, 2022

laurit Apr 6, 2022

mateuszrzeszutek Apr 7, 2022

laurit Apr 7, 2022

mateuszrzeszutek Apr 6, 2022

trask commented Apr 7, 2022

trask commented Apr 8, 2022

		private static final AgentBuilder.PoolStrategy POOL_STRATEGY =
		new AgentCachingPoolStrategy(locationStrategy(bootstrapProxy));

		// unlike byte-buddy implementation we cache the descriptor to avoid having to find
		// super class and interfaces multiple times

Faster type matching #5724

Faster type matching #5724

Conversation

laurit commented Mar 31, 2022

mateuszrzeszutek commented Apr 1, 2022

laurit commented Apr 1, 2022

trask commented Apr 1, 2022

anuraaga commented Apr 5, 2022

laurit commented Apr 5, 2022

trask left a comment

Choose a reason for hiding this comment

trask Apr 6, 2022

Choose a reason for hiding this comment

laurit commented Apr 6, 2022

mateuszrzeszutek Apr 6, 2022

Choose a reason for hiding this comment

laurit Apr 6, 2022

Choose a reason for hiding this comment

mateuszrzeszutek Apr 7, 2022

Choose a reason for hiding this comment

laurit Apr 7, 2022

Choose a reason for hiding this comment

mateuszrzeszutek Apr 6, 2022

Choose a reason for hiding this comment

trask commented Apr 7, 2022

trask commented Apr 8, 2022