Support incremental build for Javac

[testforstephen]

Fixes #443
Fixes #532

[mickaelistria]

I think you can already merge the 1st commit ( 585db98 ) if you think it's good enough. For the 2nd one, we should spend some a bit more time reviewing it to evaluate whether we can find some idea that doesn't require change in JDT API.

[testforstephen]

I think you can already merge the 1st commit ( 585db98 ) if you think it's good enough. For the 2nd one, we should spend some a bit more time reviewing it to evaluate whether we can find some idea that doesn't require change in JDT API.

As we discussed, the compiler itself (e.g., ECJ, Javac) doesn't provide incremental build capabilities. Instead, it's the wrappers like BatchImageBuilder and IncrementalImageBuilder in the JDT core that are responsible for recording the build state and implementing incremental builds. When switching to Javac, we need to modify the upstream JDT IncrementalImageBuilder to recognize the Javac build results. If we agree to proceed with this technical approach, I will open a pull request in jdt.core to extend the CompilationResult interface to support alternative compiler results.

@mickaelistria WDYT?

[mickaelistria]

Thanks; I'm not sure I fully get the problem, so I'm going to ask probably dumb questions, sorry!

I'm trying to get a clear enough understanding of what really needs to change in JDT. Do I understand it right that:

• We don't particularily need anything new in CompilationResult? Or what are we missing?
• We can already create a subtype of ClassFile to call CompilationResult.record()?
• There is nothing to wrong in calling writeClassFile, beyond a performance cost ?

What is the particular point requiring change in JDT?

[testforstephen]

They are great questions.

• We don't particularily need anything new in CompilationResult? Or what are we missing?
• We can already create a subtype of ClassFile to call CompilationResult.record()?

Yes, we don't need change the API for CompilationResult. We can create subtype to populate the ClassFile and other state data to CompilationResult without making changes to JDT.

• There is nothing to wrong in calling writeClassFile, beyond a performance cost ?

The existing writeClassFile brings two issues for Javac:

• The AbstractImageBuilder will overwrite the class file contents after Javac generates the class file. While we can set the Javac-generated class bytes to the VirtualClassFile and allow the AbstractImageBuilder to write the file again, this is not a critical problem. I can live with it if we don't change the upstream code.

• The IncrementalImageBuilder does not recompile dependent sources for Javac. The classFileChange method reads the old file bytes and compares them with the new bytes to detect structural changes. If structural changes are found, it propagates the dependent sources into the build loop via addDependentsOf(new Path(fileName), true). However, this approach fails for Javac because Javac already writes the latest bytes to the file, causing classFileChange to always see the file as unchanged. This is the real issue that makes me to change the upstream JDT code.

eclipse-jdt-core-incubator/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/IncrementalImageBuilder.java

Lines 935 to 951 in 8397f18

	protected boolean classFileChanged(IFile file, String fileName, byte[] newBytes) throws CoreException {
	try {
	byte[] oldBytes = Util.getResourceContentsAsByteArray(file);
	if (Arrays.equals(oldBytes, newBytes)) {
	return false;
	}
	URI location = file.getLocationURI();
	if (location == null) return false; // unable to determine location of this class file
	String filePath = location.getSchemeSpecificPart();
	ClassFileReader reader = new ClassFileReader(oldBytes, filePath.toCharArray());
	// ignore local types since they're only visible inside a single method
	if (!(reader.isLocal() || reader.isAnonymous()) && reader.hasStructuralChanges(newBytes)) {
	if (JavaBuilder.DEBUG)
	System.out.println("Type has structural changes " + fileName); //$NON-NLS-1$
	addDependentsOf(new Path(fileName), true);
	this.newState.wasStructurallyChanged(fileName);
	}

eclipse-jdt-core-incubator/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/IncrementalImageBuilder.java

Lines 874 to 884 in 8397f18

	protected void writeClassFileContents(ClassFile classfile, IFile file, String qualifiedFileName, boolean isTopLevelType, SourceFile compilationUnit) throws CoreException {
	// Before writing out the class file, compare it to the previous file
	// If structural changes occurred then add dependent source files
	byte[] bytes = classfile.getBytes();
	if (file.exists()) {
	if (classFileChanged(file, qualifiedFileName, bytes) || compilationUnit.updateClassFile) { // see 46093
	if (JavaBuilder.DEBUG)
	System.out.println("Writing changed class file " + file.getName());//$NON-NLS-1$
	if (!file.isDerived())
	file.setDerived(true, null);
	file.setContents(bytes, true, false, null);

[mickaelistria]

OK, I see. So the expectation is that the Compiler doesn't directly modify the class files here, but instead that it returns the bytes, and the ImageBuilder takes care of persisting the bytes where it makes sense.
With javac, we're doing too much by generating/touching the files. I'm afraid that if we let Compiler change the file directly while the consumers are designed differently, we will face more issues down the road. I don't think we can easily not touch/write those class files with Javac, but could we imagine our compiler writing the files into a temp directory instead of their actual target, and creating JavacProxyClassFile that would reference the temp file and return the bytes and so on as expected, so that the classFileChanged and writeClassFile would work as expected with it without need to change the upstream flow?

[testforstephen]

Generating Javac result into a temp directory might work. But we definitely need to find ways to clean the temp output with the clean build or rename operations. I can make some experiment to see how far it can go.

[testforstephen]

Another question about the force-pushed action. Why do I see so many commits in the PR? Rebasing my branch on the target branch (dom-with-javac) should only include my changes in the PR.

[mickaelistria]

Why do I see so many commits in the PR?

I rebased the dom-with-javac branch on top of upstream JDT master branch. As a result, all the commits that are not from upstream appear as new in the PR.

[mickaelistria]

That looks good, the design seems to use perfectly existing JDT capabilities. I'll merge it as soon as build completes.
It would probably be interesting to identify a minimal test case to run as part of this suite. I'm pretty sure that JDT already offer such a test case, so we may just reference it in the org.eclipse.jdt.core.tests.javac bundle so it gets executed with the flags to enable javac compiler.

[testforstephen]

from the feature point of view, the new approach can effectively support incremental build and it's ready for merge. I'm ok to merge it first. I'll see how to add some unit tests later.

[mickaelistria]

Thank you for this excellent addition!