Introduce additional probes for INVOKE opcodes

[C-Otto]

Introduce additional probes for INVOKE opcodes

As outlined on the mailing list, the additional probes provide better coverage results at the cost of higher runtime. I implemented the probes so that the exception paths are not counted as branches, so that the results do not indicate worse branch coverage than without these changes.

I volunteer to also update the documentation if this pull request is accepted otherwise.

[marchof]

Why do we need a new method visitMethodInsnWithProbe()? Isn't that equivalent with a sequential call to visitProbe() and visitMethodInsn()? Note that a "branch" in the analyzer does not mean that there has to be real branching point. Even today we place extra probes at the beginning and end of exception handlers.

Handling for visitInvokeDynamicInsn() is missing as this also results in a method call.

My assumption is be that this could be implemented within the org.jacoco.core.internal.flow package only.

[C-Otto]

I must admit I do not fully understand how the number of branches is computed. I saw tests failing after I added probes for INVOKE opcodes just like the other probes, as new branches were reported. I guess the problem you're referencing here is just regarding the naming of the "addProbeWithBranch" method?

I decided not to add probes for INVOKEDYNAMIC, as I have absolutely no idea how this opcode is used in practice. I am sure that the approach can easily be extended, but I am not sure if the performance penalty is compensated by more meaningful results. As INVOKEDYNAMIC was only added to increase performance for certain use cases, I feel that instrumenting these may be even worse than instrumenting the INVOKE opcodes used by Java. However, as a Java user I have no problem also adding probes for INVOKEDYNAMIC. I think we should discuss this after the more pressing issues are resolved.

I understood your comment regarding visitMethodInsnWithProbe as a suggestion to inline the code from MethodInstrumenter into MethodProbesAdapter, thus eliminating the need to have visitMethodInsnWithProbe. However, I fail to understand how I can add probes, as I'd need to access the probeInserted instance (which is not available in the adapter). I don't really see a difference between how INVOKE is handled by this code, and how the previously existing code delegates to "somethingWithProbe". As such, I don't think the flow package suffices, as we really need to add probes.

EDIT: Probes are not always added at the end of exception handlers. Indeed, the code only adds a probe at the end of a catch block if succeeding code can be reached by at least two paths (from the exception handler, and from somewhere else). This probe has nothing to do with exceptions or exception handlers, but is introduced with the same reasons as probes for other IF*/JMP opcodes. If you want, I can provide an example for that.

Furthermore, the probe added at the beginning of an "try" block is useful as there really is a branch. One path of the branch continues along the try-block and finishes normally, the other part starts in the "try" code but, after a while, branches off to the "catch" block. Thus, the probe can be used to find out if both the non-exception path and the exception path of a try-catch-block are covered.

[C-Otto]

In the mean time I learned that INVOKEDYNAMIC is used in Java 8 features (lambda expressions). Before probes are added for INVOKEDYNAMIC, I propose to run some more extensive benchmarks also on Java 8 code making use of lambda expressions.

[denes-nemeth]

Does anyone know how to get this pull request merged.
It looks like it solves a common problem.

[C-Otto]

@denes-nemeth Marc (marchof), the maintainer of JaCoCo, indicated that he'd prefer a fork. The main reason is that JaCoCo needs to be fast, and my changes would work against this goal. I am planning on releasing at least one additional version of JaCoCo (with another name?) that is able to provide better coverage results at the cost of some speed, but so far I did not find the time. If you want to help me with that, feel free to contact me.

[marchof]

@C-Otto Some thoughts about this issue:

The problem of missing coverage is primarly visible to the users when they look at the highlighted source code which is based on debug infomation (line numbers) in class files.

From the user's reports we see that the expected granularity is mostly lines: E.g. if we get a implicit exception in line 4 users expect to see at least the preceeding lines 1, 2 and 3 still as covered.

This leads me to an alternative aproach, which might result in less probes: What if we add additional probes between lines if the subsequent line contains at least one method call? This will significantly reduce the number of probes in comparison to adding a probe for every method call.

What I don't like about this aproach is that for tests with implicit exceptions you will get different coverage values for class files with debug information vs. files compiled without debug info.

[Godin]

@marchof I'm wondering if scenario of "files compiled without debug info" is really so common?

[mackerl]

I tried your patch on our source base with 6000 JUNIT Tests - the drawbacks on performance were negligible, but we gained 10% total coverage. (A lot of unit tests with expected exception now working properly)
Thanks for the patch.

[marchof]

@mackerl Thx for your feedback! We will integrate this feeture soon.

[C-Otto]

@mackerl Great, that put a smile on my face!

[C-Otto]

@marchof should I clean up and polish this PR, or are you investigating another approach you mentioned earlier?

[marchof]

@C-Otto I will create a PR for my alternative aproach as discussed earlier: Only insert probes before a method invocation when this is the first invocation of a source line. This saves as significant amount of probes. I'll will ask you for a review.

[Godin]

@marchof I see the difference between mentions of alternative approach:

probes between lines if the subsequent line contains at least one method call

and

insert probes before a method invocation when this is the first invocation of a source line

the second one seems simpler in implementation. Wondering if this is just my misunderstanding, or you had time to think and difference is intentional? 😉

Also if you need help on implementation, don't hesitate to ask - I had time thinking about first proposal, but didn't come up with implementation, whereas second looks clear.

[marchof]

@Godin It's a bit more tricky: We can't simply add additional probes before method invocations. This would typically lead to duplicate probes (which is useless overhead).
That's why I actually implemented the first approach: I extended the flow analyzer to mark labels which correspond to a new line AND have a subsequent method invocation. The MethodProbeAdapter then inserts a single probe for labels which are multi-targets or are marked as "methodInvocationLine".
Let me push a new PR tonight, so we can discuss and test on actual code.

[marchof]

I just pushed the line based implementation as a separate PR #310

Please feel free to test and add review comments.

[marchof]

Functionality has been implemented with #310.

@C-Otto Many thanks for starting this discussion and bringing such a huge improvement to JaCoCo!

[C-Otto]

It was a tough ride :) Thank you!