Closed Bug 1863783 Opened 4 months ago Closed 3 months ago

Switching tabs is slow because of Firefox View

Categories

(Firefox :: Firefox View, defect, P1)

defect

Tracking

()

RESOLVED FIXED
122 Branch
Tracking Status
firefox121 --- fixed
firefox122 --- fixed

People

(Reporter: jrmuizel, Assigned: sfoster)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fidefe-firefox-view])

Attachments

(1 file)

Profile is here: https://share.firefox.dev/49vQtJ9

I was switching back and forth between tabs. I'm seeing a bunch of time in chrome://browser/content/firefoxview/opentabs.mjs:render()

It seems like Firefox View is adding about 300ms to every tab switch.

Are you seeing this while in Firefox View and switching between tabs or not within View? And with a large amount of tabs?

This is happening while not using Firefox View. I have 2066 tabs.

I suspect the problem is that we keep event listeners and observers active for opentabs.mjs even if Firefox View isn't visible. It should be doing what other sections do and removing all of those when a user navigates away from View. There were helpers added in ViewPage for this reason.

Sam I know you're knee deep into the Open Tabs refactor right now. Does this get addressed in one of your patchs? Or do you have a WIP that I could piggy back on? This should be a simple change so we should try to get it into 121 if possible.

Severity: -- → S3
Flags: needinfo?(sfoster)
Priority: -- → P1
See Also: → 1858478
Whiteboard: [fidefe-firefox-view]

(In reply to Sarah Clements [:sclements] from comment #3)

Sam I know you're knee deep into the Open Tabs refactor right now. Does this get addressed in one of your patchs? Or do you have a WIP that I could piggy back on? This should be a simple change so we should try to get it into 121 if possible.

No this isn't directly addressed in my other patches. I think I can shuffle my queue and move this up to the front though. We do listen for visibility changes on the fxview document, and we already track which of the category pages is selected. I think we can extend and combine these to shut down all the observers and event listeners when a given page is not visible for whatever reason, and start the listening and observing for updates only when it is. That should make fxview inert when not selected and visible to the user, which should address this bug.

Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Flags: needinfo?(sfoster)

Thanks for picking this up Sam.

  • Add a .page class to the top-level ViewPage instances
  • Rename viewTabVisibleCallback and viewTabHiddenCallback to view* and call each when selectedness or visiblity changes
  • Ensure active view/pages are always properly initialized during page load and category switching
  • Add a test to verify no mutations happen when open tabs change while firefox view is inactive
Attachment #9363835 - Attachment description: WIP: Bug 1863783 - (WIP) Ensure fxview page content only listens/observes and renders updates when visible. r?#fxview-reviewers → Bug 1863783 - Ensure fxview page content only listens/observes and renders updates when visible. r?#fxview-reviewers
Attachment #9363835 - Attachment description: Bug 1863783 - Ensure fxview page content only listens/observes and renders updates when visible. r?#fxview-reviewers → * Bug 1863783 - Ensure fxview page content only listens/observes and renders updates when visible. r?#fxview-reviewers

I see this issue is in good hands, but in case this is useful here is a profile from my computer: https://share.firefox.dev/3SLMBh8. It's zoomed in on a specific switch but you can zoom out to see more if necessary.

Hope this helps

I'm surprised especially by the amount of GC. (this is also in Jeff's profile). Do you have an idea why this happens?

Attachment #9363835 - Attachment description: * Bug 1863783 - Ensure fxview page content only listens/observes and renders updates when visible. r?#fxview-reviewers → Bug 1863783 - Ensure fxview page content only listens/observes and renders updates when visible. r?#fxview-reviewers
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7015c326ee31
Ensure fxview page content only listens/observes and renders updates when visible. r=fxview-reviewers,sclements,jsudiaman
Regressions: 1866232
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 122 Branch

We'd like to consider this for beta uplift next week, but we want to let it sit in Nightly for a week or so and have QA test this early if possible.

:jrmuizel, are you able to verify this fixes the issue for you? The patch addresses the case where tabs are being selected/opened/closed/moved while the open tabs or recent browsing list is not visible. Ie. either you don't have firefox view open at all, or are on a different view/panel like recently-closed tabs.

Flags: needinfo?(jmuizelaar)

We tried verifying the fix but unfortunately it is too much to handle for the systems to open ~2k tabs at once (Ubuntu will crash completely because it is out of memory). We tried by opening a large amount of imported history and by using the profile shared by Sam in Bug 1859829.

On another note, no performance issues or strange behavior was seen while testing typical scenarios on the fixed builds.

(In reply to Catalin Sasca, Desktop QA [:csasca] from comment #13)

We tried verifying the fix but unfortunately it is too much to handle for the systems to open ~2k tabs at once (Ubuntu will crash completely because it is out of memory). We tried by opening a large amount of imported history and by using the profile shared by Sam in Bug 1859829.

Can you use the session file and procedure I attached to https://bugzilla.mozilla.org/show_bug.cgi?id=1858478#c6?
This will open 1000 tabs. But we only load the tab content when the tab is selected, so this shouldn't take up too much memory at startup. If that's still too much let me know and I can attach another session file.

Flags: needinfo?(csasca)

Sure thing. Used the session file and procedure attached to https://bugzilla.mozilla.org/show_bug.cgi?id=1858478#c6 and I couldn't reproduce any slowdowness/lags while opening random tabs on the affected build (2023-11-08). On the other hand the fixed build (2023-12-03) seemed a bit more snappier on macOS 13.6.1.

Flags: needinfo?(csasca)

Yes, this seems to solve the issue I was seeing. I don't see any firefox-view time during tab switching anymore.

Flags: needinfo?(jmuizelaar)

Comment on attachment 9363835 [details]
Bug 1863783 - Ensure fxview page content only listens/observes and renders updates when visible. r?#fxview-reviewers

Beta/Release Uplift Approval Request

  • User impact if declined: Users with slow machines and/or large numbers of open tabs may find browser performance slower after using firefox view
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low+ risk maybe? The changes are limited to firefox view. But the patch changes how and when all firefox view content is updated, making it so all updates (via observers and event listeners) are paused when the given section in firefox view (or the tab itself) is hidden, and resumed when it becomes visible.
  • String changes made/needed: None
  • Is Android affected?: No
Attachment #9363835 - Flags: approval-mozilla-beta?

Comment on attachment 9363835 [details]
Bug 1863783 - Ensure fxview page content only listens/observes and renders updates when visible. r?#fxview-reviewers

Approved for 121.0b8.

Attachment #9363835 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.