Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Incremental build #1816

Closed
wants to merge 11 commits into from
Closed

WIP: Incremental build #1816

wants to merge 11 commits into from

Conversation

Egor18
Copy link
Contributor

@Egor18 Egor18 commented Jan 8, 2018

Hi. Since build-outdated-file feature is broken, I've implemented incremental build by myself thanks to @msteinbeck's concept (#1688 (comment)).
This implementation allows us to reuse the model from the previous build so, we need to rebuild only outdated sources. The model is serialized and stored in a file.
For now, I add incremental build as a test, but probably it can be integrated into spoon itself.

However, I have some question, considering incremental build.
Is it really necessary to add .class files of unmodified types into source classpath of a new build, even when we reuse the old Factory?
I added .class files in spoon 6.0.0. and it worked fine, but in spoon 6.2.0 I got InvalidClassPathException(".class files are not accepted in source classpath.")
So, I simply commented out addition of .class files in this PR, because it seems, that it does not affect anything, and my tests are passed.
What do you think?

Thank you.

@surli
Copy link
Collaborator

surli commented Jan 8, 2018

Hi @Egor18 thanks for your contribution here!

For now, I add incremental build as a test, but probably it can be integrated into spoon itself.

I vote for integrating it directly in Spoon: as we already stated, the current mechanism is broken in Spoon, so if you developed something which can be useful, go for it!

I added .class files in spoon 6.0.0. and it worked fine, but in spoon 6.2.0 I got InvalidClassPathException(".class files are not accepted in source classpath.")

Yes, I changed that because the ".class" files were never taken into account and I wanted to avoid surprising clients. If you specify the parent directory of your classes as sourceClasspath everything's should be fine as all the classes will be considered for your build.


for (File classFile : classFiles) {
String relative = mOldBinaryOutputDirectory.toPath().toAbsolutePath().relativize(classFile.toPath().toAbsolutePath()).toString();
String suffix = FilenameUtils.removeExtension(relative) + ".java";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How this trick works when you have a compilation unit with multiples classes? Or an inner class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, thanks. I'll try to make a better algorithm for matching .java files and .class files.
Also, if some file A.java was not changed, we need to add not only A.class into classpath, but all A$...class as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msteinbeck, Thank you, that's very handy.

launcher.buildModel();
launcher.getModelBuilder().compile(SpoonModelBuilder.InputType.FILES);
saveFactory(launcher.getFactory(), new File(binaryOutputDirectory, "model"));

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should do some assertions here, to check that the factory is as expected.

List<File> incrementalSources = incrementalTool.getInputSourcesForIncrementalBuild();
String[] incrementalClasspath = incrementalTool.getClasspathForIncrementalBuild();
Factory incrementalFactory = incrementalTool.getFactoryForIncrementalBuild();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment: you got a particular instance of IncrementalTool it could be great to assert it before continuing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what do you mean by that? What exactly I should check about incrementalTool?


FileUtils.deleteDirectory(PROJECT_DIR);
PROJECT_DIR.mkdirs();
createFile(new File(PROJECT_DIR, "A.java"), "public class A { int x; }");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why you don't put the resources files directly in Spoon: you could copy the resources in a temp dir for tests and apply your changes on the temp dir.

@msteinbeck
Copy link
Contributor

Is it really necessary to add .class files of unmodified types into source classpath of a new build, even when we reuse the old Factory?

I do this to ensure that the JDT batch compiler is aware of my unmodified source files and, thus, is able to resolve dependencies that have been compiled in one of the previous iterations. In order to maximize speed, I compile the changed source files only.

@msteinbeck
Copy link
Contributor

I really like your idea of serializing the factory into a file :).

@Egor18
Copy link
Contributor Author

Egor18 commented Jan 16, 2018

I added the new constructor for Launcher, that allows to perform an incremental build.

Had to remove testSourceClasspathDoesNotAcceptDotClass and check for .class files in the classpath, because I need to select only particular .class files, hence I cannot simply specify the parent directory (alternatively we could copy them to some temporary directory, but I don't really like this idea).

Also, I refactored tests and changed algorithm for matching .java files and .class files.


@Test
public void testIncremental() throws Exception {
final File TEST_DIR = new File("./src/test/resources/inremental-test");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inremental-test -> incremental-test

@Egor18
Copy link
Contributor Author

Egor18 commented Feb 19, 2018

Still working on that. It's not really ready yet. I was testing incremental build on some real projects, and found some issues.

@monperrus monperrus changed the title Incremental build WIP: Incremental build Feb 20, 2018
@monperrus
Copy link
Collaborator

OK added WIP in the PR title.

@monperrus
Copy link
Collaborator

Also could you add API documentation for the public methods?

@Egor18
Copy link
Contributor Author

Egor18 commented Mar 7, 2018

Hi. I'm working on incremental build (actually I'm implementing it again).
And I have one tricky question considering type reference resolution.
Let's say we have the following source files:

C1.java:

class C1 {
    int val;
}

C2.java:

class C2 {
    void f(C1 c) {
        c.val = 42;
    }
}

For the first time, we build model from scratch and save it.

After that, let's change type of the val field in C1 class:

class C1 {
    float val;
}

Now we can build model incrementally, using previously saved model.
This means, that we need to rebuild only C1 class because only C1.java file has been changed.
However, we still have to update type references in all classes, which use C1.
Without it, type reference of c.val in C2 is still int (but we changed type to float!).

I see few approaches:

  1. Call getReferencedTypes() from every top-level class, and if some top-level class references changed class, rebuild it as well (the whole file).
    Probably I can even cache referenced types for every top-level class.
    This approach is quite simple to implement, but in cases when some widely used class was changed (e.g. Utils class), you would probably have to rebuild almost everything.

  2. Compare old class AST and new class AST, to find out which fields/methods were changed.
    Create some visitor, that updates all type references to changed fields/methods.
    For instance, if type reference of the c.val field has been changed, scanner would update type reference of c.val = 42 in parent assignment and so on.

So, what do you think? What is the best approach to update such type references? Are there any other approaches?

@msteinbeck
Copy link
Contributor

From what I can tell, references are resolved lazily. Thus, you don't need to update unchanged files as their references will automatically point to the updated AST Nodes.

@Egor18
Copy link
Contributor Author

Egor18 commented Mar 7, 2018

@msteinbeck, In my example type references are resolved, but they refer to invalid old type int, instead of new type float of the field val.

@monperrus
Copy link
Collaborator

Thanks for the follow up.

Before adding new code, it may be a good idea to do an initial PR to first remove all code related to the completely broken 'build-outdated' feature.

@Egor18
Copy link
Contributor Author

Egor18 commented Mar 10, 2018

Moved to #1905

@Egor18 Egor18 closed this Mar 10, 2018
@Egor18 Egor18 deleted the incremental_build branch October 5, 2018 09:30
@Egor18 Egor18 restored the incremental_build branch October 5, 2018 09:31
@Egor18 Egor18 deleted the incremental_build branch October 5, 2018 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants