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

Stats Period Details: Optimize table view updates by integrating diffable data source #22823

Conversation

staskus
Copy link
Contributor

@staskus staskus commented Mar 13, 2024

Fixes #22720
Related PRs:

Description

Introduce diffable data source into Stats Details (Traffic -> Details, Days/../Years -> Details) views. I only found 2 ViewControllers used for Stats Details:

  • PostStatsTableViewController to show Stats for a Post
  • SiteStatsDetailTableViewController for all the other Details views

Solution

  1. Change ImmuTableViewHandler to ImmuTableDiffableViewHandler
  2. Call diffableDataSource.apply instead of setting tableViewModel
  3. Build tableViewSnapshot instead of tableViewModel
  4. Make required models Hashable
  5. Added helper methods to handle a common case of building a single-section table view snapshot

To test

Test with both Stats Traffic feature flag enabled and disabled:

  1. Select a high traffic site
  2. Open Traffic or Days/../Years tab
  3. Go through each of the cards and tap "View More"
  4. For Post & Pages cards click on a specific post
  5. Confirm that views open and scroll without any crashes or glitches

Regression Notes

  1. Potential unintended areas of impact

Breaking Stats Details views

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

Manual testing

  1. What automated tests I added (or what prevented me from doing so)

None

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@staskus staskus added this to the 24.5 milestone Mar 13, 2024
@staskus staskus requested a review from guarani March 13, 2024 10:08
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 13, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr22823-47988e5
Version24.6
Bundle IDorg.wordpress.alpha
Commit47988e5
App Center BuildWPiOS - One-Offs #9418
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 13, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr22823-47988e5
Version24.6
Bundle IDcom.jetpack.alpha
Commit47988e5
App Center Buildjetpack-installable-builds #8461
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@staskus
Copy link
Contributor Author

staskus commented Mar 19, 2024

TODO: Requires the same fix #22857

Edit: Done ✅

… source apply methods

Usage of performBatchUpdates is not allowed if table view depends on a diffable data source and can result in crashes
@guarani
Copy link
Contributor

guarani commented Apr 5, 2024

When I run this on my phone, Stats feels sluggish. I haven't checked trunk yet, but this is what I see when testing this branch:

  • App freezes for 4-5 seconds when tapping back on the details screens (e.g. Posts and Pages
  • Slow scrolling on the main Day screen as well as on the detail screens

It's hard to tell if it's due to the debug build or an actual issue with the app. I'm leaving this as a partial review to come back to.

@staskus
Copy link
Contributor Author

staskus commented Apr 8, 2024

When I run this on my phone, Stats feels sluggish.

Thanks for testing!

Yes, we should be careful with performance, especially given that the goal if this task was to optimize Stats. I'll check later myself to see which operations could be making Stats sluggish and maybe there're some flaws in this implementation.

@staskus
Copy link
Contributor Author

staskus commented Apr 8, 2024

@guarani

When I run this on my phone, Stats feels sluggish. I haven't checked trunk yet, but this is what I see when testing this branch:
App freezes for 4-5 seconds when tapping back on the details screens (e.g. Posts and Pages
Slow scrolling on the main Day screen as well as on the detail screens

I can reproduce the same thing when the iPhone is connected to a debugger. Both on the trunk and on task/22720-stats-details-optimize-table-view-updates-by-integrating-diffable-data-source branch. So it doesn't look to be related to these changes.

  • Back action is delayed by WordPressComLanguageDatabase.init() method. Specifically, the call implemented in WordPressKit (wordpress-mobile/WordPressKit-iOS@b942929) to get device language slug. It's called once for each of the Stats endpoint:
image

Without a connected debugger, WordPressComLanguageDatabase.init() calls have a smaller footprint which doesn't cause a hang although it still takes a visible footprint of resources:

image

…w-updates-by-integrating-diffable-data-source
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Thanks for checking @staskus!

I re-tested with the feature flag on and off by going through all the Traffic detail screens and found no issues. It's not very easy to see the difference this PR makes because of the other performance issues that are unrelated to this PR. Even after disconnecting the debugger, the back button is very slow at times (~5 seconds to respond), but it might also have to do with it being a debug build.

I think this is good to merge because the diffable data source is the way forward 👍

@staskus staskus modified the milestones: Pending, 24.7 Apr 14, 2024
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is assigned to the milestone 24.7. This milestone is due in less than 4 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@staskus staskus merged commit 199d402 into trunk Apr 14, 2024
25 of 26 checks passed
@staskus staskus deleted the task/22720-stats-details-optimize-table-view-updates-by-integrating-diffable-data-source branch April 14, 2024 16:49
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.

Stats Details: Optimize table view updates by integrating diffable data source
4 participants