Open Bug 1405242 Opened 7 years ago Updated 1 year ago

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

Categories

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

Product:
Firefox
Component:
Bookmarks & History
Version:
56 Branch
Platform:
x86_64
Windows 7
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox56 --- disabled
firefox57 --- disabled
firefox63 --- wontfix

People

(Reporter: Virtual, Unassigned)

References

(Depends on 1 open bug)

Details

(4 keywords, Whiteboard: [fxsearch])

Attachments

(3 files)

moving bookmarks - profile.sps.json.gz
1.12 MB, application/gzip
Details
deleting bookmarks - profile.sps.json.gz
2.22 MB, application/gzip
Details
bookmarks.html
1.33 MB, text/html
Details 
Follow up to Bug #1382966


STR1:
1. Select 500 bookmarks in one folder
2. Delete them
and see that they are deleted one by one,
which took 89 seconds to delete 500 bookmarks.

STR2:
1. Select 500 bookmarks in one folder
2. Move them to another folder
and see that they are moved one by one,
which took 55 seconds to move 500 bookmarks.


Caused by:
Bug 1071513 - Enable async PlacesTransactions in Nightly

Workaround:
Set "browser.places.useAsyncTransactions" preference to "false" in about:config
to get back nearly instantaneous batch operations
Has Regression Range: --- → yes
Has STR: --- → yes
Is this on Nightly? I see the bug has been filed as 56 branch, and we don't care about 56 since the feature is disabled by default there.
Flags: needinfo?(Virtual)
Yes. It was tested on latest Mozilla Firefox Nightly 58.0a1 (2017-10-02) (64-bit).
Flags: needinfo?(Virtual)
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #0)
> 1. Select 500 bookmarks in one folder
> 2. Delete them
> and see that they are deleted one by one,
> which took 89 seconds to delete 500 bookmarks.

I can't reproduce this one. Please can you detail exactly which buttons you're pressing, and in which views. I am testing in the library window, selecting 300 bookmarks, right-click and delete, it takes a couple of seconds and they are all removed from the UI at the same time (further speed improvements we're working on in other bugs).

That was changed by bug 1392533 which recently landed.

Either the nightly you tested wasn't the latest, or I suspect you may be using a different route.

> STR2:
> 1. Select 500 bookmarks in one folder
> 2. Move them to another folder
> and see that they are moved one by one,
> which took 55 seconds to move 500 bookmarks.

This one I can reproduce, though I'm not quite sure why it is happening yet.
Flags: needinfo?(Virtual)
I wonder if Sync or another add-on is involved. The removals can easily fire hundreds of notifications.
Whiteboard: [fxsearch]
(In reply to Mark Banner (:standard8) from comment #4)
> (In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see
> your comment/reply/question/etc.) from comment #0)
> > 1. Select 500 bookmarks in one folder
> > 2. Delete them
> > and see that they are deleted one by one,
> > which took 89 seconds to delete 500 bookmarks.
> 
> I can't reproduce this one. Please can you detail exactly which buttons
> you're pressing, and in which views. I am testing in the library window,
> selecting 300 bookmarks, right-click and delete, it takes a couple of
> seconds and they are all removed from the UI at the same time (further speed
> improvements we're working on in other bugs).
> 
> That was changed by bug 1392533 which recently landed.
> 
> Either the nightly you tested wasn't the latest, or I suspect you may be
> using a different route.
> [...]

STR:
1. Open "Library"
2. Go to some folder with many bookmarks (1.000 in this case/or 500 in faster STR mode)
3. Select 500 bookmarks (by pressing Ctrl+left mouse button after scrolling some time from 1 to 500 bookmark/or by pressing Ctrl+A in faster STR mode)
4. Delete them (by pressing Delete keyboard button or selecting Delete item in mouse context menu
go for drinks as it will take some time ;)


Oddly deleting whole folder with many bookmarks works fast.



(In reply to Marco Bonardo [::mak] from comment #5)
> I wonder if Sync or another add-on is involved. The removals can easily fire
> hundreds of notifications.

Even when I disable:
> CanvasBlocker-Beta 0.4.0RC2
> Cookie AutoDelete 2.0.0b3
> Decentraleyes 2.0.0beta3
> Disable HTML5 Autoplay 2017.8.20
> Don't touch my tabs! (rel=noopener) 1.0
> Don't track me Google 4.15
> Download Star 1.0.0
> Firefox Pioneer 1.0
> h264ify 1.0.6
> uBlock Origin 1.14.11rc10
all my extensions, it's still reproducible,
and I'm not using Sync.


FYI - my places integrity information:
> > Task: checkIntegrity
> + The database is sane
> > Task: checkCoherence
> + The database is coherent
> > Task: expire
> + Database cleaned up
> > Task: vacuum
> + Initial database size is 71680 KiB
> + The database has been vacuumed
> + Final database size is 71680 KiB
> > Task: stats
> + Database size is 71680 KiB
> + pragma_user_version is 40
> + 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 120924 unique pages
> + Table moz_places has 121716 records
> + Table moz_historyvisits has 65601 records
> + Table moz_inputhistory has 0 records
> + Table moz_hosts has 6468 records
> + Table moz_bookmarks has 83574 records
> + Table moz_bookmarks_deleted has 0 records
> + Table moz_keywords has 0 records
> + Table sqlite_sequence has 1 records
> + Table moz_anno_attributes has 9 records
> + Table moz_annos has 77382 records
> + Table moz_items_annos has 65859 records
> + Table sqlite_stat1 has 16 records
> + Index sqlite_autoindex_moz_inputhistory_1
> + Index sqlite_autoindex_moz_hosts_1
> + Index sqlite_autoindex_moz_bookmarks_deleted_1
> + Index sqlite_autoindex_moz_keywords_1
> + Index sqlite_autoindex_moz_anno_attributes_1
> + Index moz_places_url_hashindex
> + Index moz_places_hostindex
> + Index moz_places_visitcount
> + Index moz_places_frecencyindex
> + Index moz_places_lastvisitdateindex
> + Index moz_places_guid_uniqueindex
> + 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_bookmarks_guid_uniqueindex
> + Index moz_keywords_placepostdata_uniqueindex
> + Index moz_annos_placeattributeindex
> + Index moz_items_annos_itemattributeindex
> + Index moz_bookmarks_dateaddedindex
> > Task: _refreshUI
Flags: needinfo?(Virtual)
+ Table moz_bookmarks has 83574 records
this is indeed A LOT. Do you have so many bookmarks, or are you an heavy tags user?
Thank you for all the information.
(In reply to Marco Bonardo [::mak] from comment #7)
> + Table moz_bookmarks has 83574 records
> this is indeed A LOT. Do you have so many bookmarks, or are you an heavy
> tags user?
> Thank you for all the information.

I have over 80k, yes, over 80.000.
Most of them all only for archivisation purposes and probably won't be used ever.
I have been gathering them starting from 1998 year, when I was starting having access to internet.
Now, I have about 20-25k bookmarks sorted in detail, others are unfortunately still unsorted and probably 99% of them still will be deleted, but I will sort them "SOON™".

I don't use tags. I'm using folders. Better way for me to manage such amount of bookmarks.

P.S. Some part of my unsorted bookmarks can be seen in attachments in bug #1320534, if you want them for testing by yourself.
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #6)
> STR:
> 1. Open "Library"
> 2. Go to some folder with many bookmarks (1.000 in this case/or 500 in
> faster STR mode)
> 3. Select 500 bookmarks (by pressing Ctrl+left mouse button after scrolling
> some time from 1 to 500 bookmark/or by pressing Ctrl+A in faster STR mode)
> 4. Delete them (by pressing Delete keyboard button or selecting Delete item
> in mouse context menu
> go for drinks as it will take some time ;)

Can you confirm that this definitely looks there is one bookmark being deleted at a time?

Can you also confirm it is just bookmarks, no sub-folders that you're deleting?

Bug 1392533 should have made this specific case update the UI only once (which is what I see locally). It sounds like somehow that the batching is being turned off.


> Oddly deleting whole folder with many bookmarks works fast.

It is a different path, where we know we're deleting the whole folder, so there's lots of optimisations that are happen as we know we don't need to deal with the individual bookmarks.
Flags: needinfo?(Virtual)
Priority: -- → P1
(In reply to Mark Banner (:standard8) from comment #9)
> Can you confirm that this definitely looks there is one bookmark being
> deleted at a time?
> [...]
> Bug 1392533 should have made this specific case update the UI only once
> (which is what I see locally). It sounds like somehow that the batching is
> being turned off.
Partially,
it looks like whole bath is being deleted,
but by hearing HDD (not to mention time), I suspect that it's still deleting bookmarks one by one.
So no one by one animation on deleting bookmarks, like it's with moving them,
and nearly 50% more time to delete bookmarks, compared to moving them.


(In reply to Mark Banner (:standard8) from comment #9)
> Can you also confirm it is just bookmarks, no sub-folders that you're
> deleting?
Yes.
I redone test and especially created "test" folder in Bookmarks Toolbar,
added "https://bugzilla.mozilla.org/show_bug.cgi?id=1405242" as bookmark,
next duplicated it 499 times and did STR.
Flags: needinfo?(Virtual)
Moving bookmarks - https://perfht.ml/2yXqtoJ

Gecko Profiler 0.17 settings:
Interval = 100ms
Buffer size = 54MB
Threads =
Features = Stack walk + Java Scrip
Deleting bookmarks - https://perfht.ml/2yoJiUC

Gecko Profiler 0.17 settings:
Interval = 100ms
Buffer size = 54MB
Threads =
Features = Stack walk + JavaScript
This is on a mechanical disk right?
I wonder if the problem is just there and related to the lack of a transaction.
Virtual_ManPL: Could you test out the build I link to below?

It is an experimental patch and is incomplete, so you might want to backup your profile first or duplicate it.

The patch should improve the copy & paste performance or the copy drag action (move is not implemented yet). Generally I've been testing with selecting contents of one folder, and copying to a newly created folder.

On my machine, this is getting copying of 300 bookmarks down from around 3 seconds to around 500 milliseconds. I'm hopeful it will help yours situation as well.

Note: redo & undo are unlikely to work properly as well. I'm not sure it'll work with folders or separators either. This is mainly a PoC and I want to find out how much it helps your situation.

For comparing, using the current nightly would be the best thing to compare against.

https://queue.taskcluster.net/v1/task/MskU4fb0Sz2dThi6Qdlb1Q/runs/0/artifacts/public/build/target.zip
Flags: needinfo?(Virtual)
Attached file bookmarks.html — 
latest Mozilla Firefox Nightly 58.0a1 (2017-10-17) (64-bit)
moving 1000 bookmarks - 2m 38s
deleting 1000 bookmarks - 28s

patched version
moving 1000 bookmarks - looks instant, but it only copy/duplicate 1000 bookmarks to new folder, not move
deleting 1000 bookmarks - 35s

Test was performed always on Firefox Portable installation with clean new fresh profile without any addons (extensions, plugins, themes, etc.) with imported bookmarks.
Flags: needinfo?(Virtual)
(In reply to Marco Bonardo [::mak] from comment #13)
> This is on a mechanical disk right?

Yes.
No longer blocks: 1404267
Virtual_ManPL: In today's nightly (20171031235118), we've just landed bug 1411891. You should see an improvement in the delete operations. Could you retry it and see how long it takes now?

Next up I'll be working on getting the move/copy performance improved.
Depends on: 1411891
Flags: needinfo?(Virtual)
Depends on: 1404909, 1413843
(In reply to Mark Banner (:standard8) from comment #17)
> Virtual_ManPL: In today's nightly (20171031235118), we've just landed bug
> 1411891. You should see an improvement in the delete operations. Could you
> retry it and see how long it takes now?
> 
> Next up I'll be working on getting the move/copy performance improved.

Awesome! Thank you very much!
It's still not that fast, as without Async Transactions, but it's nice performance boost.

comparing to old version
(Virtual_ManPL [:Virtual] - wrote in comment #15)
> Created attachment 8919705 [details]
> bookmarks.html
> 
> latest Mozilla Firefox Nightly 58.0a1 (2017-10-17) (64-bit)
> moving 1000 bookmarks - 2m 38s
> deleting 1000 bookmarks - 28s

latest Mozilla Firefox Nightly 58.0a1 (2017-11-06) (64-bit)
moving 1000 bookmarks - 2m 40s
deleting 1000 bookmarks - 4s
Flags: needinfo?(Virtual)
let's make this a meta, and track dependencies.
Keywords: meta
Priority: P1 → P5
I suspect Bug 1426103 may be related or depends on this bug
Depends on: 1460849
Too late to fix in 63. We could still take a patch for 65 and potentially for 64.
status-firefox63: affected → wontfix
status-firefox64: affected → fix-optional
There's been ongoing work for this, and I don't think it is worth drivers tracking this. For the majority of users, the bookmarks amounts are less than 2k and have a reasonable time for functions. Bug 1404909 recently landed (in 62) which should have greatly sped up the move case.

The only other optimisations we can do here are likely to be a side-effect of bug 1473530 (or possibly a follow-up to if necessary) - which is improving how we manage the notifications system, and allowing notifications to be grouped into one. Hence adding that as a dependency, and we can re-evaluate when it is done.
status-firefox64: fix-optional → ---
Depends on: 1473530
Summary: Improve performance of bookmarks batch operations with Async Transactions → [meta] Improve performance of bookmarks batch operations with Async Transactions
Depends on: 1562490

Moving open bugs with topperf keyword to triage queue so they can be reassessed for performance priority.

Performance Impact: --- → ?
Keywords: topperf
Performance Impact: ? → ---

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Severity: major → --
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: