Community
Participate
Working Groups
Build Identifier: 20100218-1602 Any Java Class that is rebuild by the IncrementalImageBuilder is falsely assumed to have structurally changed if it features an Annotation with a String Parameter (e.g. @org.hibernate.annotations.AccessType(value="field") ). * true even if the Java Class hasn't changed at all. * also true for other Types of Annotation Parameters. * may lead to unnecessary build loops up to MaxCompileLoop (=5) triggering a full build (which hurts) Reproducible: Always Steps to Reproduce: 1. Insert a whitespace into a java file that has a Class-Annotation with a String parameter 2. Trigger an incremental build by saving the java file 3. Breakpoint at org.eclipse.jdt.internal.compiler.classfmt.ClassFileReader:973 (last 'return true' in 'hasStructuralAnnotationChanges')
I'll investigate.
Here is my suspicion: In the method org.eclipse.jdt.internal.compiler.classfmt.ClassFileReader#hasStructuralAnnotationChanges(...) the values of annotation parameters are compared via 'equals'. Values are instances of subclasses of 'Constant', e.g. 'StringConstant' or in case of multivalued parameters of 'Object[]'. Neither do override the 'equals'-Method, so an identiy-comparison is performed which is wrong here. Proposal: Compare the String-Representation of the key / value - Pair (ElementValuePairInfo). That seems to work even for nested Annotation-Parameters. if (!currentPairs[j].toString().equals(otherPairs[j].toString())) return true;
You are right concerning the missing equals methods on the different constants. However I cannot get a recompilation of dependents. The actual bytes are identical if you just add a space into the .class file. So we only dump the new bytes to override the existing bytes even if we think thet are identical (I will investigate that part to see why we are doing this), but we don't actually recompile dependent types. Do you have a test case that shows recompilation of dependents ?
(In reply to comment #3) > So we only dump the new bytes to override the existing bytes even if we think > thet are identical (I will investigate that part to see why we are doing this), > but we don't actually recompile dependent types. It seems that this is done to make sure the timestamps of the class files are newer than the timestamps of the source files. > Do you have a test case that shows recompilation of dependents ? Using the debug mode, I could not see that dependents are recompiled. How did you notice that dependent types are recompiled by the incremental builder ?
Created attachment 172798 [details] Three dependent Java Classes with Annotations A non structural change to class A (Newline or change inside the Constructor) will lead to a second, unnecessary build loop. But no recompilation of dependents will occur. Breakpoints: * IncrementalImageBuilder:120 (first line in the while-loop inside 'build') * ClassFileReader:973 (as mentioned above)
I see clearer now: * Inserting a space indeed doesn't change the class file. Inserting a Newline (which i did) will, if debug-information for line numbers is written to the class file. * Doing this with the submitted test project, i can provoke an additional, unecessary build loop due to falsely detected structural changes, but as you already found out, no further propagation will occur because the compiled dependents are byte-identical to their previous class files. So here comes the speciality in our real life project: We introduced hibernate bytecode enhancement via a second Builder a while ago. So there are some Classes that will never be byte-identical to their previous, enhanced class files, when recompiled. If such an enhanced class is a direct dependent of the initially changed class, then there will be at least a third build loop, potentially more if the enhanced classes form a dependency chain. So i have to admit that the reported bug is only responsible for the first of our unnecessary build loops. Nevertheless it would help us to have it fixed in situations where non-structural changes are performed on non-enhanced classes, which does occur often. Any hint about how to use bytecode enhancement without the compile time punishment would be appreciated.
Created attachment 172940 [details] Proposed fix + regression tests
Srikanth, please review for 3.6.1 inclusion. This can cause a cascade of recompilation that we should avoid if we can.
Created attachment 172981 [details] Proposed fix + regression tests Updated patch with more regression tests.
Released for 3.7M1.
(In reply to comment #8) > Srikanth, please review for 3.6.1 inclusion. This can cause a cascade of > recompilation that we should avoid if we can. Patch looks good, some comments: 1) If there was a nuance behind the way the hashCode computation happens in some of the methods, it wasn't clear to me. For e.g: org.eclipse.jdt.internal.compiler.impl.BooleanConstant.hashCode() public int hashCode() { final int prime = 31; int result = 1; result = prime * result + (this.value ? 1231 : 1237); return result; } could have been: public int hashCode() { final int prime = 31; return prime + (value ? 1231 : 1237); } or better yet: public int hashCode() { return value ? 1231 : 1237; } Similarly, org.eclipse.jdt.internal.compiler.impl.IntConstant.hashCode() could have been: public int hashCode() { return this.value; } 2) Similarly, equals methods were slightly more verbose than could have been, e.g: (org.eclipse.jdt.internal.compiler.impl.IntConstant.equals(Object)) public boolean equals(Object obj) { return obj instanceof IntConstant && this.value == ((IntConstant) obj).value; } but this is a matter of taste and didn't find anything wrong per se with the current code. 3) The comment in many of the tests in the first half of new tests in AnnotationDependencyTests reads // verify that B was recompiled It should read // verify that B was NOT recompiled 4) Just in the way of experimentation I commented out the hashCode & equals methods in IntConstant and expected that the test org.eclipse.jdt.core.tests.builder.AnnotationDependencyTests.testTypeAnnotationDependency3() to fail, but it didn't. However, if I did the same things with StringConstant and ByteConstant, the associated tests did fail. I didn't debug this mystery any further. 5) Is testTypeAnnotationDependency15 the same as testTypeAnnotationDependency2 ? I didn't notice any material difference.
(In reply to comment #11) org.eclipse.jdt.core.tests.builder.AnnotationDependencyTests.testTypeAnnotationDependency3() > to fail, but it didn't. However, if I did the same things with > StringConstant and ByteConstant, the associated tests did fail. I didn't > debug this mystery any further. This is because the IntConstant has a pool of predefined constants. I changed the test to be outside of the pool. > 5) Is testTypeAnnotationDependency15 the same as > testTypeAnnotationDependency2 ? > I didn't notice any material difference. Fixed.
Created attachment 174195 [details] Additional patch to fix issues reported by Srikanth This doesn't change most of the equals methods, but simplifies most of the hashCode methods. All these methods were generated.
Backported to 3.6 maintenance stream.
Thanks a lot for the quick response and the fix! One last question: Does the backport to '3.6 maintance stream' mean that the fix will be released before 3.7 ?
(In reply to comment #15) > One last question: Does the backport to '3.6 maintance stream' mean that the > fix will be released before 3.7 ? Yes, this means it will be available in 3.6.1 release which is planned for September.
You can try a M-build to make sure that the problem is gone.
Super, Thx!
Verified for 3.7M1 using build I20100802-1800
Change status to RESOLVED as this needs to be reverified for 3.6.1.
Verified for 3.6.1 RC2 using Build id: M20100825-0800
