1701660 - Multiple UI refreshes degrade performance of CSV Import

Status: VERIFIED FIXED

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

Description

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Attachment: Screencast 2021-03-29 02_22_27.mp4

[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:

  • Windows 10 x64, Intel Core i7 - 10750H, 16 GB RAM: https://share.firefox.dev/3u4cXve
  • macBook Pro (Early 2015), macOS 10.15.7, Intel Core i5, 8 GB RAM: https://share.firefox.dev/3sBkKA8
  • Linux Mint 20, AMD FX (™) - 8320, 16 GB RAM: https://share.firefox.dev/3lZd60d
  • Windows 10 x64, Intel Core i5-6500, 16 GB RAM: https://share.firefox.dev/3m2iNKK

[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.

Comment 1

[Sam Foster [:sfoster] (he/him)]

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.

Comment 2

[Sam Foster [:sfoster] (he/him)]

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.

Comment 3

[Tim Giles [:tgiles]]

Attachment: WIP: Bug 1701660 - add WIP bulk add logins logic

Comment 4

[Tim Giles [: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.

Comment 5

[Sam Foster [:sfoster] (he/him)]

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.

Comment 6

[ragimeg698]

The key to copy links is broken. It was broken in 88 and remains unfixed.

Copying links is a core function of a web browser. It is completely unreasonable that this function should be removed. Tens, maybe hundreds of millions of people use hotkeys. It is an absolutely normal, absolutely acceptable way to interface with a computer. All of those people have had to find solutions outside of Firefox, and for absolutely no reason.

A "replacement" key that is not physically usable is obviously not a replacement.

Since my legitimate reports about this critical bug are being removed, I'm leaving comments on other people's bugs.

--

Comment 7

[OutdatedConcept]

(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.

Comment 8

[Sergey Galich [:serg]]

Attachment: Bug 1701660 - Multiple UI refreshes degrade performance of CSV Import r=tgiles,dimi

Comment 9

[Sergey Galich [:serg]]

(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?

Comment 10

[Sergey Galich [:serg]]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b66e64a607d
Multiple UI refreshes degrade performance of CSV Import r=tgiles

Comment 12

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/1b66e64a607d

Comment 13

[Cosmin Muntean [:cmuntean], Ecosystem QA]

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.