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

Fix performance issue in ProfileView #1744

Merged
merged 2 commits into from
Jan 29, 2025
Merged

Conversation

mplorentz
Copy link
Member

@mplorentz mplorentz commented Jan 28, 2025

Issues covered

#171

Description

This was a weird one. I wasn't able to figure out exactly why this was happening, and I didn't try to do a full git bisect after I realized that the issue was a race condition and not immediately reproducible.

What I observed is that AppView would start constantly redrawing the ProfileTab. This caused the ProfileTab to recreate the ProfileView, which triggered the ProfileView.onAppear to be called, which sends a one-time request to the RelayService to download some of the profile data. These events would be downloaded faster than they could be parsed, filling up the parse queue and causing the app to feel slow.

My working theory is that something (deep in Core Data?) was constantly mutating the signed in Author in some subtle way, somehow triggering the willChange callback which @ObservedObject picked up and caused SwiftUI views to redraw. Switching to the new SwiftUI @Observable observation system from ObservedObject seemed to do the trick. I just switched it everywhere because it's better and supposedly has no downsides.

But I'm a bit nervous about this because we tried switching the observation system in https://github.com/planetary-social/nos/pull/1590/files but it didn't fully work. We were also trying to add some caching in there though so maybe the observation system wasn't the problem?

How to test

  1. Filter the logs to only "Analytics" messages
  2. Tap back and forth between all the different tabs as fast as you can, in any order
    You should not see "Analytics: Profile View Opened" start printing to the console constantly.

It's also worth smoke testing for other observation system issues. I did a fresh install of the app and looked at some of the other updated views and they looked good, but maybe you can do the same.

@joshuatbrown
Copy link
Contributor

👀

@joshuatbrown
Copy link
Contributor

On the whole, this looks good. I don't see any evidence of the performance issue, so it looks to me like it's fixed.

I do see an issue where my notes don't all appear in the feed, though I've also seen this in the released version so I don't think it's a regression. I'll write a ticket for that.

@pelumy
Copy link
Contributor

pelumy commented Jan 29, 2025

I still see "Analytics: Profile View Opened" logged continously both on a fresh install and on an existing install.

But I noticed that it only shows up if the Profile View has been opened at least once. If the Profile View was never opened, it doesn't get logged.

@mplorentz
Copy link
Member Author

mplorentz commented Jan 29, 2025

I still see "Analytics: Profile View Opened" logged continously both on a fresh install and on an existing install.

😱 @pelumy I'm not able to reproduce that given the steps I used in the ticket. Can you maybe share a video of how you are triggering it?

@pelumy
Copy link
Contributor

pelumy commented Jan 29, 2025

@mplorentz , please see the video below:

Initially, I didn't tap the ProfileView, but immediately I opened the ProfileView, the analytics still got logged even though I was taping the Home and Discover tabs.

Screen.Recording.2025-01-29.at.9.29.06.pm.mov

@mplorentz
Copy link
Member Author

@pelumy I see. Yeah it's weird that "Profile View Opened" is firing every time. However this is still a big improvement over what I was seeing which was like a hundred "Profile View Opened" messages every second.

I'm going to go ahead and merge this, since I think it does fix the performance issue. Then I'll to look further into the issue you are seeing and hopefully fix it in another PR.

@mplorentz mplorentz enabled auto-merge January 29, 2025 21:59
@mplorentz mplorentz added this pull request to the merge queue Jan 29, 2025
Merged via the queue into main with commit 97b557c Jan 29, 2025
4 checks passed
@mplorentz mplorentz deleted the fix-profile-appear-loop branch January 29, 2025 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants