speedup context injection matchers

[mar-kolya]

This provides 0.5-1s speedup on petclinic app on java8.

There is still a question on how to make different classloader matchers not break things - but even in current state this should be functionally correct.

[dougqh]

Yes, that assumption sounds reasonable to me, but I think it speaks to the fact that our abstraction isn't quite right.

It seems to me that the contextClasses should carry a ClassLoaderMatcher constraint that is incorporated into the Instrumenter activation implicitly. An Instrumenter could safely add additional criteria, but cannot remove the requirement needed by the contextClasses.

[mar-kolya]

Yes, our abstractions are wrong and I'm not immediately sure how to fix that. I think we have to introduce the notion of 'compound instrumenter' - i.e. 'instrumenter' that covers multiple instances of Instrumenter.Default and defines common settings for them.

Currently we do not have this mechanism.

[richardstartin]

We ended up disabling field injection of Runnables and Callables when classloader matchers were added for AkkaForkJoinPool and ScalaForkJoinPool because they share context stores with the RunnableInstrumentation and CallableInstrumentation - see #1910

[mar-kolya]

interesting

[dougqh]

A Set<Map.Entry<String, String>> looks rather odd. My first thought was how this different from a Setof just the Entry keys. But I guess we're accumulating this from multiple maps, so it makes sense. Still not very obvious.

I also think there's likely a better option than reducing statics here. Ultimately, the reset boundary is contained by the caller, so I think it would be better for the caller to construct and pass in object rather than introducing a static that the caller must remember to reset.

[mar-kolya]

I do not have a strong opinion here overall, but I feel passing it from 'outside' has two problems:

• We spread way to much internal knowledge outside
• We would pipe it through quite a few levels

Overall I'm not sure it's worth the effort. Proper fix here is to fix our abstraction (with compound instrumenter of something) so we do not have to resort to this hack. But as far as hack goes I do not think that passing set all over the place is much better than using static.

[dougqh]

For me, the need to have the caller call reset is spreading the outside knowledge.

But I think this is fine for now, I think we agree the real issue is that we're missing a concept that connects multiple related Instrumenters. But I don't think, we should try to fix that in this PR.

[dougqh]

I know this wasn't part of this change, but I think we might be well served to move the construction of this Matcher into a helper method. Doesn't need to be part of this PR.

[mar-kolya]

I'm actually questioning correctness of this matcher at all - i have a feeling we should not put it here at all

[mar-kolya]

I'll look into this as separate PR.

[mar-kolya]

#1267

[dougqh]

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