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

build(android): include common bundle in snapshot #11054

Merged
merged 9 commits into from Sep 26, 2019

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Jul 16, 2019

  • Include common bundle in V8 snapshot for improved launch times
Device 8.0.2.GA 8.3.0 8.3.0 + PR
Pixel 3a 1133ms 387ms (reduced by 65%) 303ms (reduced by 73%)
Nexus 4 4723ms 3378ms (reduced by 28%) 1666ms (reduced by 65%)

JIRA Ticket

@build
Copy link
Contributor

build commented Jul 17, 2019

Warnings
⚠️

android/cli/commands/_build.js#L3313 - android/cli/commands/_build.js line 3313 – 'url.parse' was deprecated since v11.0.0. Use 'url.URL' constructor instead. (node/no-deprecated-api)

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3899 tests are passing.
(There are 475 skipped tests not included in that total)

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Generated by 🚫 dangerJS against b5c2e4f

build/lib/android/snapshot.js Outdated Show resolved Hide resolved
build/lib/builder.js Outdated Show resolved Hide resolved
build/lib/android/index.js Outdated Show resolved Hide resolved
build/lib/packager.js Outdated Show resolved Hide resolved
@hansemannn
Copy link
Collaborator

How does this affect build times? Tweaking performance is probably only relevant for production builds, but still interesting to test.

@jquick-axway
Copy link
Contributor

How does this affect build times?

We don't currently create snapshots for appc run built apps. We create the snapshots when doing a "titanium_mobile" SDK build. Partly because we don't have Windows versions of the V8 command lines tools needed to create the snapshots.

What we're currently doing for 8.1.0 is creating an empty snapshot. This has made V8 "cold startup" time about 2-3x faster. It's pretty significant. You've likely noticed it.

With Gary's change, we're taking a snapshot of our rolled-up common "ti.main.js" code, which has gotten heftier since we're babel transpile/polyfilling it now. This improves the cold startup time and relaunch time. Gary's other PR #11058 further improves the startup time as well.

Note that the newest Android OS versions have a much more aggressive battery optimizers, causing apps to be force quit more often while backgrounded. This makes improving our app cold startup time a high priority.

@@ -133,6 +134,8 @@ public static void updateActivityTransitionState(boolean state)
}
}

public static long START_TIME_MS = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: I'm putting this here so we can easily access the start time elsewhere in our SDK.

@garymathews
Copy link
Contributor Author

@sgtcoolguy @jquick-axway Updated PR

@sgtcoolguy sgtcoolguy self-requested a review July 26, 2019 19:09
Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

I think this general approach can be useful in that each platform would get only the set of polyfills/shims it needed. I'd tweak the SDK build logic to not have to produce the ios/android data and then just wipe it out of the object if it's a different platform.

Also, this presumably means we'd need to tweak the windows CLI code to point at common/Resources/windows like you do for Android/IOS here (Unless it's just all smart enough to pick up the platform-specific subfolder magically)

build/lib/android/snapshot.js Outdated Show resolved Hide resolved
build/lib/builder.js Outdated Show resolved Hide resolved
build/lib/builder.js Outdated Show resolved Hide resolved
@sgtcoolguy
Copy link
Contributor

@garymathews Also, just a note that the work you've done on Android startup is awesome. Those timing numbers above are insane.

I think the community is in for a real surprise between our app build time improvements and then app startup improvements on Android in 8.1 - 8.3 timeframe!

@tidev tidev deleted a comment from build Jul 26, 2019
@tidev tidev deleted a comment from build Jul 26, 2019
@garymathews
Copy link
Contributor Author

@sgtcoolguy Thanks for the feedback, updated PR

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

LGTM

@garymathews garymathews changed the base branch from master to 8_3_X August 14, 2019 18:15
@garymathews garymathews changed the title build(android): include common bundle in snapshot build(android)(8_3_X): include common bundle in snapshot Aug 14, 2019
@sgtcoolguy sgtcoolguy changed the base branch from 8_3_X to master September 9, 2019 18:01
chore(android): log launch time

fix(android,ios): generate platform specific bundles

fix(android): amend launch time as debug statistic

fix(android): rename snapshot method

build: refactor babel transpilation options

fix(build): bump build script ecma version

fix(build): address lint warnings

fix(android): use SystemClock.uptimeMillis()

fix(android): validate snapshot header size

fix(android): remove unused import

fix(ios): use minIosVersion
@sgtcoolguy sgtcoolguy changed the title build(android)(8_3_X): include common bundle in snapshot build(android): include common bundle in snapshot Sep 9, 2019
@@ -28,6 +34,21 @@ function thisOS() {
return osName;
}

function determineBabelOptions(babelOptions) {
const options = {
...babelOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

  • ⚠️ build/lib/builder.js line 39 – Rest/spread properties are not supported until Node.js 8.3.0. The configured version range is '>=8'. (node/no-unsupported-features/es-syntax)

@lokeshchdhry
Copy link
Contributor

Building an app with artifact from this PR errors with:

[ERROR] :  TiExceptionHandler: (main) [21,21] ti:/module.js:302
[ERROR] :  TiExceptionHandler: 	throw new Error('Requested module not found: ' + request); // TODO Set 'code' property to 'MODULE_NOT_FOUND' to match Node?
[ERROR] :  TiExceptionHandler:  ^
[ERROR] :  TiExceptionHandler: Error: Requested module not found: ../../../../../../../../../../semantic.colors.json
[ERROR] :  TiExceptionHandler:     at Module.require (ti:/module.js:302:8)
[ERROR] :  TiExceptionHandler:     at require (ti:/module.js:566:15)
[ERROR] :  TiExceptionHandler:     at startSnapshot (<embedded>:7:39)
[ERROR] :  TiExceptionHandler:     at /ti.main.js:1:8
[ERROR] :  TiExceptionHandler:     at Module._runScript (ti:/module.js:591:17)
[ERROR] :  TiExceptionHandler:     at Module.load (ti:/module.js:107:7)
[ERROR] :  TiExceptionHandler:     at Function.Module.runModule (ti:/module.js:75:9)
[ERROR] :  TiExceptionHandler:
[ERROR] :  TiExceptionHandler:     org.appcelerator.kroll.runtime.v8.V8Runtime.nativeRunModule(Native Method)
[ERROR] :  TiExceptionHandler:     org.appcelerator.kroll.runtime.v8.V8Runtime.doRunModule(V8Runtime.java:162)
[ERROR] :  TiExceptionHandler:     org.appcelerator.kroll.KrollRuntime.runModule(KrollRuntime.java:211)
[ERROR] :  TiExceptionHandler:     org.appcelerator.titanium.TiLaunchActivity.loadScript(TiLaunchActivity.java:106)
[ERROR] :  TiExceptionHandler:     org.appcelerator.titanium.TiRootActivity.loadScript(TiRootActivity.java:480)
[ERROR] :  TiExceptionHandler:     org.appcelerator.titanium.TiLaunchActivity.onResume(TiLaunchActivity.java:194)
[ERROR] :  TiExceptionHandler:     org.appcelerator.titanium.TiRootActivity.onResume(TiRootActivity.java:499)
[ERROR] :  TiExceptionHandler:     android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1412)
[ERROR] :  TiExceptionHandler:     android.app.Activity.performResume(Activity.java:7292)
[ERROR] :  TiExceptionHandler:     android.app.ActivityThread.performResumeActivity(ActivityThread.java:3776)

This does not happen with master SDK from jenkins.

@jquick-axway
Copy link
Contributor

@garymathews,

The exception @lokeshchdhry ran into might be because we are doing a require() on the new semantic color JSON file instead of loading it via our file APIs (which is how we load our "bootstrap.json" file).
https://github.com/appcelerator/titanium_mobile/pull/11097/files#diff-e66c10e6fedb17f9d452e0cfd2d1ff83R45

@garymathews
Copy link
Contributor Author

@lokeshchdhry I've fixed the require in ti.ui.js. It was an unrelated issue to this PR, but a simple fix that I've included here.

@ewanharris
Copy link
Collaborator

@garymathews, that shouldn't have been needed, this works fine on master, it looks like the rollup configuration to ignore the semantic colors require when rollup does it stuff was just removed in this PR master, this PR

@jquick-axway
Copy link
Contributor

@lokeshchdhry, regarding that exception, I assume the app didn't crash. The exception was simply logged right?

I'm asking because I see a try/catch block around the require() JS function that was causing the exception. I want to make sure that safety mechanism still works.

@lokeshchdhry
Copy link
Contributor

@jquick-axway , yes the app does not crash but a runtime error is thrown. It says at the splash screen though.

@jquick-axway
Copy link
Contributor

That would suggest our try/catch handling isn't working here.

And yet the try/catch block we have in "ti.main.js" does work when attempting to require-in the ACA module that isn't there.

Hmm...

@garymathews
Copy link
Contributor Author

@jquick-axway Rollup pulled the require out of the try catch. But it's fine now, I've addressed the issue.

@lokeshchdhry
Copy link
Contributor

FR passed.

Checked it with default app & KS. Improvement in launch time is seen.

Studio Ver: 5.1.4.201909061933
SDK Ver: 8.3.0 local build
OS Ver: 10.14.5
Xcode Ver: Xcode 11.0
Appc NPM: 4.2.15
Appc CLI: 7.1.1
Daemon Ver: 1.1.3
Ti CLI Ver: 5.2.1
Alloy Ver: 1.14.1
Node Ver: 10.16.1
NPM Ver: 6.9.0
Java Ver: 10.0.2
Android Devices: ⇨ google Pixel (Android 9), Android 4.4, android 6.0

@lokeshchdhry lokeshchdhry merged commit 1c07f0a into tidev:master Sep 26, 2019
@hansemannn
Copy link
Collaborator

hansemannn commented Oct 3, 2019

We just tried this change and it didn't really improve launch times (~3s) compared to before (~3.5s), tested on a Mi A2. You can test our app internally to reproduce the issue. Possible performance reasons:

  • We have an own our_app.bootstrap.js file that loads things like Firebase, which might cause issues
  • We have some polyfills in that file that might cause issues

@jquick-axway
Copy link
Contributor

Well if it reduced your launch time by 0.5 seconds, then I would say that is still an improvement. Every bit counts. And there are a couple other pending PRs that will improve launch times as well. One removes our usage of the Java getResourcesAsStream() and the other cuts down on the needless UTF-8/UTF-16 transcoding we were doing when requiring-in scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants