Mchr3k Fork

[Martin Hare Robertson]

Hi Marc, Evgeny,

As you know, I have been working on a fork of JaCoCo/EclEmma which adds some infrastructure for filtering code coverage along with some specific filters. I would like to discuss contributing some/all of these changes back to core JaCoCo. 

At a high level my changes can be split into the following parts:

* Coverage filter API

* Enabled by default filters:

** Synchronized exit

** Enum default methods

** Empty no-args constructor

* Optional filters:

** Source directives filter

* Finally block de-duplication

I have written about my changes:

* Project site: http://mchr3k.github.com/jacoco/

* Blog:

** http://mchr3k-coding.blogspot.com/2013/01/java-code-coverage-filtering-out-noise.html

** http://mchr3k-coding.blogspot.com/2013/01/java-bytecode-finally-blocks.html

** http://mchr3k-coding.blogspot.co.uk/2013/01/improving-finally-block-handling.html

Which pieces of my fork are you happy to accept? I should put these together into a pull request. 

Which pieces of my fork are you unconvinced about? We should discuss these. 

Are there any pieces which you explicitly don't want to accept into core JaCoCo? I would like to avoid this but I suspect the source directives filter and/or finally block dedup might fall under here. We should also discuss these. 

Martin

[Marc Hoffmann]

Hi Martin,

thanks for your work on the filtering story! All of the functionality
you implemented should make it into JaCoCo. as mentioned I have some
concerns with the implementation. But this is definitely not your fault!
For a clean implementation of filtering it is probably required to
change the internal processing in the flow/anayzer packages. I will need
to find more time to discuss the details, just some concerns about the
current implementation:

* Filtering should be an public API which allows others to implement
new filters beside default filters provided with JaCoCo (through the
same API -> "eat your own dowgfood" principle).
* I consider the probe insertion mechanism as an implementation
detail, therefore e.g. MethodProbesVisitor should not be exposed in this
public API
* The "finally block duplication" is just another filter and must not
be coded into the analyzer.

BTW, I detailed some filter specs for in the Wiki:
https://github.com/jacoco/jacoco/wiki/FilteringOptions

Best regards,
-marc

On 2013-01-14 09:32, Martin Hare Robertson wrote:
> Hi Marc, Evgeny,
>
> As you know, I have been working on a fork of JaCoCo/EclEmma which
> adds some infrastructure for filtering code coverage along with some
> specific filters. I would like to discuss contributing some/all of
> these changes back to core JaCoCo.
>
> At a high level my changes can be split into the following parts:
>
> * Coverage filter API
> * Enabled by default filters:
> ** Synchronized exit
> ** Enum default methods
> ** Empty no-args constructor
> * Optional filters:
> ** Source directives filter
> * Finally block de-duplication
>
> I have written about my changes:
>

> * Project site: http://mchr3k.github.com/jacoco/ [1]

> * Blog:
> **
>
> http://mchr3k-coding.blogspot.com/2013/01/java-code-coverage-filtering-out-noise.html

> [2]
> **
>
> http://mchr3k-coding.blogspot.com/2013/01/java-bytecode-finally-blocks.html
> [3]
> **
>
> http://mchr3k-coding.blogspot.co.uk/2013/01/improving-finally-block-handling.html
> [4]

>
> Which pieces of my fork are you happy to accept? I should put these
> together into a pull request.
>
> Which pieces of my fork are you unconvinced about? We should discuss
> these.
>
> Are there any pieces which you explicitly don't want to accept into
> core JaCoCo? I would like to avoid this but I suspect the source
> directives filter and/or finally block dedup might fall under here.
> We
> should also discuss these.
>
> Martin
>

> --
>
>
>
> Links:
> ------
> [1] http://mchr3k.github.com/jacoco/
> [2]
>
> http://mchr3k-coding.blogspot.com/2013/01/java-code-coverage-filtering-out-noise.html
> [3]
>
> http://mchr3k-coding.blogspot.com/2013/01/java-bytecode-finally-blocks.html
> [4]
>
> http://mchr3k-coding.blogspot.co.uk/2013/01/improving-finally-block-handling.html

[Martin Hare Robertson]

See inline below. 

On 14 Jan 2013, at 10:05, Marc Hoffmann <hoff...@mountainminds.com> wrote:

Hi Martin,

thanks for your work on the filtering story! All of the functionality you implemented should make it into JaCoCo. as mentioned I have some concerns with the implementation. But this is definitely not your fault! For a clean implementation of filtering it is probably required to change the internal processing in the flow/anayzer packages. I will need to find more time to discuss the details, just some concerns about the current implementation:

* Filtering should be an public API which allows others to implement new filters beside default filters provided with JaCoCo (through the same API -> "eat your own dowgfood" principle).

How would you expect filters to be wired into JaCoCo? Are you imagining a config file which lists all of the filters to use?

* I consider the probe insertion mechanism as an implementation detail, therefore e.g. MethodProbesVisitor should not be exposed in this public API
* The "finally block duplication" is just another filter and must not be coded into the analyzer.

The dedup isn't really a filter so much as a bytecode transformation. I think it would add a lot of complexity to try and separate this step out from the MethodAnalyzer. 

BTW, I detailed some filter specs for in the Wiki: https://github.com/jacoco/jacoco/wiki/FilteringOptions

I like the new detail pages. 

One correction: https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.FINALLY states that finally blocks are only emitted twice. This is only true for try/finally blocks. In try/catch/finally blocks the finally code is emitted (2 + (number of catch blocks)) times.

--

[Marc Hoffmann]

Hi Martin,

> How would you expect filters to be wired into JaCoCo?

API, something like Analyzer.addFilter(xxx);

> The dedup isn't really a filter so much as a bytecode transformation.

From my point of view it is a filter that filters out certain artifacts
which are specific to the java language and the Java compiler.

> I think it would add a lot of complexity to try and separate this
> step
> out from the MethodAnalyzer.

That's why I think we do not have a powerful filtering API yet.

> One correction:
> https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.FINALLY
> states that finally blocks are only emitted twice. This is only true
> for
> try/finally blocks. In try/catch/finally blocks the finally code is
> emitted
> (2 + (number of catch blocks)) times.

Thanks for noting this! I do furter analysis and update the page.

Another remark about filter implementations: All filters should work
without line number information.

Cheers,
-marc

On 2013-01-14 13:43, Martin Hare Robertson wrote:
> See inline below.
>
> On 14 Jan 2013, at 10:05, Marc Hoffmann <hoff...@mountainminds.com

> [3]> wrote:
>
>> Hi Martin,
>>
>> thanks for your work on the filtering story! All of the
>> functionality you implemented should make it into JaCoCo. as
>> mentioned I have some concerns with the implementation. But this is
>> definitely not your fault! For a clean implementation of filtering
>> it is probably required to change the internal processing in the
>> flow/anayzer packages. I will need to find more time to discuss the
>> details, just some concerns about the current implementation:
>>
>> * Filtering should be an public API which allows others to
>> implement new filters beside default filters provided with JaCoCo
>> (through the same API -> "eat your own dowgfood" principle).
>
> How would you expect filters to be wired into JaCoCo? Are you
> imagining a config file which lists all of the filters to use?
>
>> * I consider the probe insertion mechanism as an implementation
>> detail, therefore e.g. MethodProbesVisitor should not be exposed in
>> this public API
>> * The "finally block duplication" is just another filter and must
>> not be coded into the analyzer.
>
> The dedup isn't really a filter so much as a bytecode transformation.
> I think it would add a lot of complexity to try and separate this
> step
> out from the MethodAnalyzer.
>
>
>

> Links:
> ------
> [1] https://github.com/jacoco/jacoco/wiki/FilteringOptions
> [2] http://mchr3k.github.com/jacoco/
> [3] mailto:hoff...@mountainminds.com
> [4] https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.FINALLY

[Martin Hare Robertson]

Inline again :)

Do I need to address all of the issues you have raised to unblock my fork bring merged? Are any of these issues mainly for guidance/future consideration?

Martin

On 14 Jan 2013, at 13:15, Marc Hoffmann <hoff...@mountainminds.com> wrote:

> Hi Martin,
>
>> How would you expect filters to be wired into JaCoCo?
>
> API, something like Analyzer.addFilter(xxx);
>> The dedup isn't really a filter so much as a bytecode transformation.
>
> From my point of view it is a filter that filters out certain artifacts which are specific to the java language and the Java compiler.
>> I think it would add a lot of complexity to try and separate this step
>> out from the MethodAnalyzer.
>
> That's why I think we do not have a powerful filtering API yet.

I will give this some thought but I don't currently see how finally block dedup can live behind any sane general purpose filtering API rather than being part of the MethodAnalyzer. I think I will need you to propose a concrete alternative before I can make any progress on this.

>> One correction: https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.FINALLY
>> states that finally blocks are only emitted twice. This is only true for
>> try/finally blocks. In try/catch/finally blocks the finally code is emitted
>> (2 + (number of catch blocks)) times.
>
> Thanks for noting this! I do furter analysis and update the page.
>
> Another remark about filter implementations: All filters should work without line number information.

I agree this would be ideal. However, I think a finally block dedup implementation that did not rely on line numbers would be much more complex.

> --
>
>

[Marc Hoffmann]

Hi Martin,

the team and resources are simply too limited to put too much technical
depth into the project. So, yes I think we need a clean and simple to
use API first. The analyzer is really critical core component that we
really need to keep maintainable.

But as said before: I really value your contributions and all the
filters you implemented (and the corresponding tests) provide excellent
use-cases for the new API.

Concerning the finally filter: Imagine the filter could directly work
in a tree API and emit results to an interface like

IFilterOutput.ignore(instruction)
IFilterOutput.merge(instruction1, instruction2)

Wouldn't the finally filter be much simpler and readable than the
implementation is today?

Cheers,
-marc

[Martin Hare Robertson]

Inline.

On 14 Jan 2013, at 18:32, Marc Hoffmann <hoff...@mountainminds.com> wrote:

> Hi Martin,
>

> the team and resources are simply too limited to put too much technical depth into the project. So, yes I think we need a clean and simple to use API first.

I'm not convinced that exposing an external filter API should be an initial goal. My existing filters are made simpler by being an internal part of JaCoCo.

> The analyzer is really critical core component that we really need to keep maintainable.

Given the limited resources I'm keen to keep my changes as minimal as possible and, where sensible, to avoid adding complexity in the name of speculative future work.

> But as said before: I really value your contributions and all the filters you implemented (and the corresponding tests) provide excellent use-cases for the new API.

Thanks :)

> Concerning the finally filter: Imagine the filter could directly work in a tree API and emit results to an interface like
>
> IFilterOutput.ignore(instruction)
> IFilterOutput.merge(instruction1, instruction2)
>
> Wouldn't the finally filter be much simpler and readable than the implementation is today?

The problem is that the finally filter needs to examine the complete instruction stream within a method to try and spot duplicate sequences - a process which currently relies on line numbers. This doesn't fit with the Visitor pattern used by ASM - I currently do the main work on MethodAnalyzer.visitEnd(). Every other filter which I wrote worked fine as a Visitor.

I would like to see another example of a desirable filter as complex as the finally filter before trying to extract out a common API.

My existing filter API in my fork was driven by the requirements of the four filters which I wrote.

> --
>
>

[Marc R. Hoffmann]

Hi Martin,

I agree the visitor is hard to use for complex filtering scenarios.
That's why I propose to use the tree API, see ASM docs:

http://asm.ow2.org/asm40/javadoc/user/org/objectweb/asm/tree/package-summary.html

Internally we already build the tree for every method so the performance
impact shouldn't be that bad.

-marc

--
Marc Hoffmann
hoff...@mountainminds.com
_______________________________________________
Mountainminds GmbH & Co. KG

Nussbaumstr. 4 * 80336 Muenchen * Germany
Phone/Fax +49-700-68664637 * 0700-MTNMINDS

Registergericht Muenchen * HRA 80201
Mountainminds Verwaltungs GmbH
Registergericht Muenchen * HRB 143183
Geschaeftsfuehrer Marc Hoffmann

[Marc R. Hoffmann]

Martin, concerning your filtering implementation: I think you should
consider exception handler offsets instead of line numbers.

-marc

On 14.01.13 21:15, Martin Hare Robertson wrote:

[Martin Hare Robertson]

Sadly the exception handler offset only points to the start of the catch anything block. It doesn't mark the end of the catch block and it doesn't mark the start of the finally block copies.

The challenge is to find the corresponding copies of instructions without any false positives for any other instruction sequences that could be identical:

try {
A
} catch (X) {
B
} finally {
C
try {
D
} catch (Y) {
E
} finally {
C
}
C
}

I'm not entirely sure how you would deduplicate this properly (without using my approach relying on line numbers and tight integration within the MethodAnayzer). I suspect it would work something like this:

* Find the start of the catch(Anything) block and search forwards for the corresponding throw instruction. Be careful to avoid matching a throw instruction from a nested try/finally block.
* Find the goto at the end of the try block. This should jump to the start of the no-exception copy of the finally block.
* Find the predecessors of the label after the end of the no exception finally block copy. These should be the end of the try/catch/finally exception blocks finally copies.

I think this would be enough to find the start/end of the duplicate sequences. However, this leaves the question of how to best use this information to have the effect of propagating coverage amongst the duplicates without it then being propagated backwards through the instruction flow beyond the start of a finally block copy.

Do you think there is a simpler solution or do you think we need to embrace this complexity and come up with a "proper" implementation using an external API and not relying on line numbers even if it is many extra lines of code to write/test/maintain?

> --
>
>

[Marc R. Hoffmann]

> Find the start of the catch(Anything) block and search forwards for
> the corresponding throw instruction. Be careful to avoid matching a
> throw instruction from a nested try/finally block.

You probably need to consider the data flow here: The matching throw
instruction is the one that throws the exception that has been stored
before.

I agree that a naiv implementation of such searches will result in
unmaintainable code. Imagine we would have something like regex on tree
structures, or a formal grammar to describe such situations. Then we
have clear separation of concerns:

a) a well tested generic matcher (hopefully some standard alorithm)
b) a simple description of the pattern for every filter

[mchr3k]

FYI - I have completed a rewrite of my finally block dedup code. You can see the results here: https://github.com/mchr3k/jacoco/blob/filters/org.jacoco.core/src/org/jacoco/core/internal/analysis/MethodAnalyzer.java

The code isn't very pretty as there seem to be quite a lot of different bytecode patterns which need to be handled.

@marchof I think your work in jacoco-playground-filters might enable a cleaner implementation.

[Marc R. Hoffmann]

Hi Martin,

thanks for the update! I'm also currently playing with a finally filter
in my playground project (not yet checked in), so I will also study your
aproach.

Cheers,
-marc

On 23.02.13 02:07, mchr3k wrote:
> FYI - I have completed a rewrite of my finally block dedup code. You can
> see the results here:
> https://github.com/mchr3k/jacoco/blob/filters/org.jacoco.core/src/org/jacoco/core/internal/analysis/MethodAnalyzer.java
>
> The code isn't very pretty as there seem to be quite a lot of different
> bytecode patterns which need to be handled.
>
> @marchof I think your work in jacoco-playground-filters

> <https://github.com/marchof/jacoco-playground-filters> might enable a

> <hoff...@mountainminds.com <javascript:>> wrote:
> >
> >> Martin, concerning your filtering implementation: I think you
> should consider exception handler offsets instead of line numbers.
> >>
> >> -marc
> >>
> >> On 14.01.13 21:15, Martin Hare Robertson wrote:
> >>> Inline.
> >>>
> >>> On 14 Jan 2013, at 18:32, Marc Hoffmann

> <hoff...@mountainminds.com <javascript:>

> >>>>>>> [3] mailto:hoff...@mountainminds.com <javascript:>
> >>>>>>> [4]
> https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.FINALLY
> <https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.FINALLY>
> >>

[Martin Hare Robertson]

If you haven't already noticed, you will need to deal with different bytecode patterns produced by the JDK vs Eclipse java compilers.

Let me know when you do publish any of your finally dedup work. I look forward to reading your version.

> --
> You received this message because you are subscribed to the Google Groups "JaCoCo Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to jacoco-dev+...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>