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

Perform deeper database cleanup upon Settings clear cache #4574

Merged
merged 9 commits into from Aug 2, 2023
Merged

Conversation

tonisevener
Copy link
Collaborator

Phabricator:
https://phabricator.wikimedia.org/T341104

Notes

This PR adds some additional database and caching cleanup logic to the Settings > Clear Cache action. It leans on our existing WMFDatabaseHousekeeper and SharedContainerCacheHousekeeping (which we currently trigger via BackgroundTasks). I'm passing in a new WMFCleanupLevel parameter to indicate that from these contexts we want to do a deeper clean in hopes of removing vandalism. In regards to the requirements of https://phabricator.wikimedia.org/T341104, we are deleting all Explore feed data and refreshing it upon completion.

Test Steps

  1. Load several days worth of Explore feed data.
  2. Tap Clear Cache via Settings.
  3. Main thread should not be blocked, and an alert should appear when the cleanup task has completed.
  4. Explore feed should refresh.

@staykids staykids requested review from a team and staykids and removed request for a team August 1, 2023 18:28
@staykids staykids self-assigned this Aug 1, 2023
Copy link
Contributor

@staykids staykids left a comment

Choose a reason for hiding this comment

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

Nice - inspecting the SQLite db, looks like WMFContentGroup is successfully clearing with this change. The current plan is to get this in the with the diffs release, so changes needed are 1) 7.4.0 as the base and 2) merge conflicts resolved.

- (void)clearCache {

[NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(showClearCacheInProgressBanner) object:nil];
[self performSelector:@selector(showClearCacheInProgressBanner) withObject:nil afterDelay:1.0];
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't yet experienced seeing this banner, but only because I guess my explore feed content when testing hasn't been large enough for the deletion to take long. Seems like a good delay value here (for me at least!) to not have to see quick double banners appear!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good, glad you haven't seen any double banners!

Yeah, this may be me going overboard on performance testing, but I (programmatically) loaded a year's worth of Explore feed data and tested this on my old iPhone 7. It caused an alarming UI freeze in this situation, which is why I added 4c244cb to perform the cleanup on a different thread and show banners.

So if something will take a while, the user will see that a cleanup started via the banner, but the UI is still responsive. They can then go back to the Explore feed. Once cleanup completes, the Explore feed refreshes and the "complete" banner displays.


@objc private func databaseHousekeeperDidComplete() {
DispatchQueue.main.async {
self.refresh()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm on occasion running into a situation where the content is deleted, but the explore feed doesn't refresh (see screenshot). I have English (primary) and Arabic in my languages, scroll several days worth of explore feed content, then perform a Settings > Clear cached data operation. I haven't been able to reliably reproduce this though yet. Certainly not a dealbreaker as pulling to refresh the explore feed still refreshes the feed, but just wanted to check in to see if you'd encountered anything like this yourself.
sim

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch - I hadn't noticed that. I am occasionally able to reproduce. When watching the traffic, I notice AR Wikipedia sometimes takes a long time to load the featured endpoint, particularly with older dates. Maybe its a server-side caching thing? In my testing, July 15th took 1+ minutes to load, and July 14th timed out.

I suspect at this line, it's exiting early, due to a previous slow paging operation. I could maybe bypass this check in situation if you think I should - let me know what you think.

@staykids staykids removed their assignment Aug 1, 2023
@tonisevener tonisevener changed the base branch from main to 7.4.0 August 2, 2023 15:09
Copy link
Contributor

@staykids staykids 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 making those branch changes/the conflict resolutions!

@tonisevener tonisevener merged commit 828fdfb into 7.4.0 Aug 2, 2023
2 checks passed
@tonisevener tonisevener deleted the T341104 branch August 2, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants