New: Add INVOKEDYNAMIC instrumentation.

[LlamaLad7]

This adds an additional CoverageDataAccess option with an implementation using INVOKEDYNAMIC.
It is suitable for class file versions from Java 7 onwards, but from Java 11 onwards the ConDy implementation takes precedence by default. Where possible the new approach takes precedence over the field-based approach.

The principle is very similar to the ConDy approach, where a bootstrap method is invoked to find the correct array, and the result is then cached by the JVM. Unlike the ConDy approach, one lookup is required per method rather than per class, since each individual call-site will use the BSM on first load. The one-time cost is therefore slightly higher, so it makes sense to use ConDy where possible, but the approaches are for the most part equivalent and the JITed assembly is the same for both.
Compared to the field approach, however, the important benefit is that the changes are invisible to the user program. Additionally, the transformation itself is much simpler, which should roughly balance out the extra one-time hashtable lookups.

My primary motivation for this is to fix various tests in the Kotlin compiler which are currently broken by the field instrumentation approach, e.g. this one, but in general it is good to disrupt the user code as little as possible.

[Maksim Zuev (zuevmaxim)]

LlamaLad7 Thank you for a great work! I have only some performance concerns. Do I understand correctly, that the CoverageRuntime.getHits method will be called on every instrumented method call? In this case, what is the difference with the NameCoverageDataAccess?

Making a hash table lookup per every method call was way slower than field/condy approach

[LlamaLad7]

Do I understand correctly, that the CoverageRuntime.getHits method will be called on every instrumented method call?

No, the getHits method or similar is only called on the first invocation of each method during the call-site linkage, after which the array becomes directly bound to the call-site like with the ConDy approach. The approach uses 1 lookup per instrumented method, compared to ConDy's 1 lookup per instrumented class. After the initial costs they are equivalent.

[Maksim Zuev (zuevmaxim)]

LlamaLad7 Looks like you are right. It looks better in terms of bytecode modifications with a small extra overhead, compared to field access

[LlamaLad7]

Yes, it will result in more lookups than the field approach, but for one-time costs I think this is acceptable, given the benefits of much simpler transformation and not disrupting the user code.

[Maksim Zuev (zuevmaxim)]

I think it still worth running benchmarks tests/jmh.gradle:54. I can run them a bit laster, or you can try and share results. The benchmarks need to be updated for the new indy option

[LlamaLad7]

Ah, I didn't notice those benchmarks. I will run them now.

The benchmarks need to be updated for the new indy option

It doesn't seem like the benchmarks currently have any way to compare different coverage approaches within a version, but I could add one if you like?

[Maksim Zuev (zuevmaxim)]

Yes, it would be great

[Maksim Zuev (zuevmaxim)]

You can reuse configureCompareWith

[LlamaLad7]

It seems that the test libs (apache commons and joda time) were built with java 8 and 5 respectively, so am I correct in thinking that I'd need to transform those jars to use the Java 11 classfile version for the benchmarks to be effective?
The same applies to the jmh subproject (built with java 5) but that's simpler to fix.

[Maksim Zuev (zuevmaxim)]

Oh, I forgot about this. It makes everything complicated. You could try instrumenting joda -- it has larger overhead when running with coverage

[LlamaLad7]

Joda was built with Java 5, so assuming most of the time is spent in library classes it wouldn't be affected by either the indy or condy approaches.

[LlamaLad7]

I don't think it would be too hard to manually force the jars onto classfile version 11, if that would be an acceptable solution. You would then of course only be able to run the benchmarks with JDK 11+

[LlamaLad7]

I was going to approach this with Gradle's Artifact Transforms, but I forgot that classes below Java 7 don't have frames, and computing them is not possible without the entire set of classes.
I could make a task that takes the sourceSets.jmh.runtimeClasspath classes and transforms them as a whole while computing frames based on ClassReaders, like the agent itself already does, but maybe for now it would be acceptable only to upgrade classes from Java 7+, which would cover the apache commons tests?
Edit: Shouldn't actually be that bad to upgrade them all, i'll try it.

[LlamaLad7]

I need to clean up the code a bit but the benchmark results are:

Benchmark score:
      OldCoverage vs FieldCoverage:
      apacheCollectionsTestCoverage: [ss] 2% speedup +- 0%
		10.99 (+-0.03) vs 10.81 (+-0.02)
		jodaTimeTestCoverage: [ss] 58% speedup +- 5%
		2.84 (+-0.05) vs 1.20 (+-0.04)
Benchmark score:
      FieldCoverage vs IndyCoverage:
      apacheCollectionsTestCoverage: [ss] 0% speedup +- 0%
		10.81 (+-0.02) vs 10.81 (+-0.02)
		jodaTimeTestCoverage: [ss] 20% speedup +- 5%
		1.23 (+-0.03) vs 0.98 (+-0.03)
Benchmark score:
      IndyCoverage vs CondyCoverage:
      apacheCollectionsTestCoverage: [ss] 0% speedup +- 0%
		10.78 (+-0.02) vs 10.78 (+-0.02)
		jodaTimeTestCoverage: [ss] 23% speedup +- 5%
		1.25 (+-0.03) vs 0.96 (+-0.03)

This does give me some doubts about the legitimacy of the joda time benchmark, but hopefully this helps to alleviate your performance concerns.

[Maksim Zuev (zuevmaxim)]

Looks suspicious -- FieldCoverage should be the same as CondyCoverage

FieldCoverage vs IndyCoverage:
jodaTimeTestCoverage: [ss] 20% speedup +- 5%
1.23 (+-0.03) vs 0.98 (+-0.03)

[LlamaLad7]

Some of the strange results might have been due to external workload, rerunning the joda tests gives:

Benchmark score:
      OldCoverage vs FieldCoverage:
      jodaTimeTestCoverage: [ss] 57% speedup +- 4%
		2.90 (+-0.04) vs 1.24 (+-0.04)
Benchmark score:
      FieldCoverage vs IndyCoverage:
      jodaTimeTestCoverage: [ss] 18% speedup +- 6%
		1.22 (+-0.03) vs 1.00 (+-0.03)
Benchmark score:
      IndyCoverage vs CondyCoverage:
      jodaTimeTestCoverage: [ss] 2% degradation +- 6%
		0.97 (+-0.03) vs 0.98 (+-0.03)

which seems better.
I also ran:

Benchmark score:
      FieldCoverage vs CondyCoverage:
      jodaTimeTestCoverage: [ss] 22% speedup +- 5%
		1.23 (+-0.03) vs 0.96 (+-0.03)

just to check. I don't think it's unreasonable for the field approach to be slower, there's a lot of extra jumping on every method call to check if the field is initialized yet.

[LlamaLad7]

I've pushed my changes if you want to try the benchmarks yourself.

[LlamaLad7]

Any updates on this? Since Kotlin/kotlinx-kover#673 was fixed I can work round the issue by filtering the instrumented classes, but it would be great not to have to.

[Maksim Zuev (zuevmaxim)]

You can pull this code out of both else branches

[Maksim Zuev (zuevmaxim)]

I think it worth setting this option to false by default for now. This option can be much slower then the field access when all the methods are called exactly once.

[LlamaLad7]

Happy to have it off by default if you're tentative, but IMO the 1-time extra hash table lookup per method should be quite comparable in cost to the extra transformation per method that the field approach does.