CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 137
Incremental Compilation #334
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
Conversation
…nges in input jar or class files
…iler active. Added timestamp for logging to spot bottlenecks.
# Conflicts: # compiler/compiler/src/main/java/org/robovm/compiler/AppCompiler.java
Thanks! Looks good to me, @dkimitsa what do you think? |
@florianf hi, will take a look tomorrow. thanks |
Looks good to me, will merge in the next days if no one has objections. |
Hi Florian, that's great. I've been working with this version of the plugin in the last weeks and can still confirm that there are no problems. I just saw, that the required copyrights were missing and one was mistakenly copied into a new class. Fixed that. Best wishes! |
@dthommes Haven't spent time looking in detail so I may have missed something obvious but could you please clarify how it improves the existing incremental compilation? RoboVM doesn't recompile every time but only when changes to classes are made, isn't that incremental compilation? Thanks. |
@obigu
In short: This PR is about skipping steps 2-4 for classes in your project and steps 5-6 for all classes. Longer info: Being incremental means, that the compiler should only execute the compilation process starting with a certain step, when the input for the same step has changed since the last run. So let's say, this PR adds additional incremental behavior to the existing one. |
Cool, thanks for the explanation, this is actually an improvement for the very specific (but quite common) scenarios of not changing any class. One question, what happens when MobiVM version is updated? Will users need to remember to manually clean the build folder after the update? |
there is a correction, all project classes.o go to probably your changes affects linking, deployment preparation and code sign. I will check code later today. but first comment from my side:
|
@obigu @dkimitsa Regarding switching the functionality of this PR: The compiler already reads the "clean flag" that can be given via the Config ( |
@dthommes |
@dkimitsa Okay, so this might be a global setting and should be in a preferences pane, right? |
it would be enough to have it config |
Sorry, I don't understand. Is there a way to modify the config anyways? |
sorry for confusion, you mentioned that there is but my point is that functionality added by this PR should have options to be turned on or off. and it shall be off by default. and it might be good to have it as option in config, similar to |
Ah, okay. Then I would add an element for |
… incremental compilation
There you go: Just add
to your robovm.xml to enable the new feature. Otherwise, it will be disabled. Get an impression of its effect on the restart time here: |
Thanks @dthommes . I think it's a nice addition and I like the fact that for the moment it's only opt-in via configuration (while it's experimental). My only minor criticism would be that as confirmed by @dkimitsa this change is not really about compilation (or incremental compilation) but about skipping linking, code sign, etc... The name of the property |
Yes, if everybody agrees with |
@dthommes : Please re-name it, I think the name is o.k. Then I'll merge the PR. Thanks a bunch! |
@florianf please keep it open for this weekend, as I had no time yet to look through it. sorry for delay |
Renamed as suggested. |
@@ -253,6 +254,10 @@ public static void actool(Config config, File partialInfoPlist, File inDir, File | |||
} | |||
} | |||
|
|||
//Prepares for incremental build - not applied at this time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add comment what it affect to and how it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a longer comment here.
@@ -223,12 +231,16 @@ public void run() { | |||
}); | |||
} | |||
|
|||
private static String getNowString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename it to something like getFormattedTimeStamp
just to keep it same as everywhere in the world
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
public static void logInfo(Project project, String format, Object... args) { | ||
log(project, ConsoleViewContentType.SYSTEM_OUTPUT, "[INFO] " + format, args); | ||
log(project, ConsoleViewContentType.SYSTEM_OUTPUT, "[INFO] " + getNowString() + format, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need these timestamps at every day basics ? As it useful during development but just mess an output when you don't care. Probably it might be useful to use it in case when RoboVM is running in development mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need them. It enables everybody to see, when there's a problem. Additionally it has the potential to motivate other people to contribute. E.g. somebody could be motivated to contribute logic to skip the slow interface compilation or asset creation processes (example from my machine):
[INFO] 10:22:33.071 /Applications/Xcode.app/Contents/Developer/usr/bin/ibtool ...
[INFO] 10:22:35.726 /Applications/Xcode.app/Contents/Developer/usr/bin/actool ...
[INFO] 10:22:38.892 /* com.apple.actool.compilation-results */
So, let's keep them.
} | ||
|
||
//Has the configuration changed between runs (e.g. forcelink)? | ||
boolean configsEqual; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why just don't check timestamps of robovm.xml, robovm.properties and Info.plist.xml files ?
it will do same but will remove lot of code that was added/changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a dependency to robovm.xml and the like would imply, that you're always configuring RoboVM via these files. This will fall back on you, if you once decide to make a configuration via Gradle or Maven (used today by javafxports and desirable anyways IMHO as of single-source-of-truth principle).
In fact, today you're configuring parts of the build process via the IDEA run configuration (e.g. the architecture - and please don't let us take this information from the build path).
So I think, comparing the configurations this way is required.
|
||
//Check for newer input files compared to Main binary | ||
File classPathsFile = new File(config.getTmpDir(), CLASSPATHS_FILENAME); | ||
File binaryFile = new File(config.getTmpDir(), "Main"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bug here: application name should be checked from config and not hardcoded to Main
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
*/ | ||
private boolean needsRecompilation(Config config) throws IOException { | ||
if (!config.isSmartSkipRebuild()) { | ||
config.getLogger().info("Rebuilding because smartSkipRebuild is disabled. Enable it by adding " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove it as we should not advertise any configuration when it not requested. especially experimental ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
hi @dthommes |
@dkimitsa Thanks for the review. I think this clarified the situation. |
BTW: I have tested the latest changes by building RoboVM in a whole and testing the IDEA plugin. It should be working fine. |
@dthommes btw, have you tested it with gradle plugin |
Yes (see first comment). |
Additionally: Tested it with Maven a few days ago after fixing the errors from the wrong template. Works as expected as Maven cleans the build folder. |
@@ -12,4 +12,6 @@ | |||
<directory>resources</directory> | |||
</resource> | |||
</resources> | |||
<!-- Set to 'true' to trigger compilation and linking depending on changes in IDEA --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another moment/bug-like:
please comment out <smartSkipRebuild>false</smartSkipRebuild>
as when previous compiler is used (without IC) it crashes with exception
ElementException: Element 'smartSkipRebuild' does not have a match in class org.robovm.compiler.config.Config
I understand that new compiler will handle it but it is better to keep compatibility with older ones.
Probably better case would be to have it commented out with true
state and comment saying to uncomment line bellow to enable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this.
…kipRebuild element
@dkimitsa Is this mergeable now? |
Yep, it works for me. Thanks |
Merged, thanks guys! |
As discussed in the Gitter channel, I have added incremental compilation capabilities to RoboVM's compiler for the case that no classes change between runs. This speeds up the build process for scenarios where you only modify resources or simply restart the app for hitting a certain break point in debug mode. Here are some details about the changes:
Changes in RoboVM Compiler
classpath_dependencies.txt
into the build folder. This file contains information about the classes and jar files that were used to build the Main binary. On the next run (and if this file is still available), the compiler compares the creation dates of the files mentioned in this file and the Main binary. Only, if one of the given files is newer, recompilation is triggered.config.xml
), the compiler's configuration is written into the build folder. With the next run, the configurations are compared. If there are changes (e.g. forcelink settings), recompilation is triggered.assetcatalog_dependencies
is now created by XCode's asset tool. With this file, it is possible to check whether assets have been changed between runs. It would be possible to skip asset packaging, if there are no changes. This is not implemented, yet.These changes only take effect, if RoboVM's build folder is not cleaned between runs, which was the default case for all plugins before the below mentioned changes.
Changes in IDEA Plugin
robovm-build
folder for each build. I have added a new menu entry in the RoboVM menu (Clean Build Folders
) to do this manually instead. It was not possible to let the plugin run the clean process as part of IntelliJ's default clean action (Build -> Rebuild Project
).RoboVM -> Clean cache
toRoboVM -> Clean Global Cache
to make a clear distinction between the actions.Tests
Speed-Up
For our extensive application with about 7000 classes, restarting the app without changes can now be done in 14 instead of 39 seconds.