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 Traffic: Optimize table view updates by integrating diffable data source #22542

Merged
merged 14 commits into from
Feb 22, 2024

Conversation

staskus
Copy link
Contributor

@staskus staskus commented Feb 5, 2024

Description

Each time we update Stats data, we build a new tableViewModel() and reload all table view cells. It happens for each type of update we receive, which results in almost a dozen refreshes of table view cells happening in a short time. It can cause flickering of cell UI, or contribute to lag when scrolling.

Solution

Implemented native UITableViewDiffableDataSource for Traffic tab which would only update table view cells if data changes. In a gist, we only need to make cell models Hashable, and the system will take care of efficient updates:

  1. Defined Snapshot and DataSource types which depend on StatsTrafficSection and StatsHashableImmuTableRow to integrate within existing ImmuTable
  2. We have a different section for each PeriodType, so StatsTrafficSection only depends on it
  3. We already define view models ImmuTableRow for cells, so StatsHashableImmuTableRow only depends on a Hashable version of ImmuTableRow
  4. The diffable data source relies on both the identity and the equality of the items it manages. The identity is determined by the item's hash, and equality is determined by whether the item's content has changed. Row's identity depends on StatSection and on RowType. For example, periodAuthors section and StatsGhostTwoColumnImmutableRow row type.
  5. Made all the rows we use within Traffic tab to conform to Hashable
  6. Once it's done, we build Snapshot the same way we previously built ImmuTable view model. No changes in logic are necessary and we can follow a similar pattern
  7. Made updates to SiteStatsPeriodTableViewController and ImmuTable to use a new DiffableDataSource and applySnapshot instead of calling tableView.reloadData

Now any cell is only reloaded when the data of this particular cell changes.

If these changes prove successful, I plan to apply the same changes to the Insights tab to avoid similar issues such as flickering their as well.

Video

Before

before.MP4

After (together with #22592)

after.MP4

Regression Notes

  1. Potential unintended areas of impact

Days/Weeks/Months/Years tabs remain unaffected since they use SiteStatsPeriodTableViewControllerDeprecated.

The only unintended effect is implementing Hashable incorrectly. We want to make sure we identify each different period row uniquely but also detect when its content changes.

  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)

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.

UI Changes testing checklist:

  • 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)

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 5, 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 Numberpr22542-d130c3a
Version24.3
Bundle IDorg.wordpress.alpha
Commitd130c3a
App Center BuildWPiOS - One-Offs #8907
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 Feb 5, 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 Numberpr22542-d130c3a
Version24.3
Bundle IDcom.jetpack.alpha
Commitd130c3a
App Center Buildjetpack-installable-builds #7937
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@staskus staskus marked this pull request as draft February 5, 2024 15:28
… place

The diffable data source relies on both the identity and the equality of the items it manages. The identity is determined by the item's hash, and equality is determined by whether the item's content has changed. If the content of an item is considered to have changed (even if its hash hasn't), the diffable data source may decide to reload that item
Use default hash(into...) implementation that calculates identity based on StatSection and RowType
Build StatsTrafficSnapshot instead of ImmuTable to be used in VC without changing any logic
- Create StatsTrafficDataSource
- Apply snapshot instead of reloading table view
@staskus staskus force-pushed the task/stats-traffic-diffable-optimization branch from cd0ed06 to 5949c5e Compare February 12, 2024 13:44
@staskus staskus changed the title [WIP] Stats Traffic: Optimize table view updates by integrating diffable data source Stats Traffic: Optimize table view updates by integrating diffable data source Feb 12, 2024
@staskus staskus marked this pull request as ready for review February 12, 2024 14:00
@staskus staskus requested a review from guarani February 12, 2024 14:00
Make DiffableDataSource easier to integrate for existing ImmuTableVieHandler users Define common Snapshot and DataSource. Override delegate methods that rely on fetching row items.
@staskus staskus force-pushed the task/stats-traffic-diffable-optimization branch from 5949c5e to 0b8d77a Compare February 13, 2024 08:52
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@staskus staskus modified the milestones: Pending, 24.3 ❄️, 24.4 Feb 19, 2024
@staskus staskus marked this pull request as draft February 21, 2024 08:53
@staskus staskus marked this pull request as ready for review February 21, 2024 13:24
@staskus staskus force-pushed the task/stats-traffic-diffable-optimization branch from 56e854f to d5aae87 Compare February 21, 2024 13:25
@staskus
Copy link
Contributor Author

staskus commented Feb 21, 2024

Made the last little improvements, since I noticed a couple of issues after merging with the trunk:

  1. Make sure AnyHashableImmuTableRow equality == is checked correctly, without involving hashValue. The working solution turned out to be wrapping ImmutableRow into AnyHashable and then comparing ==, without this type of erasure Swift doesn't allow to compare Hashable protocols for equality. To reiterate, all of this is required since hashValue is used to determine identity of a row while equality == is used to determine whether a row needs to be updated.
  2. Wrote manual equality implementation for StatsTotalRowData. It took some time to realize that two objects that should be equal are not equal because UIImage object has changed although the image remained the same.

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 @staskus! The improvement in loading is noticeable! I couldn't spot any issues and don't have any feedback on the code.

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.

4 participants