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

MM-34585 Use removeClippedSubviews from Lists to improve scroll perf #5199

Merged
merged 5 commits into from
Apr 8, 2021

Conversation

enahum
Copy link
Contributor

@enahum enahum commented Mar 1, 2021

Summary

A few users have reported that in some android devices (Samsung with OneUI 3+) the app runs at low fps, this seems to be related with screens that are capable to have a higher refresh rate. Following the RN docs we are now using removeClippedSubviews to help improve the scrolling perf.

Ticket Link

Resolves: #5046
https://mattermost.atlassian.net/browse/MM-34585

@enahum enahum added 2: Dev Review Requires review by a core commiter CherryPick/Candidate A candidate for a quality or patch release, but not yet approved 3: QA Review Requires review by a QA tester labels Mar 1, 2021
@enahum enahum added this to the v1.41.0 milestone Mar 1, 2021
@enahum enahum added the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 1, 2021
@mattermod mattermod removed the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 1, 2021
@mattermod
Copy link
Contributor

Building app in separate branch.

@mattermod
Copy link
Contributor

Float refreshRate = (Float)stringObjectHashMap.getOrDefault("refreshRate", 0f);
if (!seen || Math.round(refreshRate) == 60) {
seen = true;
best = stringObjectHashMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just break out of the for loop after setting best.

app/utils/images.js Show resolved Hide resolved
@@ -550,14 +550,14 @@ workflows:
- test
filters:
branches:
only: /^build-pr-.*/
only: /^(build|android)-pr-.*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this PR be reverted at some point? If so, consider having these changes in a separate PR?

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 would like to keep this, that way if a PR fails for a particular platform we can just build the platform that we need

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yea, I meant that this specific change can be in a separate PR if you foresee the FPS changes being reverted in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe ;)

Copy link
Contributor

@josephbaylon josephbaylon left a comment

Choose a reason for hiding this comment

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

LGTM. I don't have similar devices as found in the original issue and I can't find a hardware profile online to emulate. I just did a smoke test on a Pixel 3 XL (Android 11) devices and I don't experience sluggishness. Navigation, interaction with components, posting messages, switching channels and teams, searching, etc all seem to be functioning well.

Let me know if there's specific areas I need to test more.

@josephbaylon josephbaylon removed the 3: QA Review Requires review by a QA tester label Mar 2, 2021
@enahum
Copy link
Contributor Author

enahum commented Mar 2, 2021

@josephbaylon people over here #5046 can share more data points

@josephbaylon josephbaylon self-requested a review March 2, 2021 16:42
@josephbaylon josephbaylon added the 3: QA Review Requires review by a QA tester label Mar 2, 2021
@shaz-r
Copy link
Contributor

shaz-r commented Mar 3, 2021

I've got a Note 20 that I'm experimenting with - I don't think this PR has fixed the issue; unsure of what the cause is.

@enahum
Copy link
Contributor Author

enahum commented Mar 3, 2021

@SHAZM you and I should work together on this one, perhaps friday or over the weekend

@amyblais amyblais removed this from the v1.41.0 milestone Mar 8, 2021
@josephbaylon josephbaylon requested review from josephbaylon and removed request for josephbaylon March 29, 2021 23:00
@enahum enahum changed the title Force Android to run the app at 60 fps Use removeClippedSubviews from Lists to improve scroll perf Mar 31, 2021
@enahum enahum requested review from svelle and migbot March 31, 2021 22:57
@enahum enahum added this to the v1.42.0 milestone Mar 31, 2021
@enahum enahum added CherryPick/Approved Meant for the quality or patch release tracked in the milestone and removed CherryPick/Candidate A candidate for a quality or patch release, but not yet approved labels Mar 31, 2021
@enahum enahum added the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 31, 2021
@mattermod
Copy link
Contributor

Building app in separate branch.

@mattermod mattermod removed the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 31, 2021
@mattermod
Copy link
Contributor

Copy link
Contributor

@josephbaylon josephbaylon left a comment

Choose a reason for hiding this comment

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

I retested on a Pixel 3 XL (Android 11) device and I'm not experiencing sluggishness. Scrolling, navigation, interaction with components, posting messages, switching channels and teams, searching, etc all seem to be functioning well.

@SHAZM has the device where he can repro the original issue. I'll wait for his testing before I approve the PR.

@josephbaylon josephbaylon changed the title Use removeClippedSubviews from Lists to improve scroll perf MM-34585 Use removeClippedSubviews from Lists to improve scroll perf Apr 2, 2021
Copy link
Member

@svelle svelle left a comment

Choose a reason for hiding this comment

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

Tested on my Pixel 5. Showed some improvements. Performance graph showed less extreme spikes when movement was happening on the screen. Still room for improvement but a good start.

@enahum enahum removed the 3: QA Review Requires review by a QA tester label Apr 6, 2021
@enahum enahum removed the request for review from shaz-r April 8, 2021 20:51
@enahum enahum added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Apr 8, 2021
@enahum
Copy link
Contributor Author

enahum commented Apr 8, 2021

Spoke with @SHAZM this is OK to merge

@enahum enahum merged commit 9c043f1 into master Apr 8, 2021
@enahum enahum deleted the fps branch April 8, 2021 20:52
@mattermod
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit to mattermost-build/mattermost-mobile that referenced this pull request Apr 8, 2021
@mattermod mattermod added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Apr 8, 2021
enahum added a commit that referenced this pull request Apr 10, 2021
…5199) (#5298)

(cherry picked from commit 9c043f1)

Co-authored-by: Elias Nahum <nahumhbl@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App is low on fps on new upgrade to android 11
7 participants