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

bookmarksStorage.getBookmarksWithUrl takes unexpectedly long time (10 - 50ms) #6370

Closed
mcomella opened this issue Oct 31, 2019 · 5 comments
Closed
Labels
performance Possible performance wins wontfix

Comments

@mcomella
Copy link
Contributor

mcomella commented Oct 31, 2019

Severin mentioned to me that:

bookmarksStorage.getBookmarksWithUrl(session.url).firstOrNull { it.url == session.url }

(source) takes anywhere from 10-50 MS in a unit test even if there are a small number of bookmarks, like 4: this is an unexpectedly long time.

We should investigate possible causes.


I wonder if the unit test call sthis before the DB is initialized (e.g. initial data creation, or even just loading the DB from disk) so calling this sets up the DB and is what makes this take a long time, but we wouldn't see this in practice.

┆Issue is synchronized with this Jira Task

@mcomella mcomella added the performance Possible performance wins label Oct 31, 2019
@mcomella mcomella added this to Needs prioritization in Performance, front-end roadmap via automation Oct 31, 2019
severinrudie added a commit to severinrudie/fenix that referenced this issue Oct 31, 2019
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 1, 2019
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 5, 2019
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
@hawkinsw
Copy link
Contributor

hawkinsw commented Nov 5, 2019

Actions

  1. Get median #, p90 of bookmarks that real mobile users have. Is this expected to change with new versions of Fenix? @ecsmyth
  2. Where is this called from within Fenix? That will determine how we prioritize. @hawkinsw

@mcomella mcomella moved this from Needs prioritization to In progress in Performance, front-end roadmap Nov 5, 2019
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 6, 2019
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 7, 2019
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 8, 2019
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 11, 2019
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 11, 2019
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie pushed a commit that referenced this issue Nov 12, 2019
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits
mozilla-l10n-automation-bot pushed a commit to mozilla-l10n-automation-bot/android-l10n that referenced this issue Nov 12, 2019
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile/fenix#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits

X-Channel-Revision: [master] mozilla-mobile/android-components@ba3f75e
X-Channel-Converted-Revision: [master] mozilla-mobile/fenix@6909a76
X-Channel-Revision: [master] mozilla-mobile/firefox-tv@e8ccd59
X-Channel-Revision: [master] MozillaReality/FirefoxReality@ebc151c
X-Channel-Revision: [master] mozilla-lockwise/lockwise-android@e070cfc
mozilla-l10n-automation-bot pushed a commit to mozilla-l10n-automation-bot/android-l10n that referenced this issue Nov 12, 2019
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile/fenix#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits

X-Channel-Revision: [master] mozilla-mobile/android-components@ba3f75e
X-Channel-Converted-Revision: [master] mozilla-mobile/fenix@6909a76
X-Channel-Revision: [master] mozilla-mobile/firefox-tv@e8ccd59
X-Channel-Revision: [master] MozillaReality/FirefoxReality@ebc151c
X-Channel-Revision: [master] mozilla-lockwise/lockwise-android@e070cfc
mozilla-l10n-automation-bot pushed a commit to mozilla-l10n-automation-bot/android-l10n that referenced this issue Nov 12, 2019
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile/fenix#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits

X-Channel-Revision: [master] mozilla-mobile/android-components@ba3f75e
X-Channel-Converted-Revision: [master] mozilla-mobile/fenix@6909a76
X-Channel-Revision: [master] mozilla-mobile/firefox-tv@e8ccd59
X-Channel-Revision: [master] MozillaReality/FirefoxReality@ebc151c
X-Channel-Revision: [master] mozilla-lockwise/lockwise-android@e070cfc
mozilla-l10n-automation-bot pushed a commit to mozilla-l10n-automation-bot/android-l10n that referenced this issue Nov 12, 2019
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile/fenix#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits

X-Channel-Revision: [master] mozilla-mobile/android-components@ba3f75e
X-Channel-Converted-Revision: [master] mozilla-mobile/fenix@6909a76
X-Channel-Revision: [master] mozilla-mobile/firefox-tv@e8ccd59
X-Channel-Revision: [master] MozillaReality/FirefoxReality@ebc151c
X-Channel-Revision: [master] mozilla-lockwise/lockwise-android@e070cfc
mozilla-l10n-automation-bot pushed a commit to mozilla-l10n-automation-bot/android-l10n that referenced this issue Nov 12, 2019
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile/fenix#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits

X-Channel-Revision: [master] mozilla-mobile/android-components@ba3f75e
X-Channel-Converted-Revision: [master] mozilla-mobile/fenix@6909a76
X-Channel-Revision: [master] mozilla-mobile/firefox-tv@e8ccd59
X-Channel-Revision: [master] MozillaReality/FirefoxReality@ebc151c
X-Channel-Revision: [master] mozilla-lockwise/lockwise-android@e070cfc
@mcomella
Copy link
Contributor Author

mcomella commented Dec 27, 2019

Where is this called from within Fenix? That will determine how we prioritize.

@ecsmyth This is called when:

  • The URL changes (to update the internal "isUrlBookmarked"? state). This occurs off the main thread.
  • The user taps the bookmark star in the 3-dot menu (to determine if we should go directly to the "edit" screen or if we should save and then go to the "edit"). This occurs off the main thread and there is a brief delay before displaying the appropriate UI element.

Since this happens off the main thread, I think it's probably unimportant.

@mcomella mcomella removed their assignment Dec 27, 2019
@boek boek added the P1 Current sprint label Dec 27, 2019
@ecsmyth ecsmyth added P2 Upcoming release and removed P1 Current sprint labels Jan 6, 2020
@ecsmyth ecsmyth removed their assignment Jan 6, 2020
@ecsmyth
Copy link

ecsmyth commented Jan 6, 2020

Based on @mcomella's comment, I'm dropping the priority to P2 to align with proposed performance release criteria.

@mcomella mcomella added P1 Current sprint and removed P2 Upcoming release labels Jan 6, 2020
@mcomella mcomella moved this from In progress to Needs prioritization in Performance, front-end roadmap Jan 6, 2020
@mcomella
Copy link
Contributor Author

mcomella commented Jan 6, 2020

Fenix uses the P* labels, not us, so replacing the label and sending back to triage.

@mcomella mcomella moved this from Needs prioritization to Low impact (unprioritized) in Performance, front-end roadmap Jan 21, 2020
@vesta0 vesta0 removed the P1 Current sprint label Apr 20, 2020
@stale
Copy link

stale bot commented Feb 4, 2021

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 Feb 4, 2021
@stale stale bot closed this as completed Feb 11, 2021
Performance, front-end roadmap automation moved this from Low impact (unprioritized) to Done Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
performance Possible performance wins wontfix
Projects
None yet
Development

No branches or pull requests

5 participants