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

fix(android): performance issue with deeply nested views as of 7.5.0 #11253

Merged
merged 2 commits into from Oct 3, 2019
Merged

fix(android): performance issue with deeply nested views as of 7.5.0 #11253

merged 2 commits into from Oct 3, 2019

Conversation

jquick-axway
Copy link
Contributor

@jquick-axway jquick-axway commented Oct 2, 2019 •
edited

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

Summary:

  • A re-layout of 15 or more nested views can cause major performance issues as of 7.5.0.
    • Caused by window inset handling needed by our "extendSafeArea" and "safeAreaPadding" features.
    • Inset propagation to child views (which in turn trigger re-layouts) gets exponentially worse the deeper it gets.
  • Resolved by storing last received insets and only dispatching insets to children if changed.

Performance Test:

  1. Build and run the code attached to TIMOB-27434 on Android.
  2. Tap-and-hold on the text within the TextField.
  3. Verify that copy/paste clipboard immediately appears. (Used to hang for a few seconds.)
  4. Rotate the app to landscape.
  5. Verify that the app does not hang for a few seconds when rotated.

Inset Test:
This verifies that insets are still correctly propagated to child views.

  1. Build and run the "ExtendSafeAreaDrawerTest.js" attached to TIMOB-26427 on Android.
  2. Verify that red title bar extends beneath the status bar and its title text is not overlapped by the status bar, as shown in TIMOB-26427 screenshot.
  3. Tap on the hamburger button in the top-left corner to open the drawer layout.
  4. Verify that drawer's blue title bar extends beneath status bar and its text is not overlapped by the status bar, as shown in TIMOB-26427 screenshot.
  5. Rotate the app to landscape and verify both the red and blue title bars text is not overlapped as shown in the screenshots.

@jquick-axway jquick-axway added this to the 8.3.0 milestone Oct 2, 2019
@build build requested a review from a team October 2, 2019 02:47
@build
Copy link
Contributor

build commented Oct 2, 2019 •
edited

Messages
📖

💾 Here's the generated SDK zipfile.
📖

✅ All tests are passing
Nice one! All 4120 tests are passing.
(There are 469 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 705e23c

@jquick-axway jquick-axway mentioned this pull request Oct 2, 2019
Merged
sgtcoolguy
sgtcoolguy previously requested changes Oct 2, 2019
View reviewed changes
android/titanium/src/java/org/appcelerator/titanium/view/TiCompositeLayout.java
Comment on lines +477 to +480
if (insets.equals(this.previousInsets)) {
return insets;
}
this.previousInsets = insets;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this also need an Android api level guard? Would accessing it on api level < 20 cause a crash or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need a guard inside the onApplyWindowInsets() method since it has argument type WindowInsets which is API Level 20 only. Attempting to call onApplyWindowInsets() on a lower API level would crash, meaning that the API Level guard needs to be done on the caller's side.

But good question. Thanks for asking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, I'll do a quick experiment with Google's @RequireApi(ApiLevel) Java annotation. Android Studio's built-in linting tool is supposed to trigger compiler errors/warnings when making unguarded calls to methods like this. When we switch over to gradle, this may help reduce crashes on older OS versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote up a ticket TIMOB-27437 to implement the @RequireApi() annotations in the future. We can't leverage it until we support the gradle linting tool and add the Google Support Annotation library.

@sgtcoolguy sgtcoolguy dismissed their stale review October 2, 2019 18:57

sounds like it's not an issue...

@lokeshchdhry
Copy link
Contributor

FR Passed.

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)
Emulator: Android 6.0

@lokeshchdhry lokeshchdhry merged commit 057dad3 into tidev:master Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgtcoolguy sgtcoolguy sgtcoolguy left review comments

Assignees
No one assigned
Labels
android bug in-qe-testing 🕵 no tests
Projects
None yet
Milestone
8.3.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jquick-axway @build @lokeshchdhry @sgtcoolguy