fix: improve app build times #10789
fix: improve app build times #10789
Conversation
| @@ -5884,7 +5884,7 @@ iOSBuilder.prototype.copyResources = function copyResources(next) { | |||
| // dest doesn't exist, or new contents differs from existing dest file | |||
| if (!exists || newContents !== fs.readFileSync(to).toString()) { | |||
| this.logger.debug(__('Copying and minifying %s => %s', from.cyan, to.cyan)); | |||
| exists && fs.unlinkSync(to); | |||
| // no need to delete if it exists, writeFile will overwrite anyways | |||
There was a problem hiding this comment.
Android does the same thing on line 2974.
There was a problem hiding this comment.
Yes, but there it's needed. Because on Android we try to symlink files over, and if the destination is a symlink it must be removed first or the write will actually write to the symlink target (i.e. the user's original source)
|
|
Android should be comparing the post-jsanalyze to the destination like iOS. We must transpile every file every build so that we can compare the hashes and see if we need to write the new contents. This could be a huge bug. The code related to the part of the build has been touched since I first wrote it, so I won't be surprised if something slipped through the cracks. Another thing that's bugging me about this PR is if I could have parallelized this logic, I would have. There has to be some reason I didn't. I need to think hard why it wasn't parallelized. |
|
So I probably need to re-run my results here to compare a 2nd build, since the timings above are "clean"/1st builds (so it doesn't measure how a differential build logic may help/hurt each platform) We do transpile the code every build on both platforms. The difference here is that Android doesn't care if a previous build has already written the destination file - it just writes out the current transpiled contents regardless (so potentially it may write the exact same contents to the same file and cause the modification timestamp to update unnecessarily). iOS looks at the previous build's output to compare before choosing to write the contents or not. The question is whether we must compare existing files from previous builds to current build on Android like we do iOS. Or if the need to check existence and read before writes negates any benefit to doing so. |
|
Maybe I'm missing something, but where's the differential build code? Is it only resources? I went back to older 7.x and 6.x versions of the build and I question if differential checks ever worked for source files. I swear I added it. |
|
We should not compare existing files to new contents. We should be comparing the mtime from the build manifest and if that differs, then compare hashes. |
|
For more timings, here's what I got for runs of 1 clean build versus a clean build followed by a second "incremental" build pre and post-pR for each platform: iosclean build, series (pre-PR):
clean build, parallel:
multiple runs, series:
multiple runs, parallel:
androidclean build, series (pre-PR):
clean build, parallel:
multiple runs, series:
multiple runs, parallel:
To try and summarize:
|
| @@ -5818,7 +5818,7 @@ iOSBuilder.prototype.copyResources = function copyResources(next) { | |||
| this.logger.info(__('Processing JavaScript files')); | |||
| const sdkCommonFolder = path.join(this.titaniumSdkPath, 'common', 'Resources'); | |||
|
|
|||
| async.eachSeries(Object.keys(jsFiles), function (file, next) { | |||
| async.each(Object.keys(jsFiles), function (file, next) { | |||
| setImmediate(function () { | |||
There was a problem hiding this comment.
Is this setImmediate necessary anymore?
There was a problem hiding this comment.
I don't remember why the setImmediate() was there in to begin with in this situation, however I generally use setImmediate() to guarantee a block is truly async (which is sometimes necessary if returning an event emitter or there's unified error handling) or to collapse the call stack, though this function isn't very recursive.
|
Some quick benchmarks from our project (276+ JavaScript source files): AndroidTBD iOS8.1.0 (master)
8.1.0 (this pull)
Note: We use a Crashlytics script phase to upload our DSYMs after each clean build which takes ~ 10s, so that needs to be removed from the clean build times. Side notes:
That's it! With these changes, we could probably have a 5s incremental build, which is the most important thing to use. After that's done, Liveview "v2" could use that as a base to enable hot reloading but...different topic ✌️ Thanks for the great work @sgtcoolguy! |
|
Juts a general note here that I'm just looking for easy low hanging fruit perf win here, not to rewrite the build pipeline (which I think requires a much larger discussion and some tech spikes - including around possibly taking advantage of gulp/vinyl). It's clear we have an issue of Alloy/classic/hyperloop doing lots of unnecessary additional reads/writes/copies/parses when combined together. |
|
Also, to record a note that @ewanharris and I discussed.: he was concerned not using I appreciate the concerns here that just throwing so many files at once may bog the system down or hit some limit - and it is entirely possible, but I did not run into it with a fairly sizable number of files - and I'm not sure what would be a reasonable value to choose as a limit here. |
|
The limit should be set to |
|
Merged manually to master. |
|
@hansemannn, the framework scanner has incremental build support. It's just the initial scanning of paths that is done repeatedly. However, that just reads the directories and should not take a long time. No processing is done during that phase. If no frameworks in the scanned paths change the framework hook will do nothing. You can see that in the trace logs of a build. @sgtcoolguy, i think we could use the change detection from https://github.com/appcelerator/appc-tasks (which is used by the framework hook hans mentioned above) to further speed up our builds without changing too much on the existing build pipeline. I will give it a look when i have some free time. |
JIRA: https://jira.appcelerator.org/browse/TIMOB-26917
Description:
Note that this fix does not yet touch the originally reported issue that we're checking all files for changes resulting in slower builds - instead this simply makes our processing of JS files run in parallel. Specifically the code around reading the files, parsing/transpiling/minifying and then writing the results.
Here's some quick and dirty timings of just that portion of the build for me:
ios
As-is:
in parallel:
android
As-is:
parallel:
So this does substantially improve performance for me. The difference is less noticeable on large Android projects as I suspect we'll hit a general chokepoint of the babel parse/transform (perhaps moving to async APIs in
node-titanium-sdkcould help there?) But for iOS projects, and especially large ones we see a huge performance increase (in this case 29x!).Interesting to note that even with this changes there's an obvious order-of-magnitude performance difference between iOS and Android, likely due to the exact issue @hansemannn reported.
On Android, we compare the original contents to the contents post-jsanalyze (both values we already have in vars/memory at the time). If they don't differ and we can do symlinks we simply symlink the original. If they do differ we delete the destination (in case it was a symlink) and then write the new contents.
On iOS, we compare the contents post-jsanalyze to the destination file contents (here we check if the file exists and then if it does we read it in). If they match we do nothing. If the destination does not exist or is different, we write the new contents.
The issue here is how each plays with "differential" builds (and indeed if having to read the destination files in each time negates any benefit of a differential build). Android is comparing original src js to post-babel contents and ignoring if we've already got a post-babel version in the app destination. iOS is comparing post-babel to app destination file and attempting to avoid modifying the file in the app destination if nothing changed.