1382966 - [meta] Improve performance of bookmarks batch operations with Async Transactions

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P5)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Caused by:
Bug 1071513 - Enable async PlacesTransactions in Nightly

Workaround:
set "browser.places.useAsyncTransactions" preference to "false" in about:config

STR:
1. Select 300 bookmarks in one folder
2. Delete them
and see that they are deleted one by one,
which took over 1 minute to delete just only 300 bookmarks.

Comment 1

[Marco Bonardo [:mak]]

We're first looking into deprecation and removal of the old API, then we'll look into performance of batch removals. We can't do both at the same time because we have a double codebase to maintain atm.

At this time it is expected that async removals may be a bit slower than synchronous ones, due to their nature and the lack of batching (that is likely the main problem here, we could workaround it while waiting for bug 1340498 by firing artificial batch notifications).
Async transactions are only enabled in Nightly, so there's no reason to track this for 56, maybe 57. We'll do some measurement and see what we can improve easily.

Comment 2

[Kestrel]

Same with cut/copy/paste/undo, all operations are done incrementally. Since it takes some time there should be some feedback to the user on the status and progress of operations like with OS file managers.

Comment 3

[Marco Bonardo [:mak]]

no, it should just be faster, we don't want to introduce horrible progress bars.

Comment 4

[Marco Bonardo [:mak]]

To be clear, I appreciate the bug report and we'll address it, so please keep up reporting differences from before.
I just wanted to clarify that currently we are trying to make things work correctly, later we'll look into making them fast too.

Comment 5

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Marco Bonardo [::mak] from comment #4)
> so please keep up reporting differences from before.
Don't worry about that, we will "whine" about even least and smallest regression. ;)

Comment 7

[nivtwig]

If that helps, here is a profile of deleting 37 bookmarks hanging the browser for ~4 seconds :
https://perfht.ml/2tWKYTv

Comment 8

[nivtwig]

(In reply to nivtwig from comment #7)
> If that helps, here is a profile of deleting 37 bookmarks hanging the
> browser for ~4 seconds :
> https://perfht.ml/2tWKYTv

A small correction : ... of deleting a bookmarks folder with 37 bookmarks ...

Comment 9

[Mark Banner (:standard8)]

(In reply to nivtwig from comment #8)
> (In reply to nivtwig from comment #7)
> > If that helps, here is a profile of deleting 37 bookmarks hanging the
> > browser for ~4 seconds :
> > https://perfht.ml/2tWKYTv
> 
> A small correction : ... of deleting a bookmarks folder with 37 bookmarks ...

Looking at that, most of the JS time spent appears to be in _populateRecentBookmarks, and appears to be mainly in the query. How many bookmarks and history items do you have in your database?

Also, did you try this before and after the preference change? Given the location of timings in your profile, I think it is unlikely the async change has made a significant different to this.

Comment 10

[nivtwig]

(In reply to Mark Banner (:standard8) from comment #9)
> (In reply to nivtwig from comment #8)

> 
> Looking at that, most of the JS time spent appears to be in
> _populateRecentBookmarks, and appears to be mainly in the query. How many
> bookmarks and history items do you have in your database?
> 

*) According to about:support -> "Verify integrity", 
Table moz_places has 17230 records, Table moz_bookmarks has 29170 records . The full report is below at the end.

> Also, did you try this before and after the preference change? Given the
> location of timings in your profile, I think it is unlikely the async change
> has made a significant different to this.

*) Yes, I usually work with large bookmark folders (hundreds per folder), and the terribly slow performance of the delete operation is only in the last few days. Firefox 54 (release) is also much better in this regard, and also Nightly until the last few days. This is a regression.
Also Bug 1385733 is about a regression from the last few days in slow time to wait for the "Bookmark all tabs" dialog fields to appear when there are many tabs because the dialog probably tries to bookmark all tabs in the backgroud , and it takes.

The full "Verify integrity" report:
> Task: checkIntegrity
+ The database is sane
> Task: checkCoherence
+ The database is coherent
> Task: expire
+ Database cleaned up
> Task: vacuum
+ Initial database size is 15360 KiB
+ The database has been vacuumed
+ Final database size is 15360 KiB
> Task: stats
+ Database size is 15360 KiB
+ pragma_user_version is 38
+ pragma_page_size is 32768
+ pragma_cache_size is -2048
+ pragma_journal_mode is wal
+ pragma_synchronous is 1
+ History can store a maximum of 89495 unique pages
+ Table moz_places has 17230 records
+ Table moz_historyvisits has 238 records
+ Table moz_inputhistory has 6 records
+ Table moz_hosts has 563 records
+ Table moz_bookmarks has 29170 records
+ Table moz_keywords has 0 records
+ Table sqlite_sequence has 0 records
+ Table moz_favicons has 0 records
+ Table moz_anno_attributes has 8 records
+ Table moz_annos has 1 records
+ Table moz_items_annos has 53 records
+ Table sqlite_stat1 has 16 records
+ Table moz_bookmarks_deleted has 0 records
+ Index sqlite_autoindex_moz_inputhistory_1
+ Index sqlite_autoindex_moz_hosts_1
+ Index sqlite_autoindex_moz_keywords_1
+ Index sqlite_autoindex_moz_favicons_1
+ Index sqlite_autoindex_moz_anno_attributes_1
+ Index sqlite_autoindex_moz_bookmarks_deleted_1
+ Index moz_places_faviconindex
+ Index moz_places_hostindex
+ Index moz_places_visitcount
+ Index moz_places_frecencyindex
+ Index moz_places_lastvisitdateindex
+ Index moz_historyvisits_placedateindex
+ Index moz_historyvisits_fromindex
+ Index moz_historyvisits_dateindex
+ Index moz_bookmarks_itemindex
+ Index moz_bookmarks_parentindex
+ Index moz_bookmarks_itemlastmodifiedindex
+ Index moz_places_guid_uniqueindex
+ Index moz_bookmarks_guid_uniqueindex
+ Index moz_keywords_placepostdata_uniqueindex
+ Index moz_annos_placeattributeindex
+ Index moz_items_annos_itemattributeindex
+ Index moz_places_url_hashindex
> Task: _refreshUI
- JavaScript component does not have a method named: "onBeginUpdateBatch"'JavaScript component does not have a method named: "onBeginUpdateBatch"' when calling method: [nsINavHistoryObserver::onBeginUpdateBatch]

Comment 11

[Mark Banner (:standard8)]

Attachment: Bug 1382966 - Use skipDescendantsOnItemRemoval in nsNavHistoryResult to improve performance when deleting bookmark folders.

Review commit: https://reviewboard.mozilla.org/r/163364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163364/

Comment 12

[Jim Mathies [:jimm]]

Unowned P1, please find an owner or alter the priority.

Comment 13

[Mark Banner (:standard8)]

I'll take for now to make sure we do breakdown - realistically, this is probably a set of bugs to fix the issues with async places transactions which are only on for nightly at the moment (though hopefully will stay on for 57).

However: also note that the Search team generally has P1 bugs that are unassigned and get owners as we progress through the work, according to who gets to them first.

Comment 14

[Mark Banner (:standard8)]

Adding bug 1388687 to the deps as that might help nivtwig's case if I've understood their actions correctly.

Comment 15

[Marco Bonardo [:mak]]

let's convert this to a meta, and do actual work in dependencies

Comment 16

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

All dependencies were resolved as fixed, so let's mark this bug also as fixed.