Exclude from a report a part of bytecode that compiler generates for a synchronized statement and that is hard to cover by tests #501

Godin · 2017-03-19T12:47:14Z

No description provided.

Godin · 2017-03-25T20:21:30Z

Analysis of 17391 classes from rt.jar (≈ 65 Mb) from JDK 1.8.0_121 using following build.xml

<project xmlns:jacoco="antlib:org.jacoco.ant" default="report">
  <taskdef uri="antlib:org.jacoco.ant" resource="org/jacoco/ant/antlib.xml">
    <classpath path="jacocoant.jar"/>
  </taskdef>
  <target name="report">
    <jacoco:report>
      <executiondata>
        <file file="jacoco.exec"/>
      </executiondata>
      <structure name="rt.jar">
        <classfiles>
          <fileset file="rt.jar"/>
        </classfiles>
      </structure>
    </jacoco:report>
  </target>
</project>

takes 5 seconds on my laptop, both without this change as well as with it. So I believe we are safe to merge this.

@marchof do you have objections or some review comments?

marchof

@Godin I added several comments for things that could be improved from my point of view (especially the documentation issues).

The unit testing facilities I commented about can be improved later from my point of view.

marchof · 2017-03-27T17:35:25Z

org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/IFilter.java

+
+import org.objectweb.asm.tree.MethodNode;
+
+public interface IFilter {


This is the central API for filters. We should add some JavaDoc here. Especially about the usage contract: E.g. IFilter instances are re-used and must be thread safe.

@marchof done ✅

marchof · 2017-03-27T17:35:58Z

org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/IFilterOutput.java

+
+import org.objectweb.asm.tree.AbstractInsnNode;
+
+public interface IFilterOutput {


JavaDoc would be nice too.

@marchof done ✅

marchof · 2017-03-27T17:36:30Z

org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/IFilterOutput.java

+
+public interface IFilterOutput {
+
+	void ignore(AbstractInsnNode from, AbstractInsnNode to);


Javadoc: What exactly is the semantics of from and to?

Are there any constraints regarding from and to nodes? "to must be a successor of from in the instruction sequence. from is inclusive, to is exclusive".

@marchof Actually both are inclusive. And we already use term "successor" in analysis of CFG, where its usage implies that not each instruction is a "successor" of preceding instruction from a list 😉 so not sure that "successor" will be a good unambiguous word here.

@marchof Javadoc added ✅

marchof · 2017-03-27T17:57:10Z

org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/SynchronizedFilterTest.java

+		m.visitInsn(Opcodes.RETURN);
+
+		new SynchronizedFilter().filter(m, this);
+		assertEquals(m.instructions.get(12), from);


So what means 12 or 16 here? Maybe we should enumerate instructions with comments?

@marchof fixed ✅

marchof · 2017-03-27T18:06:32Z

org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/SynchronizedFilterTest.java

+import org.objectweb.asm.tree.AbstractInsnNode;
+import org.objectweb.asm.tree.MethodNode;
+
+public class SynchronizedFilterTest implements IFilterOutput {


From my point of view in JaCoCo core our unit tests should create full coverage on its own (with all paths). The validation tests come on top. Currently it looks like the only purpose of the unit test is the fill ECJ case.

@marchof I agree that code generated by javac should be captured in unit test and will do this.

However regarding

full coverage on its own (with all paths)

I doubt that generation of synthetic non-realistic bytecode in unit test or change of the code just to achieve full branch/path coverage makes sense. This is probably not that hard here for SynchronizedFilter, but I expect this to be really tedious in case of filter for try-with-resources.

@marchof BTW we probably can exclude/separate verification tests from unit test coverage to enforce practice of completeness of unit tests.

@marchof added bytecode that javac produces and negative test. However this does not lead to a full branch coverage (few are still missed), but at least main paths are covered ✅

@Godin No problem. I assume we will need a (well tested) general purpose matcher facility soon. Then the number of branches within the specific filters will greatly reduce.

marchof · 2017-03-27T18:10:46Z

org.jacoco.core.test/src/org/jacoco/core/test/filter/targets/Synchronized.java

+		synchronized (lock) { // $line-monitorEnter$
+			nop(); // $line-body$
+		} // $line-monitorExit$
+	} // $line-after$


What about another nop() here? Otherwise we rely on the implicit return.

Such usage seems legitime to me given that implicit return is always presented and always assigned to such line as verified by another test. Moreover we already use such pattern in Target03 and EnumImplicitMethods, however in InterfaceDefaultMethodsTarget and InterfaceOnlyDefaultMethodsTarget we use explicit return statements 😆 And BTW I've just realized that we test "before" and "after" only in case of exceptions, but not in case of control structures 😉

@marchof added nop() here so that to have a symmetry with before marker at least in this test ✅

marchof · 2017-03-27T19:27:03Z

org.jacoco.core/src/org/jacoco/core/internal/flow/MethodProbesVisitor.java

@@ -147,4 +148,8 @@ public void visitLookupSwitchInsnWithProbes(final Label dflt,
 			final int[] keys, final Label[] labels, final IFrame frame) {
 	}

+	public void accept(final MethodNode methodNode,


Whooo, here we start mixing the ASM event and tree API. I consider this as a temporary hack until we fully move to the tree API ;-)

Nevertheless we should clearly document the method, especially the point in time when it is called.

"Nothing is more permanent than the temporary" 😉 but future will show. We kind of already had such mix - see org.jacoco.core.internal.flow.LabelFlowAnalyzer#markLabels(org.objectweb.asm.tree.MethodNode), and exactly this gave me an idea of how to use ASM Tree API for filters without massive changes of everything, so this mix is worth it.

Nevertheless we should clearly document the method, especially the point in time when it is called.

@marchof Not sure I understand this point - do you want to document method or its call site or both? And in any case - do you have idea about wording?

@Godin The method itself:

/** * This method can be overwritten to hook into the process of emitting the * instructions of this method as <code>visitX()</code> events. * * @param methodNode * the content to emit * @param methodVisitor * A visitor to emit the content to. Note that this is not * necessarily this visitor instance but some wrapper which * calculates the probes. */

@marchof done ✅

marchof · 2017-03-27T19:29:42Z

org.jacoco.core/src/org/jacoco/core/analysis/Analyzer.java

@@ -108,6 +108,7 @@ public void visitEnd() {
 	public void analyzeClass(final ClassReader reader) {
 		final ClassVisitor visitor = createAnalyzingVisitor(
 				CRC64.checksum(reader.b), reader.getClassName());
+		// TODO maybe we can SKIP_FRAMES here?


I agree in the comment but separate issue? On the other hand I'm not sure what the benefit would be as frames are ignored at analysis time.

@marchof I believe that in this case FrameNode won't appear in methodNode.instructions and since they anyway not used for analysis, we won't be wasting CPU cycles to skip them in filters - see method SynchronizedFilter.Matcher#next(). While this doesn't play any (or at least not much) role for SynchronizedFilter, the same method is used in filter for try-with-resources (#500). And now that you reminded me about that - probably I can demonstrate observable impact on performance using its implementation prior to the last optimizations. I'm fine with postponing this, however this shouldn't be forgotten and in fact I almost forgot about this, but good that left this comment.

@marchof I removed comment from this PR as anyway it will be presented in #500 , so that we can come back to this later. ✅

marchof · 2017-03-27T19:34:48Z

org.jacoco.core/src/org/jacoco/core/internal/analysis/MethodAnalyzer.java

+		for (final TryCatchBlockNode n : methodNode.tryCatchBlocks) {
+			n.accept(methodVisitor);
+		}
+		for (int i = 0; i < methodNode.instructions.size(); i++) {


If a InsnList is traversed by index an internal array is created lazily. This can probably be avoided by using the linked list API.

@marchof as link in Javadoc shows - this was inspired by implementation of org.objectweb.asm.tree.MethodNode#accept(org.objectweb.asm.MethodVisitor), so strictly speaking there is no change in behavior in regards to creation of array. But you right that we can avoid this. And if so, then I'm wondering - how to prevent regression of this? Probably it can/should be avoided for instrumentation too? Are there other places that trigger or might trigger its creation?

In fact I was wrong - default implementation in org.objectweb.asm.tree.MethodNode#accept(org.objectweb.asm.MethodVisitor) does not construct array, don't know from where opposite came to my mind. Still wondering how to prevent such regressions, especially given that MethodNode#instructions is accessible by filters now.

Only suggestions that come to mind are:

Add build-time class file scanning for use of InsnList.get(int).

Create InsnList subclass that throws and use it in unittests. I guess this would require subclassing ClassAnalyzer to override visitMethod, which probably requires reimplementing Analyzer.analyzeClass/createAnalyzingVisitor.

More suggestions:

A reflection based assertion that InsnList.cache is stays null

A JaCoCo coverage check that method InsnList.toArray() is not executed during our tests.

But I woudn't worry too much right now - unless performance testing shows a bottle neck here.

And one more suggestion 😆 - InsnList as well as whole MethodNode comes to us via MethodSanitizer#visitEnd() in ClassProbesAdapter, where we can replace value of field instructions (that is public in MethodNode) by a new instance with overriden method toArray().

But I woudn't worry too much right now - unless performance testing shows a bottle neck here.

I've checked that our code doesn't trigger creation of array anymore.
So indeed - let's don't worry about this right now. ✅

marchof · 2017-03-27T19:38:59Z

org.jacoco.core/src/org/jacoco/core/internal/flow/Instruction.java

@@ -25,6 +27,8 @@

 	private Instruction predecessor;

+	public AbstractInsnNode node;


As it is always set after the instance is created this should be final and set within the constructor.

For consistency I would prefer to make it private with a public getter.

@marchof indeed - this is a leftover of an early prototyping.

@marchof fixed ✅

Godin · 2017-03-28T07:43:24Z

org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/SynchronizedFilter.java

+			if (tryCatch.start == tryCatch.handler) {
+				continue;
+			}
+			final AbstractInsnNode to = new Matcher(tryCatch.handler).match();


Number of allocations of Matcher can be reduced here - can allocate and reuse just one for the whole lifetime of a method instead of allocation for TryCatchBlockNode.

… as linked list

…r` marker instead of implicit return

marchof · 2017-03-28T16:51:15Z

@Godin Thanks for following up all my picky comments ;-) Let's merge this and continue with another filtering PR. I assume the overall implementation will improve step by step as we see more requirements and use cases.

Godin · 2017-03-28T16:55:04Z

@marchof Hooray! 😋 Will do squash and merge quickly and then will rebase #500 so that to continue with filter that doesn't require changes outside of filter 😉

Godin added component: core.filters type: enhancement labels Mar 19, 2017

Godin self-assigned this Mar 19, 2017

Godin force-pushed the issue-501 branch 2 times, most recently from 06ba2f1 to 117d36e Compare March 19, 2017 13:53

Godin added this to TODO in Filtering Mar 19, 2017

Godin added this to IN PROGRESS in Current work items Mar 19, 2017

Godin requested a review from marchof March 19, 2017 14:42

Godin mentioned this pull request Mar 21, 2017

Exclude from a report a part of bytecode that ECJ generates for a String in switch statement and that is hard to cover by tests #496

Closed

Godin force-pushed the issue-501 branch 5 times, most recently from 39baa53 to 9618e63 Compare March 23, 2017 00:20

Godin added this to the 0.7.10 milestone Mar 25, 2017

Godin force-pushed the issue-501 branch 2 times, most recently from c5a88c1 to f0cc333 Compare March 26, 2017 03:01

Add filter for synchronized statement

f0cc333

Godin mentioned this pull request Mar 27, 2017

Exclude from a report a part of bytecode that compiler generates for a try-with-resources #500

Merged

marchof requested changes Mar 27, 2017

View reviewed changes

(wip) field Instruction#node should not be public

0f456eb

Godin commented Mar 28, 2017

View reviewed changes

Godin added 7 commits March 28, 2017 11:07

(wip) add bytecode generated by javac to SynchronizedFilterTest

854b767

(wip) add negative test to SynchronizedFilterTest

0cf500e

(wip) avoid creation of array by traversing MethodNode#instructions…

84cf00f

… as linked list

(wip) for the symmetry with before marker use nop() for the `afte…

33afe5b

…r` marker instead of implicit return

(wip) use labels for assertions

7bf8daa

(wip) add Javadocs

f9087be

(wip) add Javadoc

b16974d

(wip) remove TODO comment

18c9e4b

marchof approved these changes Mar 28, 2017

View reviewed changes

Merge branch 'master' into issue-501

5d725dc

Godin merged commit eaef191 into master Mar 28, 2017

Godin deleted the issue-501 branch March 28, 2017 17:20

Godin removed this from IN PROGRESS in Current work items Mar 28, 2017

Godin moved this from TODO to DONE in Filtering Mar 28, 2017

jacoco locked as resolved and limited conversation to collaborators Jan 11, 2018

Godin added the language: Java label Oct 12, 2023


		import org.objectweb.asm.tree.MethodNode;

		public interface IFilter {


		import org.objectweb.asm.tree.AbstractInsnNode;

		public interface IFilterOutput {


		public interface IFilterOutput {

		void ignore(AbstractInsnNode from, AbstractInsnNode to);

		@@ -25,6 +27,8 @@

		private Instruction predecessor;

		public AbstractInsnNode node;

Exclude from a report a part of bytecode that compiler generates for a synchronized statement and that is hard to cover by tests #501

Exclude from a report a part of bytecode that compiler generates for a synchronized statement and that is hard to cover by tests #501

Conversation

Godin commented Mar 19, 2017

Godin commented Mar 25, 2017 • edited Loading

marchof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchof Mar 27, 2017 • edited by Godin Loading

Choose a reason for hiding this comment

marchof Mar 27, 2017 • edited by Godin Loading

Choose a reason for hiding this comment

Godin Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Godin Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Godin Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Godin Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Godin Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Godin Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Godin Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Godin Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchof commented Mar 28, 2017

Godin commented Mar 28, 2017

Godin commented Mar 25, 2017 •

edited

Loading

marchof Mar 27, 2017 •

edited by Godin

Loading

marchof Mar 27, 2017 •

edited by Godin

Loading

Godin Mar 27, 2017 •

edited

Loading

Godin Mar 28, 2017 •

edited

Loading

Godin Mar 28, 2017 •

edited

Loading

Godin Mar 28, 2017 •

edited

Loading

Godin Mar 27, 2017 •

edited

Loading

Godin Mar 28, 2017 •

edited

Loading

Godin Mar 28, 2017 •

edited

Loading

Godin Mar 28, 2017 •

edited

Loading