perf: improve incremental app build times #10805

janvennemann · 2019-03-25T22:18:28Z

JIRA: https://jira.appcelerator.org/browse/TIMOB-27043

This is a follow up for #10789 which further improves the build time on incremental builds.

Some timings i made with our mocha test suite (average of three runs each):

Scenario	Build time (master)	Build time (PR)
Clean build	24417 ms	22856 ms
Incremental, no changes	6779 ms	645 ms
Incremental, one file changed	7126 ms	955 ms

The clean build difference can be neglected i guess, that's probably just random due to other system usage. Incremental build time improvements however are huge!

I tried to change as little as possible and just moved the related code to a task that automatically does file change detection. The async stuff is now handled in promises and i refactored the existing code into the two methods processJsFile and transformAndCopy. The rest is just utility stuff for the task to load and save state and merging data from the previous run with incremental changes back into our builder.

This is currently iOS only but once we decide to go for this i'll update the Android build too.

janvennemann · 2019-03-25T22:21:14Z

iphone/cli/lib/process-js-task.js

+						// If the file didn't change from previous run, return early
+						const currentHash = this.generateHash(source);
+						const previousHash = this.data.contentHashes[from];
+						if (previousHash && previousHash === currentHash) {


This is basically where all the magic happens. If the source file content did not change BEFORE we run jsanalyze just skip the whole thing, re-use the existing file and load used Ti symbols from previous run.

We cannot skip jsanalyze. We need to AST parse everything so we can detect all Ti.* and Titanium.* symbols and enable the various APIs such as Ti.Media. Same goes for Android (and maybe Windows?).

I highly recommend testing using iOS device builds of KitchenSink. There's a lot of sensitive code here.

I store the symbols at the end so they can be re-used on the next run. If nothing in the source file changed i don't see why we would need to run the jsanalyze again. We can simply load the symbols from the last run and inject them back into the builder.

I tested this with our KitchenSink app using device builds and this works just fine. The symbols for unchanged files will be re-used from the last build and changed files will run jsanalyze again and have their Ti symbols updated. If a file gets removed then it's stored Ti symbols also get cleared.

So I think the concern here is that we need to take into account more than just the original source file changing. Did any options baked into jsanalyze change? minify, transpile, etc. Basically anything that goes into analyzeOptions. Additionally, and this is implicit and not properly handled as-is, but the user could have a .babelrc/config that will get loaded any modify babel usage. I think @brentonhouse is doing this. We haven't officially supported/fixed handling that yet, but it's an expected use case. (right now I think for example we bomb out if a babel config ignores a file because transform returns null and we expect an object with a code property).

Second, is obviously we are generating both a transformed file and gathering Ti API usage. You need to handle "storing" both of those and handling merging the Ti API usage symbols stuff back into the build properly.

The jsanalyze options are a good point. I can check those before the task starts and fallback to a full run if anything changed. I can also feed in possible .babelrc/config paths and force a full task run if any of those changed. So that's also not a problem once that is officially supported.

Regarding your second point, I'm doing exactly that. The TI API usage symbols are stored per file in the task data that get's written to disk and will be loaded and merged back into the builder on subsequent runs. The transformed file is preserved between builds anyway so it's also kind of "stored".

build · 2019-03-25T22:41:24Z

	Warnings
⚠️	🔍 Can't find junit reports at `./junit.*.xml`, skipping generating JUnit Report.
⚠️	cli/lib/tasks/process-js-task.js#L156 - cli/lib/tasks/process-js-task.js line 156 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)
⚠️	iphone/cli/commands/_build.js#L6347 - iphone/cli/commands/_build.js line 6347 – 'out' is defined but never used. (no-unused-vars)

New dependencies added: p-limit.

p-limit

Author: Sindre Sorhus

Description: Run multiple promise-returning & async functions with limited concurrency

Homepage: https://github.com/sindresorhus/p-limit#readme


Created	over 2 years ago
Last Updated	about 2 months ago
License	MIT
Maintainers	1
Releases	7
Direct Dependencies	`p-try`
Keywords	promise, limit, limited, concurrency, throttle, throat, rate, batch, ratelimit, task, queue, async, await, promises and bluebird

README

p-limit

Run multiple promise-returning & async functions with limited concurrency

Install

$ npm install p-limit

Usage

const pLimit = require('p-limit');

const limit = pLimit(1);

const input = [
	limit(() => fetchSomething('foo')),
	limit(() => fetchSomething('bar')),
	limit(() => doSomething())
];

(async () => {
	// Only one promise is run at once
	const result = await Promise.all(input);
	console.log(result);
})();

API

pLimit(concurrency)

Returns a limit function.

concurrency

Type: number

Minimum: 1

Concurrency limit.

limit(fn, ...args)

Returns the promise returned by calling fn(...args).

fn

Type: Function

Promise-returning/async function.

args

Any arguments to pass through to fn.

Support for passing arguments on to the fn is provided in order to be able to avoid creating unnecessary closures. You probably don't need this optimization unless you're pushing a lot of functions.

limit.activeCount

The number of promises that are currently running.

limit.pendingCount

The number of promises that are waiting to run (i.e. their internal fn was not called yet).

FAQ

How is this different from the `p-queue` package?

This package is only about limiting the number of concurrent executions, while p-queue is a fully featured queue implementation with lots of different options, introspection, and ability to pause and clear the queue.

License

MIT © Sindre Sorhus

Generated by 🚫 dangerJS against b330082

janvennemann · 2019-03-27T14:34:13Z

Changes to jsanalyze options will now also be detected and a fallback to processing all JS files is done.

iphone/cli/lib/process-js-task.js

sgtcoolguy · 2019-03-27T22:30:34Z

iphone/cli/lib/process-js-task.js

+
+		return new Promise((resolve) => {
+			try {
+				this.builder.cli.createHook('build.ios.copyResource', this.builder, (from, to, cb) => {


I'd like to break these nesting levels up and try to minimize the mismatches here between Promises and callback functions if we can. Basically extract these callback functions out to their own async methods (using async fs APIs), then have just one method where you interface between the Promises and callbacks (maybe just using util.callbackify/util.promisify?)

iphone/cli/lib/process-js-task.js

sgtcoolguy · 2019-03-27T22:38:32Z

Forgot to mention: I don't know how to avoid it, but I'm not a huge fan of having to pass in the builder object here and rely on all sorts of properties/values internal to it. In the past I ran into this as well where we pass this massive object holding all the state and it makes it near impossible to test if you want to just hand craft a simple mock object yourself.

build · 2019-04-30T16:41:34Z

cli/lib/tasks/process-js-task.js

+
+			this.transformAndCopy(source, from, to).then(() => {
+				this.data.contentHashes[from] = currentHash;
+				return done();


⚠️ cli/lib/tasks/process-js-task.js line 149 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)

build · 2019-04-30T20:53:57Z

cli/lib/tasks/process-js-task.js

+					this.logger.error(e.codeFrame);
+				}
+
+				done(e);


⚠️ cli/lib/tasks/process-js-task.js line 156 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)

build · 2019-04-30T20:53:58Z

	Warnings
⚠️	iphone/cli/commands/_build.js#L6402 - iphone/cli/commands/build.js line 6402 – 'code' is defined but never used. Allowed unused args must match /^.+/. (no-unused-vars)
⚠️	iphone/cli/commands/_build.js#L6402 - iphone/cli/commands/build.js line 6402 – 'out' is defined but never used. Allowed unused args must match /^.+/. (no-unused-vars)

	Messages
📖	✅ All tests are passing Nice one! All 1863 tests are passing. (There are 225 tests skipped)

New dependencies added: p-limit.

p-limit

Author: Sindre Sorhus

Description: Run multiple promise-returning & async functions with limited concurrency

Homepage: https://github.com/sindresorhus/p-limit#readme


Created	over 2 years ago
Last Updated	3 months ago
License	MIT
Maintainers	1
Releases	7
Direct Dependencies	`p-try`
Keywords	promise, limit, limited, concurrency, throttle, throat, rate, batch, ratelimit, task, queue, async, await, promises and bluebird

README

p-limit

Run multiple promise-returning & async functions with limited concurrency

Install

$ npm install p-limit

Usage

const pLimit = require('p-limit');

const limit = pLimit(1);

const input = [
	limit(() => fetchSomething('foo')),
	limit(() => fetchSomething('bar')),
	limit(() => doSomething())
];

(async () => {
	// Only one promise is run at once
	const result = await Promise.all(input);
	console.log(result);
})();

API

pLimit(concurrency)

Returns a limit function.

concurrency

Type: number

Minimum: 1

Concurrency limit.

limit(fn, ...args)

Returns the promise returned by calling fn(...args).

fn

Type: Function

Promise-returning/async function.

args

Any arguments to pass through to fn.

Support for passing arguments on to the fn is provided in order to be able to avoid creating unnecessary closures. You probably don't need this optimization unless you're pushing a lot of functions.

limit.activeCount

The number of promises that are currently running.

limit.pendingCount

The number of promises that are waiting to run (i.e. their internal fn was not called yet).

FAQ

How is this different from the `p-queue` package?

This package is only about limiting the number of concurrent executions, while p-queue is a fully featured queue implementation with lots of different options, introspection, and ability to pause and clear the queue.

License

MIT © Sindre Sorhus

Generated by 🚫 dangerJS against 03297fe

lokeshchdhry · 2019-05-28T21:54:44Z

I am seeing the following timings building for KS V2:

8.1.0 (no fix):
Studio:

Clean build - 27 sec.
Incremental(no change) - 7s 180ms.
Incremental(Change in 1 file) - 6s 188ms.

CLI:

Clean build - 23s 321ms.
Incremental(no change) - 7s 136ms.
Incremental(Change in 1 file) - 6s 157ms.

This PR:
Studio:

Clean build - 24s 328ms.
Incremental(no change)- 3s 661ms.
Incremental(Change in 1 file) - 4s 511ms.

CLI:

Clean build - 21s 773ms.
Incremental(no change) - 2s 736ms.
Incremental(Change in 1 file) - 5s 556ms.

@janvennemann , I definitely see an improvement in the timings but do not see a vast improvement as you see in incremental builds.

Studio Ver: 5.1.2.201903111843
OS Ver: 10.14.5
Xcode Ver: Xcode 10.2.1
Appc NPM: 4.2.13
Appc CLI: 7.0.12-70X.4
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.10
Node Ver: 8.15.1
NPM Ver: 6.4.1
Java Ver: 10.0.2
IOS Simulator: ⇨ iPhone 7 Plus (iOS 12.2)

lokeshchdhry · 2019-05-29T21:05:08Z

FR Passed.

hansemannn · 2019-05-30T11:00:49Z

@lokeshchdhry Most of the benefits of this PR can be seen on larger projects. We've cut our build times to at least 1/3 of the original build time.

whitfin · 2019-05-30T17:05:57Z

I was actually working on some of these changes locally in the last few days because it's definitely causing slowness.

On my local project (which is small, but has a fairly large amount of node modules) it reduces my build time from ~200s to ~12s. Definitely a good improvement!

lokeshchdhry · 2019-05-30T17:44:10Z

@janvennemann , Can you please resolve the conflicts. Thanks.

whitfin · 2019-05-31T22:29:55Z

cli/lib/tasks/process-js-task.js

+
+const i18n = appc.i18n(__dirname);
+const __ = i18n.__;
+const limit = pLimit(8);


Why we need to limit the promises at all? Are we not slowing things down artificially? Or is there some reason we need to do this? If so, why 8 as a limit?

hansemannn · 2019-06-04T14:19:02Z

Can someone resolve the merge conflicts and merge this? We need to cherry pick this since months now and it's a pain.

hansemannn · 2019-06-04T18:00:14Z

@sgtcoolguy The merge conflicts come from your env-var changes. I think @janvennemann is on holiday, so can you resolve those? I currently needed to fix the merge problems locally but reverted the env-changes there.

…_mobile into janvennemann-build-perf

build · 2019-06-04T21:11:48Z

iphone/cli/commands/_build.js

+						.then(() => {
+							this.tiSymbols = task.data.tiSymbols;
+
+							return next();


⚠️ iphone/cli/commands/_build.js line 5875 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)

whitfin · 2019-06-04T21:40:43Z

I resolved the merge conflicts and this should be good to merge. I'm still curious about why we need p-limit and why we need a limit on the Promise throughput at all, but I can save that question for another day.

cb1kenobi · 2019-06-04T22:13:38Z

Packages like p-limit and promise-limit are essentially the same thing as async.eachLimit() where you want to limit the number of concurrent tasks. If you do a Promise.all() on many, many things, you could exhaust the memory and crash.

I've always put a limit when synchronizing an open-ended number of tasks.

I've never done any real world benchmarks, so I can't say what the upper limit is, but it probably depends on the task workload. If you're copying files, 20 may seem low, but 20k, sure seems high. I'd be interested to see some data on it.

whitfin · 2019-06-04T22:46:16Z

@cb1kenobi sure, but 8 seems pretty low - I agree that tuning it would be nicer (in future). Given that most of the operations are just waiting on IO, there shouldn't be much impact of waiting on a fair bit more than that.

improve incremental app build times on iOS and Android * move to async/await * let errors bubble up to builder Fixes TIMOB-27043

perf: improve incremental app build times

ea0128c

janvennemann added ios improvement labels Mar 25, 2019

janvennemann added this to the 8.1.0 milestone Mar 25, 2019

janvennemann requested a review from sgtcoolguy March 25, 2019 22:18

janvennemann commented Mar 25, 2019

View reviewed changes

build added the needs jira 🚨 label Mar 25, 2019

janvennemann added 2 commits March 26, 2019 12:12

fix syntax error and typos

500f7b5

detect changes in jsanalyze options

45903a6

sgtcoolguy suggested changes Mar 27, 2019

View reviewed changes

janvennemann added the work in progress 🚧 label Apr 12, 2019

janvennemann added 4 commits April 26, 2019 14:31

move to async/await

7734fc0

let errors bubble up to builder

a9bc8b4

handle deleted files sync

2f8175a

add missing async keyword

f28737b

janvennemann mentioned this pull request Apr 29, 2019

fix(android): fix copying of resources #10702

Merged

janvennemann added 2 commits April 30, 2019 17:16

re-use task in the android build

08de229

fix linting issues

b330082

build removed the needs jira 🚨 label Apr 30, 2019

build reviewed Apr 30, 2019

View reviewed changes

janvennemann added 3 commits April 30, 2019 20:59

Merge branch 'master' into build-perf

3093860

fix merge error

f3c4304

fix remaining merge errors

bef8533

build reviewed Apr 30, 2019

View reviewed changes

janvennemann marked this pull request as ready for review April 30, 2019 20:54

janvennemann removed the work in progress 🚧 label Apr 30, 2019

janvennemann requested a review from sgtcoolguy May 21, 2019 17:58

sgtcoolguy approved these changes May 23, 2019

View reviewed changes

sgtcoolguy added the in-qe-testing 🕵 label May 23, 2019

sgtcoolguy and others added 2 commits May 23, 2019 11:46

Merge branch 'master' into build-perf

6617f6c

Merge branch 'master' into build-perf

38477e1

lokeshchdhry added 2 commits May 29, 2019 09:23

Merge branch 'master' into build-perf

9e08296

Merge branch 'master' into build-perf

54d5939

build added the merge conflicts 🚨 label May 30, 2019

whitfin reviewed May 31, 2019

View reviewed changes

sgtcoolguy modified the milestones: 8.1.0, 8.2.0 Jun 3, 2019

whitfin added 2 commits June 4, 2019 13:47

Merge branch 'build-perf' of https://github.com/janvennemann/titanium…

0bd15aa

…_mobile into janvennemann-build-perf

Merge branch 'master' into build-perf

03297fe

build removed the merge conflicts 🚨 label Jun 4, 2019

build reviewed Jun 4, 2019

View reviewed changes

sgtcoolguy merged commit af51700 into tidev:master Jun 5, 2019

sgtcoolguy pushed a commit that referenced this pull request Jun 5, 2019

refactor(build): improve incremental app build times (#10805)

93b9f1c

improve incremental app build times on iOS and Android * move to async/await * let errors bubble up to builder Fixes TIMOB-27043

garymathews mentioned this pull request Jun 5, 2019

fix(android): unmark AssetCryptImpl from build directory #10944

Closed

hansemannn mentioned this pull request Jun 10, 2019

feat(process): load ENV variables into process.env #10895

Merged

perf: improve incremental app build times #10805

perf: improve incremental app build times #10805

Conversation

janvennemann commented Mar 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgtcoolguy Mar 26, 2019 • edited

Choose a reason for hiding this comment

janvennemann Mar 27, 2019 • edited

Choose a reason for hiding this comment

build commented Mar 25, 2019 • edited

p-limit

Install

Usage

API

pLimit(concurrency)

concurrency

limit(fn, ...args)

fn

args

limit.activeCount

limit.pendingCount

FAQ

How is this different from the p-queue package?

Related

License

janvennemann commented Mar 27, 2019

Choose a reason for hiding this comment

sgtcoolguy commented Mar 27, 2019

build Apr 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

build commented Apr 30, 2019 • edited

p-limit

Install

Usage

API

pLimit(concurrency)

concurrency

limit(fn, ...args)

fn

args

limit.activeCount

limit.pendingCount

FAQ

How is this different from the p-queue package?

Related

License

lokeshchdhry commented May 28, 2019 • edited

lokeshchdhry commented May 29, 2019

hansemannn commented May 30, 2019

whitfin commented May 30, 2019

lokeshchdhry commented May 30, 2019

Choose a reason for hiding this comment

hansemannn commented Jun 4, 2019

hansemannn commented Jun 4, 2019

Choose a reason for hiding this comment

whitfin commented Jun 4, 2019

cb1kenobi commented Jun 4, 2019

whitfin commented Jun 4, 2019

janvennemann commented Mar 25, 2019 •

edited

sgtcoolguy Mar 26, 2019 •

edited

janvennemann Mar 27, 2019 •

edited

build commented Mar 25, 2019 •

edited

How is this different from the `p-queue` package?

build Apr 30, 2019 •

edited

build commented Apr 30, 2019 •

edited

How is this different from the `p-queue` package?

lokeshchdhry commented May 28, 2019 •

edited