Closed Bug 1701660 Opened 2 years ago Closed 9 months ago

Multiple UI refreshes degrade performance of CSV Import

Categories

(Firefox :: about:logins, defect, P2)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
97 Branch
Tracking Status
firefox87 --- disabled
firefox88 --- disabled
firefox89 --- disabled
firefox97 --- verified

People

(Reporter: cmuntean, Assigned: serg, Mentored)

References

(Blocks 2 open bugs, )

Details

Attachments

(2 files, 1 obsolete file)

[Notes]:

  • Importing a CSV file with 1000 logins takes between 3 to 10 minutes. During the import, there are multiple performance issues that can be observed. The performance issues get worse after ~800/1000 logins are imported. Here are a few examples of performance issues observed:

    • Firefox can’t navigate to any page using the address bar until all logins have been imported.
    • The top sites from the new tab, cannot be opened while the CSV file is imported. If one of the top sites is clicked, then a new tab is opened, the page remains blank until the logins are imported.
    • In certain cases, Firefox also freezes. This happens after a few minutes while the importing process is about to end (when ~850 logins are imported).
    • If the about:logins page is closed while importing a CSV file then if it's reopened, the page remains blank until all the logins are imported..
    • The hover effect has a delay.
    • Opening new tabs or Firefox menus have a delay.
  • Considering the fact that this is a performance issue we have captured a performance profile using the Firefox Profiler addon on multiple machines:

[Affected versions]:

  • Firefox Nightly 89.0a1 (Build ID:20210328213901);
  • Firefox Beta 88.0b (Build ID: 20210328185936)

[Affected Platforms]:

  • Windows 10 x64;
  • macOS 10.15.7;
  • Linux Mint 20 x64;

[Prerequisites]:

  • Have the latest Firefox Beta build installed.
  • Have a large CSV file that contains multiple logins or you can use this one (1004 logins): file.

[Steps to reproduce]:

  1. Open the Firefox browser with the profile from prerequisites.
  2. Navigate to the "about:logins" page.
  3. Open the menu, click the "Import from a file..." option.
  4. Select and import the CSV file described in perquisites.
  5. Wait until ~800 logins are imported.
  6. Open a new tab, click on the top sites, navigate to multiple websites.
  7. Open different Firefox menus and click on options.
  8. Observe how the browser behaves.

[Expected result]:
There are no noticeable performance issues.

[Actual result]:

  • Different performance issues can be observed.

[Additional Notes]:

  • Attached a screen recording of the issue.
See Also: → 1702336

A quick glance shows some low hanging fruit here. We shouldn't _setListItemAsSelected and scrollIntoView here when we are importing. We might have to refactor things though to avoid this, as we simply react to the loginAdded message in the same way as if the user had just created a single new login entry.

https://searchfox.org/mozilla-central/source/browser/components/aboutlogins/content/components/login-list.js#719-734

We may need to signal from the importer that we are expecting a batch of new records and signal again when the import is done so the UI can flush and re-draw once instead of per new record.

This feature is held to nightly only for now, so I'll mark as P2. But it should be the first thing we address to try and get the feature ready for shipping.

Priority: -- → P2
Blocks: 1705529
Assignee: nobody → tgiles

Adding a link to my investigation of this bug. There a lot of synchronous operations we are doing on the main thread in multiple places during the import process. We need to solve those as well as optimize the rendering pipeline for login items.

I don't believe solving these performance issues will result in particularly novel solutions. Because of this, we should have some resources on file on how to mitigate and solve these kind of performance issues. If we don't have these resources, then this bug could be a good case study of how to use the profiler, moving main thread IO off the main thread (or minimizing main thread IO if we can't move all the work off the main thread), and optimizing the pixels to screen pipeline. The performance best practices for Firefox front-end engineers highlights these issues at a high level so maybe this bug can expand on those high level points.

Assignee: tgiles → nobody
See Also: → 1658766

This is not a trivial bug, but if someone wants to investigate and develop a patch, I'm very happy to mentor them through it. I'm happy to add to Tim's notes (comment #4) and chat to help guide a contributor through a step by step process to solving this. We don't need to boil the ocean here, we should start with profiling, identifying the main sources of slowness, setting up to respond to batch rather than one-by-one login additions and take it from there.

Mentor: sfoster
Assignee: nobody → sgalich
Status: NEW → ASSIGNED
Severity: S2 → S3

(In reply to Sam Foster [:sfoster] (he/him) from comment #5)

... but if someone wants to investigate and develop a patch, I'm very happy to mentor them through it. ...

This bug (and the subsequent disabled-by-default of signon.management.page.fileImport.enabled) directly impacts me.

My current solution is to add a default parameter to loginAdded() in login-list.js. This can then be used to selectively skip the render operation when a second parameter is passed (without requiring any refactoring other than in LoginHelper.jsm#344).
If this seems like a reasonable solution I would be more than happy to get started on the process of getting this bundled up into whatever the Mercurial equivalent of a pull-request is (I've never used Mercurial, but am more than happy to do so if it means we can get this pref switched back).

If a more robust solution (that addresses the fact that this workload doesn't belong on the main-thread) is desired I am more than happy to start testing solution for that locally.

(In reply to OutdatedConcept from comment #7)

If a more robust solution (that addresses the fact that this workload doesn't belong on the main-thread) is desired I am more than happy to start testing solution for that locally.

Sorry it's taking so long, I'm looking into better ways to structure whole import process. Current approach does not scale well.

Significant loss of performance happens because of thousands notifications for each login added. They cause thousands of reloads in UI and it takes a lot of time. I've attached a patch to suppress notifications while importing and do a bulk reload at the end. Still going to be some delays, but for me it works much faster than before. Can you take a look and share your observations please?

Attachment #9256349 - Attachment description: WIP: Bug 1701660 - Performance issues while importing a large CSV file with ~1000 logins → Bug 1701660 - Performance issues while importing a large CSV file with ~1000 logins r=tgiles,dimi
Blocks: 1748491

Attachment 9256349 [details] improves performance by removing unnecessary UI refreshes. Bug 1748491 will focus on CSV file reading/parsing/processing (see comment for details).

Summary: Performance issues while importing a large CSV file with ~1000 logins → Multiple UI refreshes degrade performance of CSV Import
Attachment #9256349 - Attachment description: Bug 1701660 - Performance issues while importing a large CSV file with ~1000 logins r=tgiles,dimi → Bug 1701660 - Multiple UI refreshes degrade performance of CSV Import r=tgiles,dimi
Pushed by sgalich@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b66e64a607d
Multiple UI refreshes degrade performance of CSV Import r=tgiles
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch

I have verified this issue on the latest Nightly 97.0a1 build (Build ID: 20220105093353) on Windows 10 x64, macOS 10.15.7 and Linux Mint 20.

  • In order to verify this I have followed the STR from comment 0. I have imported a CSV file that contains 1,004 logins, after ~4 seconds, all the logins are imported and the "Import Complete" dialog is displayed. I haven't encountered any performance issues while verifying this.

Considering this, I will mark this issue as verified - fixed.

Status: RESOLVED → VERIFIED
Attachment #9227749 - Attachment is obsolete: true
Regressions: 1754468
You need to log in before you can comment on or make changes to this bug.