Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

SyncedTabsTrayLayout attached when switching in/out of private tabs tray position in the tabs tray #23199

Closed
grigoryk opened this issue Jan 14, 2022 · 6 comments
Labels
eng:perf-impact This issue may have an impact on performance: the perf team will investigate Feature:SyncTabs Sync tabs Feature:Tabs needs:triage Issue needs triage pin Issues, features, improvements that are still valid

Comments

@grigoryk
Copy link
Contributor

grigoryk commented Jan 14, 2022

I've noticed that while switching between normal and private parts of the tabs tray, we are doing a lot of allocations for SyncedTabs-related objects.

Turns out that, somehow, that we are hitting SyncedTabsTrayLayout#onAttachedToWindow when going in and out of private tabs tray view. This starts syncedTabsFeature, which will start the configured presenter, which ends up calling controller's refreshSyncedTabs and syncAccount, kicking off bunch of expensive work.

This happens every time private tabs are opened in the tray. Looking at this briefly, I can see that the getItemViewType is being hit with position=2 when switching to private tray page, via LinearLayoutManager's layout logic (there's its next method in the stack trace somewhere). Position=2 maps the synced tabs tray page, which ends up instantiating and attaching SyncedTabsPageViewHolder.

Now, the synced tabs tray page is getting converted to compose in #22798, but it's not clear to me that ^^ won't happen in that branch since the overall flow of things is mostly the same after the conversion.

cc @MozillaNoah

┆Issue is synchronized with this Jira Task

@grigoryk grigoryk added Feature:Tabs eng:perf-impact This issue may have an impact on performance: the perf team will investigate Feature:SyncTabs Sync tabs labels Jan 14, 2022
@github-actions github-actions bot added the needs:triage Issue needs triage label Jan 14, 2022
@MozillaNoah
Copy link
Contributor

MozillaNoah commented Jan 14, 2022

This sounds like part of how ViewPager2 works, since it by default pre-loads the immediately adjacent Views/Fragments and has them go though most of their start-up lifecycle. So this bug will probably be here until TabsTray is completely Composified or the ViewPager is told to load ONLY the active View/Fragment.

@jonalmeida
Copy link
Contributor

Composified or the ViewPager is told to load ONLY the active View/Fragment.

If we need to make the Synced Tabs page a special one, let's try to not do that for all the other tabs. Going from private to normal (or vice versa) would be slower than expected otherwise and we do a decent amount of things in the normal tabs list already that affect performance.

With a ViewPager2 we can kind of do this today, but I don't know how that would work when the whole tray is Compose.

@jonalmeida
Copy link
Contributor

Another thing that could make this better is if we had a persistent or cached state of Synced Tabs so that even after a cold boot, we didn't have to query for tabs immediately. I had this issue filed for it as well in a-s: mozilla/application-services#3373

@stale
Copy link

stale bot commented Nov 21, 2022

See: #17373 This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 21, 2022
@stale stale bot closed this as completed Dec 5, 2022
@jonalmeida
Copy link
Contributor

This is still a valid bug and we should fix it - mozilla/application-services#3373 is fixed and we can use the cached version of synced tabs to display and only when visible should we trigger another sync.

@jonalmeida jonalmeida reopened this Dec 7, 2022
@jonalmeida jonalmeida added pin Issues, features, improvements that are still valid and removed wontfix labels Dec 7, 2022
@jonalmeida
Copy link
Contributor

Moved to bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1810774

Change performed by the Move to Bugzilla add-on.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
eng:perf-impact This issue may have an impact on performance: the perf team will investigate Feature:SyncTabs Sync tabs Feature:Tabs needs:triage Issue needs triage pin Issues, features, improvements that are still valid
Projects
None yet
Development

No branches or pull requests

3 participants