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.

Sign up for GitHub

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

Jump to bottom

TiAPI: App launch slowed down by 2-5 seconds in SDK 8 #10640

Closed
wants to merge 1 commit into from
Closed

TiAPI: App launch slowed down by 2-5 seconds in SDK 8 #10640

wants to merge 1 commit into from

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Jan 22, 2019 •
edited

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

Description:

This is a performance fix to improve iOS require behavior to be more in line with Android's.

On Hans' app, this took startup from 5.5 seconds to load app.js down to ~400-500ms for me. For reference, Android takes roughly ~217ms.

The general theme here is to:

  • Make use of an index.json file to "cheat" so we know which JS/JSON files exist in the user's app. (which is what we do on Android)
  • Additionally mark the files as encrypted or not by assigning different values to them in the index.json (thereby eliminating the dumb behavior on iOS of trying encrypted first for everything and falling back to disk, which alone resulted in a halving of the times in my last commit introducing it)
  • Rewriting the require process to separate resolution of the path to a file from actually loading the module. This way we can resolve everything and keep a much better cache around with the resolved values.

@sgtcoolguy
Copy link
Contributor Author

cc @hansemannn

@build build added this to the 8.0.0 milestone Jan 22, 2019
@build build requested a review from a team January 22, 2019 19:47
@build
Copy link
Contributor

build commented Jan 22, 2019 •
edited

Fails
🚫 Tests have failed, see below for more information.
Warnings
⚠️

iphone/cli/commands/_build.js#L6406 - iphone/cli/commands/_build.js line 6406 – 'out' is defined but never used. (no-unused-vars)
Messages
📖

💾 Here's the generated SDK zipfile.
📖 ❌ 3 tests have failed There are 3 tests failing and 449 skipped out of 2990 total tests.

Tests:

Classname Name Time Error
android.Titanium.UI.TabGroup add Map.View to TabGroup 10.006 timeout of 10000ms exceeded
android.Titanium.UI.TabGroup.events close 5.486 timeout of 5000ms exceeded
android.Titanium.UI.View .backgroundImage (URL-redirect) 10.471 timeout of 10000ms exceeded

Generated by 🚫 dangerJS against d8bddd8

@sgtcoolguy
Copy link
Contributor Author

no tests as we already have a good suite of tests around require (which caught a bug during the process!)

Note to reviewer(s)/QE: I'd prefer to merge this myself so I can squash it and get the message/body correct.

@keerthi1032
Copy link
Contributor

@sgtcoolguy im getting the following error with this PR build and app stuck at the splash screen
steps:
1.created default classic app
2.run against iOS device with local SDK 8.0.0.v20190123072053

DEBUG] : Loading: /var/containers/Bundle/Application/665A2CF9-ED9D-472F-86B4-80476F7B74E3/K2test.app/ti.main.js, Resource: ti_main_js
[DEBUG] : Loading: /var/containers/Bundle/Application/665A2CF9-ED9D-472F-86B4-80476F7B74E3/K2test.app/index.json, Resource: index_json
[ERROR] : Script Error Couldn't find module: ./ti.internal/extensions/Error for architecture: arm64
[DEBUG] : Application booted in 54.376960 ms

@janvennemann
Copy link
Contributor

Code in general looks good, however i'm also getting the above error on a device build. Simulator works fine though.

@sgtcoolguy
Copy link
Contributor Author

Looks like this is due to encrypted assets being renamed during the build. So the file ti.internal/extensions/Error.js is renamed to ti_internal/extensions/Error_js, and that's the entry we're stuffing in our require index.json file.
I'll have to modify the encryption stuff to retain the original name as well, since I can't trust doing a reverse replacement will work (_ to ., since file may actually have had underscores in its name originally).

@sgtcoolguy
perf(ios): improve performance of require function
d8bddd8 
Generate an index of js/json files (_index_.json) packaged into the app for faster existence checks/reading.
Generate index when running local dev xcode build using node script.

Fixes TIMOB-26742
@sgtcoolguy
Copy link
Contributor Author

ok, I rebase all the commits down to one with a proper commit message.

I included the fix for encrypted JS/JSON assets being required (tested on my local device). I also renamed the file we write to be _index_.json rather than index.json to help avoid clashes with possible file existing in the user app. Lastly, this file is also encrypted when we're encrypting JS/JSON assets.

hansemannn
hansemannn reviewed Jan 24, 2019
View reviewed changes
iphone/TitaniumKit/TitaniumKit/Sources/API/KrollBridge.m
@@ -31,6 +31,45 @@
//Defined private method inside TiBindingRunLoop.m (Perhaps to move to .c?)
void TiBindingRunLoopAnnounceStart(TiBindingRunLoop runLoop);

typedef NS_ENUM(NSInteger, FileStatus) {
DoesntExist,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only code style, but usually in ObjC, the enumeration cases are prefixed with the enumeration itself, e.g. FileStatusDoesntExist.

@keerthi1032
Copy link
Contributor

FR Passed.
Test Environment:
Operating System
Name = Mac OS X
Version = 10.13.6
Architecture = 64bit
Node.js
Node.js Version = 8.12.0
npm Version = 6.4.1
Titanium CLI
CLI Version = 5.1.1
Titanium SDK
SDK Version = local sdk 8.0.0.v20190124091001
Studio Version =5.1.2.201812191857
Device = iPhone xs max iOS 12
Simulator =iPhone 6s iOS 11

@sgtcoolguy
Copy link
Contributor Author

Manually merging...

@sgtcoolguy sgtcoolguy closed this Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hansemannn hansemannn hansemannn left review comments

Assignees
No one assigned
Labels
improvement ios no tests parity 👯‍♀️
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sgtcoolguy @build @keerthi1032 @janvennemann @hansemannn