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

Bugfix FXIOS-10482 Improve memory usage during tab tray open/close #22982

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mattreaganmozilla
Copy link
Collaborator

@mattreaganmozilla mattreaganmozilla commented Nov 7, 2024

📜 Tickets

Jira ticket
Github issue

💡 Description

Bandaid for a memory leak that occurs after opening/closing the tab tray. A general memory comparison graph (before/after) is shown below.

The underlying bug is still a problem unfortunately, and I believe we are still leaking a few of the container views higher up in the view hierarchy. Additionally, I'm seeing some other related scenarios that need further investigation and also might be leaking other UI.

This PR, however, does free up memory use considerably in this basic scenario, so I'm opening this for review now so we can have this available in the meantime until there is a better longterm solution in place.

Note: this appears to be a longstanding issue with the tab tray that predates any of the recent UI updates.


Memory Use Comparison

For this test the app was launched and then the tab tray was opened and closed repeatedly.

Allocations on device (iPhone 12 mini)

Before
Screenshot 2024-11-08 at 1 16 16 PM

After
Screenshot 2024-11-08 at 1 14 44 PM

Before
before

After
after

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

…n/close. This still needs investigation but this bandaid for now at least cleans up considerable memory in that specific scenario.
Copy link
Collaborator

@PARAIPAN9 PARAIPAN9 left a comment

Choose a reason for hiding this comment

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

Definitely an improvement, @mattreaganmozilla. I see now an approximate 1MB increase in memory compared to 7MB previously, so an approximate of 7 times improvement. Really nice job.

@mattreaganmozilla
Copy link
Collaborator Author

I see now an approximate 1MB increase in memory compared to 7MB previously

@PARAIPAN9 What steps were you using to test? I'm still investigating so I'll take a closer look; 1MB is still a big memory leak and I'm actually not seeing that with the fix in place.

@PARAIPAN9
Copy link
Collaborator

@mattreaganmozilla, the same steps: Opening and closing the tab tray.

@mattreaganmozilla
Copy link
Collaborator Author

mattreaganmozilla commented Nov 8, 2024

the same steps: Opening and closing the tab tray.

Ok interesting. For reference, I'm profiling on device (iPhone 12 mini) and if open/close the tray 10x these are the results I currently get:

Before
Screenshot 2024-11-08 at 1 16 16 PM

After
Screenshot 2024-11-08 at 1 14 44 PM

So this is just an approximation based on the output from Allocations, but across 10x repetitions I see about a ~1.2MB total increase in memory (or ~122kb each open/close) with the fix in place. Without the fix it's closer to ~25-30MB total (or about 2-3MB each open/close). So that's a factor of 20x or more.

In any case, I'd like to get the leak to zero so I'm going to keep this open a bit longer.

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