Open Bug 1320534 Opened 7 years ago Updated 11 months ago

[meta] Speed and performance of importing bookmarks from JSON or HTML files is awful

Categories

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

defect

Tracking

()

Performance Impact low

People

(Reporter: Virtual, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [fxsearch:p5])

Attachments

(8 files, 1 obsolete file)

1.96 MB, application/x-7z-compressed
Details
4.57 MB, application/x-7z-compressed
Details
2.43 MB, application/x-7z-compressed
Details
4.57 MB, application/x-7z-compressed
Details
10.46 KB, image/png
Details
3.75 KB, text/plain
Details
3.70 KB, text/plain
Details
1.27 KB, text/plain
Details
Importing about 75.000 bookmarks, either from 35MB json file or 25MB html file, took over 12 hours to complete, with hundreds of thousands slow script warnings which I marked to ignore after few times...

Other competitive browser complete this task with seconds, not in half day...
Hundreds of thousands slow script warnings:
> Script: chrome://browser/content/places/editBookmarkOverlay.js:1055
> Script: chrome://browser/content/browser-places.js:1750

STR:
1. Try to import large database of bookmarks, either from json or html file
2. Try to be patient, because you will have to wait many hours to complete this task

Firefox is only using about 6% of whole CPU and about 98% of one of CPU threads on latest high end CPU...
Has STR: --- → yes
Depends on: 1340498
Flags: firefox-backlog?
Priority: -- → P3
This doesn't need to block bug 1332225, since the codebases differ.
The code that is being added for solvign bug 1332225 could be used here instead, making BookmarkHTMLUtils and BookmarkJSONUtils use the new insertTree API.
No longer blocks: 1332225
Depends on: 1344282
Do you have a large db like the one you're talking about that you feel comfortable attaching, that reproduces this problem for you?
Flags: needinfo?(Virtual)
Sure.

Which one of these: places.sqlite, bookmarks.html, bookmarks-2017-02-11.json; should I upload or all of them?
Flags: needinfo?(Virtual) → needinfo?(gijskruitbosch+bugs)
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me) from comment #4)
> Sure.
> 
> Which one of these: places.sqlite, bookmarks.html,
> bookmarks-2017-02-11.json; should I upload or all of them?

Well, comment #0 mentions you're importing from the HTML or JSON file, so I guess those. Probably want to zip them if they're that large to avoid bugzilla's attachment size limit.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(Virtual)
Apart from using the new API, there are various other problems here.
First, the notification system has a lot of overhead. But this is complicate and depends on completely refactoring it. Alternatively though, the nsnavhistoryresult observer should likely have skipDescendantsOnItemRemoval, since the ui doesn't need to remove every single descendant before removing a folder. This could provide a nice speedup on removals of entire trees of bookmarks. Dunno if it may cause some unexpected issues, but hopefully it would just work.

with just the new API and the observer change we could already have a pretty interesting gain.
In comment #7, comment #8, comment #9 and comment #10, I attached my bookmarks (unfortunately not sorted to subfolders and subcategories yet and mostly will be just probably deleted) which I can share publicly, it's only 1/4 part of all my bookmarks, but feel free to use all like you want. I'm suggesting duplicating about them 4 times, creating about 10 main folder with about 10 subfolders etc. and pasting in each folder about 1.000 bookmarks etc.
Keywords: hang
Summary: Bookmarks importing speed and performance is terrible and awful → Bookmarks importing speed and performance is terrible and awful with countless hangs and script warnings
Whiteboard: [qf]
Depends on: 1095426, 1095427
Flags: qe-verify+
Priority: P3 → P2
QA Contact: adrian.florinescu
Whiteboard: [qf] → [photon-performance] [qf]
Whiteboard: [photon-performance] [qf] → [photon-performance] [qf:p1]
Whiteboard: [photon-performance] [qf:p1] → [photon-performance] [qf:p1][fxsearch]
Gijs, may I know if your fixes on bug 1344282 and bug 1344759 have improved the problem? How can we follow-up here? Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Evelyn Hung [:evelyn] from comment #12)
> Gijs, may I know if your fixes on bug 1344282 and bug 1344759 have improved
> the problem? How can we follow-up here? Thanks!

To fix this bug someone needs to update the importers for JSON and HTML files to use the API from bug 1344282. Those importers weren't changed as part of bug 1344759, so those bugs in and of themselves will not have positively affected this usecase.
Flags: needinfo?(gijskruitbosch+bugs)
It sounds to me that this bug should be covered by Photon-onboarding as we are going to do bookmark auto migration. 

Fischer, please update your findings here if you think it might not be relevant. Thanks.
Flags: needinfo?(fliu)
Whiteboard: [photon-performance] [qf:p1][fxsearch] → [photon-performance] [qf:p1][fxsearch][photon-onboarding]
(In reply to Evelyn Hung [:evelyn] from comment #14)
> It sounds to me that this bug should be covered by Photon-onboarding as we
> are going to do bookmark auto migration. 
> 
> Fischer, please update your findings here if you think it might not be
> relevant. Thanks.
Thanks, we should consider how this bug is going to impact user experience on auto migration.
Blocks: 1361283
Flags: needinfo?(fliu)
(In reply to Evelyn Hung [:evelyn] from comment #14)
> It sounds to me that this bug should be covered by Photon-onboarding as we
> are going to do bookmark auto migration. 
> 
> Fischer, please update your findings here if you think it might not be
> relevant. Thanks.

Auto-migration only operates on migration from other browsers. All of those have already been updated (except 360se). This bug is not about that - it is specifically about manually importing bookmarks from JSON or HTML bookmark files. That code isn't used at all by the auto-migration operations.
No longer blocks: 1361283
Flags: needinfo?(ehung)
Summary: Bookmarks importing speed and performance is terrible and awful with countless hangs and script warnings → Speed and performance of importing bookmarks from JSON or HTML files is awful
(In reply to :Gijs from comment #16)
> (In reply to Evelyn Hung [:evelyn] from comment #14)
> > It sounds to me that this bug should be covered by Photon-onboarding as we
> > are going to do bookmark auto migration. 
> > 
> > Fischer, please update your findings here if you think it might not be
> > relevant. Thanks.
> 
> Auto-migration only operates on migration from other browsers. All of those
> have already been updated (except 360se). This bug is not about that - it is
> specifically about manually importing bookmarks from JSON or HTML bookmark
> files. That code isn't used at all by the auto-migration operations.

Got it. Thanks for clarifying this for me. :)
Flags: needinfo?(ehung)
Whiteboard: [photon-performance] [qf:p1][fxsearch][photon-onboarding] → [photon-performance] [qf:p1][fxsearch]
I captured a profile with a much smaller bookmarks.html (1570 bookmark items): https://perfht.ml/2pSszDF

The highlighted section contains 2 parts:

1. Real work of Bookmarks.jsm (~6 sec)
In this part, Bookmarks takes about 1300 ms, and Sqlite.jsm takes about 500 ms. In contrast, Promise-backend.jsm takes about 5000 ms. This shows that more than half of the time are spent in the overhead of promises instead of actual import of bookmarks.

I think batching will help in reducing the promise overhead.

2. Cycle collection (~3 sec)
It's surprising that importing 1570 bookmark items requires a cycle collection of 3 seconds. That means we generated too many JS-facing native objects. We need to profile the CC heap to see what are collected.
what's the point of doing lots of measurements and micro-optimizations, when we are using the wrong API in the first place?
Once we have converted the component to use the correct API, then it will be a good time to measure.
Assignee: nobody → jaas
Josh, what are you planning to do here? Mark is already working on bug 1095427, and regardless I'm not sure what we'd do here if we don't first address dependencies.
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [photon-performance] [qf:p1][fxsearch] → [reserve-photon-performance] [qf:p1][fxsearch]
also, how can this be qf:p1 if its dependencies are qf:p3?
(In reply to Marco Bonardo [::mak] from comment #20)
> Josh, what are you planning to do here? Mark is already working on bug
> 1095427, and regardless I'm not sure what we'd do here if we don't first
> address dependencies.

I have been working on converting the HTML import code in BookmarkHTMLUtils.jsm to the insertTree API, as prescribed earlier in this bug's comments.

I don't see any indication that anyone is working on bug 1095427 (no assignee, no recent comments), and I'm not sure how it's a prerequisite for this work. Can you explain?
Here's what I've got so far. This converts the HTML importer to use the insertTree API. There are a bunch of issues that still need to be fixed.
I see, then I assume you want to work in bug 1095427, rather than in this tracking bug. Its title looks wrong, let me fix it.
Josh, I've been working on bug 1095426 for JSON import, I think some of that answers the questions that you've got in your patch, or at least I can answer them. I'll get a new patch up on that bug later today hopefully, but if you're moving your patch across, then I can add some comments once it has been moved.
Assignee: jaas → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
Attachment #8868888 - Attachment is obsolete: true
Whiteboard: [reserve-photon-performance] [qf:p1][fxsearch] → [reserve-photon-performance] [qf:p2][fxsearch]
No longer blocks: qf-bugs-upforgrabs
Priority: P3 → P4
Whiteboard: [reserve-photon-performance] [qf:p2][fxsearch] → [reserve-photon-performance] [qf:p3][fxsearch]
Hey dthayer, would your work in bug 1414892 have any impact on importing bookmarks like in comment 0? Or is the change in bug 1414892 strictly for history entries?
Flags: needinfo?(dothayer)
I haven't looked too deeply into bookmark inserts yet, but off the top of my head, I'm relatively certain it goes through maybeInsertManyPlaces, so I would expect bookmark imports to be faster as a result of this.
Flags: needinfo?(dothayer)
Great! Hey Virtual_Man - bug 1414892 landed a few days back and might help here. Any improvement in a recent Nightly?
Flags: needinfo?(Virtual)
Update: after reading through some of the comments, I'm not sure. It looks like this doesn't use insertTree yet(?). If not, then bug 1414892 probably doesn't have an effect.
Both JSON & HTML both now are async & use insertTree. There's been a lot of changes since this was first filed, and there's other things that we've been doing that will likely have affected performance as well (e.g. the transition to async, but also subsequent tidy-ups).

Note also, 75000 bookmarks is an extreme case, 95% of users have around 1500 bookmarks.
Priority: P4 → P5
Some test, brace yourself.



Exporting ~86.000 bookmarks in Mozilla Firefox Nightly 59.0a1 (2017-12-19) (64-bit) took:
- 33 seconds  [to JSON format] 
- 1 minutes 4 seconds  [to HTML format]


Importing ~86.000 bookmarks to JSON format in Mozilla Firefox Nightly 59.0a1 (2017-12-19) (64-bit) took:
- 08 minutes 47 seconds
- 03 hours 10 minutes and ended with error message (see "error.png" attachment), so complete bookmark backup wasn't restored properly and ended in data lose, as only 82.223 bookmarks were restored (see "bugged - Places Database Integrity from about;config.txt" attachment), not 84.640 bookmarks (see "not bugged - Places Database Integrity from about;config.txt" attachment) [with browser.places.useAsyncTransactions=false],
but I won't be reporting this issue, as Async Transactions are already enabled by default in Firefox 58 per bug #1404267, so it will be meaningless.


Importing ~86.000 bookmarks to HTML format in Mozilla Firefox Nightly 59.0a1 (2017-12-19) (64-bit) took:
- 13 minutes 57 seconds
- 03 hours 41 minutes 56 seconds [with browser.places.useAsyncTransactions=false]


Importing ~86.000 bookmarks from HTML format in Mozilla Firefox competitors took:
- 29 seconds in Chromium 65.0.3300.0 (64-bit)
- 29 seconds in Google Chrome Canary 65.0.3299.5 (64-bit)
- 30 seconds in Google Chrome 65.0.3294.5 (32-bit)
- 36 seconds in Opera developer 51.0.2809.0 (64-bit)
- 37 seconds in Vivaldi 1.14.1042.3 (64-bit)
- over 7h and still not ended, but my patience ended, so I shutdown Brave 0.19.121 (64-bit), what's more some bookmarks were imported, but not all, also Brave 0.19.121 (64-bit) eaten all available memory in this operation and on Brave 0.19.121 (64-bit) restarting same thing happened, so I suspect that I broke Brave 0.19.121 (64-bit) profile completely



tl;dr
In over 1 year, time to import ~80.000 bookmarks in HTML format in Mozilla Firefox was reduced from over 12 hours to nearly 4 hours without Async Transactions and to nearly 14 minutes with Async Transactions.
It's awesome improvement! Thank you very much!
Still there is some more work to do here, as Mozilla Firefox time is nearly 14 minutes and Chromium does this in only 29 seconds.
Flags: needinfo?(Virtual)
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #31)
> as only 82.223 bookmarks were restored [...], not 84.640 bookmarks

should be
> as only 82.819 bookmarks were restored [...], not 85.265 bookmarks
as I copied wrong values.
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #31)
> In over 1 year, time to import ~80.000 bookmarks in HTML format in Mozilla
> Firefox was reduced from over 12 hours to nearly 4 hours without Async
> Transactions and to nearly 14 minutes with Async Transactions.
> It's awesome improvement! Thank you very much!
> Still there is some more work to do here, as Mozilla Firefox time is nearly
> 14 minutes and Chromium does this in only 29 seconds.

Thank you for testing, luckily html is not that common, as well as users with that many bookmarks.
Regardless, I agree there is still space for improvement in importing from both html and json. I must note that Chrome keeps bookmarks in memory at runtime and doesn't support tags (and keywords I think), and those likely helps it.
Priority: P5 → --
Whiteboard: [reserve-photon-performance] [qf:p3][fxsearch] → [fxperf][qf:p3][fxsearch]
Priority: -- → P5
Whiteboard: [fxperf][qf:p3][fxsearch] → [fxperf][qf:p3][fxsearch:p5]
Keywords: topperfmeta
Whiteboard: [fxperf][qf:p3][fxsearch:p5] → [fxperf:p3][qf:p3][fxsearch:p5]
Depends on: 1473530
No longer depends on: 1340498
Summary: Speed and performance of importing bookmarks from JSON or HTML files is awful → [meta] Speed and performance of importing bookmarks from JSON or HTML files is awful
Performance Impact: --- → P3
Whiteboard: [fxperf:p3][qf:p3][fxsearch:p5] → [fxperf:p3][fxsearch:p5]

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 → --
Depends on: 1562490
Keywords: perfperf:frontend
Whiteboard: [fxperf:p3][fxsearch:p5] → [fxsearch:p5]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: