inject fields once at the root of the type hierarchy #2007
Conversation
richardstartin
force-pushed
the
rgs/targeted-field-injection
branch
from
b0a1ac3
to
32bb658
Compare
...t/agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldInjectionVisitor.java
Outdated
Show resolved
Hide resolved
richardstartin
force-pushed
the
rgs/targeted-field-injection
branch
4 times, most recently
from
e4b8620
to
30f0783
Compare
...ent-tooling/src/main/java/datadog/trace/agent/tooling/context/ShouldInjectFieldsMatcher.java
Show resolved
Hide resolved
richardstartin
force-pushed
the
rgs/targeted-field-injection
branch
from
30f0783
to
57ef21e
Compare
richardstartin
force-pushed
the
rgs/targeted-field-injection
branch
from
57ef21e
to
72e27cd
Compare
...ent-tooling/src/main/java/datadog/trace/agent/tooling/context/ShouldInjectFieldsMatcher.java
Outdated
Show resolved
Hide resolved
| KEY_TYPE_IS_CLASS.put(keyType, true); | ||
| return keyType; | ||
| } | ||
| for (TypeDescription.Generic iface : superClass.getInterfaces()) { |
There was a problem hiding this comment.
We'll still miss a chance to optimize if the class hierarchy implements an interface that extends the key type and doesn't implement the key type directly (since this only checks directly declared interfaces, and not the interfaces they extend) but I think that's OK as widening the search to include transitive interfaces could be expensive.
There was a problem hiding this comment.
You're right, but this should work for all the types we're interested in injecting (we need to stop doing this for Runnable for other reasons). This kind of matching is done repetitively, and I think the structure of the types we care about, at least for the purpose of field injection, could be memoized cheaply so we don't need to do so many traversals. The result is also correct no matter what, it's just a missed optimisation opportunity (both in space and transformation time).
There was a problem hiding this comment.
Having said that, I would be amazed if traversing the hierarchy when we already know there's a subtype relationship outweighs the cost of unnecessary transformation, or the long term cost of carrying extra fields around.
There was a problem hiding this comment.
true - if you do extend the search to cover transitive interfaces then you might want to consider a local cache to track which interfaces you've seen
There was a problem hiding this comment.
For more context to anyone else reviewing, the single case that I considered making this for is ForkJoinTask. Whether you're using the streams API or RecursiveAction etc. the API depends on being able fork lots of tiny objects but there are often at least two levels of class inheritance. This won't make much difference to wide interface inheritance hierarchies.
| * fields before is being transformed again. Note: here we assume that Class#getInterfaces() | ||
| * returns list of interfaces defined immediately on a given class, not inherited from its | ||
| * parents. It looks like current JVM implementation does exactly this but javadoc is not | ||
| * explicit about that. |
There was a problem hiding this comment.
I think that the javadoc is very clear:
Returns the interfaces directly implemented by the class or interface represented by this object.
There was a problem hiding this comment.
This is an existing comment
There was a problem hiding this comment.
Yes, I agree. There are also a few missing articles which certainly justifies a cleanup.
...ent-tooling/src/main/java/datadog/trace/agent/tooling/context/ShouldInjectFieldsMatcher.java
Show resolved
Hide resolved
...ent-tooling/src/main/java/datadog/trace/agent/tooling/context/ShouldInjectFieldsMatcher.java
Show resolved
Hide resolved
|
@bantonsson it's not clear which changes you are requesting. |
|
I just wanted to get a chance to review again, and this is the only way 😉 |
| // false: the key type is an interface, so we need to find the class | ||
| // closes to java.lang.Object which implements the key type | ||
| // null: we don't know yet because we haven't seen the key type before | ||
| Boolean keyTypeIsClass = KEY_TYPE_IS_CLASS.get(keyType); |
There was a problem hiding this comment.
Very slim edge case that is probably not worth considering: It is possible for this result to be different on different classloaders within a single jvm.
There was a problem hiding this comment.
I'm fairly sure there would be a ClassCastException in any instrumentation using a context store if your edge case existed in practice.
does field injection in the highest concrete type still a subtype of the context store key class, which prevents from adding a field to every class in the hierarchy.