Closed Bug 1382966 Opened 3 years ago Closed 3 years ago

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

Categories

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

56 Branch
x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- disabled
firefox58 --- verified

People

(Reporter: Virtual, Assigned: mak)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [fxsearch])

Attachments

(1 obsolete file)

[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.
Flags: needinfo?(standard8)
Flags: needinfo?(mak77)
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.
Flags: needinfo?(standard8)
Flags: needinfo?(mak77)
Priority: -- → P1
Whiteboard: [fxsearch]
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.
no, it should just be faster, we don't want to introduce horrible progress bars.
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.
(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. ;)
Has Regression Range: --- → yes
Has STR: --- → yes
Summary: Speed and performance of deleting bookmarks is terrible and awful after landing patches from bug #1071513 → Speed and performance of batch operations on bookmarks are terrible and awful after landing patches from bug #1071513
Duplicate of this bug: 1385737
If that helps, here is a profile of deleting 37 bookmarks hanging the browser for ~4 seconds :
https://perfht.ml/2tWKYTv
(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 ...
(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.
Flags: needinfo?(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]
Flags: needinfo?(nivtwig)
Attachment #8892414 - Attachment is obsolete: true
Attachment #8892414 - Flags: review?(mak77)
Summary: Speed and performance of batch operations on bookmarks are terrible and awful after landing patches from bug #1071513 → Speed and performance of batch operations on bookmarks are terrible and awful after landing patch from bug #1071513
Unowned P1, please find an owner or alter the priority.
Flags: needinfo?(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.
Assignee: nobody → standard8
Flags: needinfo?(standard8)
Adding bug 1388687 to the deps as that might help nivtwig's case if I've understood their actions correctly.
Depends on: 1388687
Depends on: 1392533
let's convert this to a meta, and do actual work in dependencies
Assignee: standard8 → nobody
Keywords: meta
Priority: P1 → P5
Summary: Speed and performance of batch operations on bookmarks are terrible and awful after landing patch from bug #1071513 → [meta] Improve performance of bookmarks batch operations with Async Transactions
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
All dependencies were resolved as fixed, so let's mark this bug also as fixed.
Status: RESOLVED → VERIFIED
Resolution: WORKSFORME → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.