Distributed feed updating will cause frequent temp table syncs (and fsyncs), make livemarks dynamic containers

RESOLVED FIXED

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: mak, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 obsolete attachments)

(Reporter)

Description

9 years ago
We are changing feeds updating to distribute them, what will happen when we will sync on bookmarks?
with many feeds updating could last for minutes, and we could be synching temp tables often.

Since feeds are continuously added and removed from moz_bookmarks table and probably are not so useful in offline mode we could find some way to hold them in a temp table, feed children are not real bookmarks (they are not starred for example)
Maybe we should consider making feeds children not real bookmarks?  It seems like this is a prime candidate for a smart container, IMHO.
(Reporter)

Comment 2

9 years ago
oh well the network outage has eaten my comment, i had written a long comment about this :( Hwv, i have a self made patch to move livemarks to a temp table, it's a wip to test this possibility. I can tell that we have following results:

the idea is change livemarks from being folders to being queries like "place:type=8" (RESULTS_AS_LIVEMARK_CONTENTS)

pros:
- no fsyncs!
- we can do fast updates on background without heavy disk usage / ui lockups, also with a ton of livemarks
- code is cleaner since we don't have to filter out them from bookmarks
- we don't have to cheat isBookmark for the awesomebar
- naturally readonly
- checking if an item is a livemark or livemark child is faster (only check resultType)

cons:
- visiting a livemark child creates a TRANSITION_TYPED visit (imho it should be TRANSITION_LINK but since we don't know anything about that url it's added as TYPED)
- we have one more temp table to create and manage (i still have to find a good place to put the init)
- on browser close, livemark children are lost (so on every restart we have to reparse the feed).
- no feed children in offline mode, this is not a big deal imo because having a livemark child but don't have the possibility to visit it, is somehow useless.
(Reporter)

Comment 3

9 years ago
Created attachment 339953 [details] [diff] [review]
really wip

to build this you need:
- latest patch from bug 407468
- a new profile (i still have not written migration code)

known issues to be fixed:
- table init code sucks (Any idea on where to put that?)
- favicons are M.I.A. (needs a subselect)
- livemark children sorting could be/is wrong
- livemark children date could be taken from the feed entry if it exists (now it's the table insertion time)
- migration code missing (where? add a db schema change?)
- refresh times are wrong, we should force a refresh at start always, regardless expiration

So, i'm attaching to have comments and discuss about directions we could follow to solve our "livemarks don't love hard disks" issue.
(Reporter)

Comment 4

9 years ago
i'll try to move investigation toward dynamic containers, we never used them, could be a good candidate, since probably they were originally created for livemarks.

Actually i've a new version of the previous patch that works better, but has still some issue (mainly Library right pane cannot distinguish between different places queries with the same url, so i would need to change the library or add another attribute to the query). will put that apart and later do a comparation between the two approaches.
(Reporter)

Comment 5

9 years ago
Created attachment 340262 [details] [diff] [review]
dynamic containers wip

first take with dynamic containers... this is full of bugs, we never really used them and are mostly untested, for example i already catched a crash in navHistoryResult where we were not taking dynamic_container type into account.

major concern for now is that the feed is populated when the container is opened, so when a livemark is empty the loading item appear, and then disappear, but to see children you have to close and reopen the popup. This can maybe be fixed at the view level, however, before that, i need to make them working correctly.
Attachment #339953 - Attachment is obsolete: true
Marco - can I suggest that those crashers and whatnot that you find get filed into other bugs so the reviewers are easier to do (smaller patches are easier to look at than one big one)?
(Reporter)

Comment 7

9 years ago
(In reply to comment #6)
> Marco - can I suggest that those crashers and whatnot that you find get filed
> into other bugs so the reviewers are easier to do (smaller patches are easier
> to look at than one big one)?

yes, i can split them later though, i'd like to have a general view of what is needed before splitting
(Reporter)

Comment 8

9 years ago
Created attachment 340324 [details] [diff] [review]
dynamic wip 0.2

fixed more issues with dynamic containers, mainly changes we have done where we did not consider they were a special type of folder, working almost fine on menus/toolbar, still throwing in trees, but appear as a good change for now, so i'm optimistic.
all tests pass except test_bookmarks_html.js, will look at it (i have to test import/export backup/restore, will do after having the ui trees working), probably i have to do some work in importExport service.

taking bug for now.

PS: bug 407468 has landed so, from now on, to test this, you only need to pull latest trunk.
Assignee: nobody → mak77
Attachment #340262 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Reporter)

Updated

9 years ago
Depends on: 329534
(Reporter)

Updated

9 years ago
Depends on: 405922, 405921
(Reporter)

Comment 9

9 years ago
Dietrich: about bug 405922, should we sanitize anything received from the feed, or can we consider the data safe since it comes from the feed processor? I think it is safe, we were already adding it to the database, so i don't see what different kind of problems we could hit putting those uris/titles into dynamically generated nodes.
Finally, if bug 405922 is a wontfix, can you mark it like that?
(Reporter)

Comment 10

9 years ago
Created attachment 340561 [details] [diff] [review]
dynamic wip 0.3

DONE (in this version):
- fixed library assertions
- fixed persist state
- fixed hasChildren
- fixed icons
- fixed context menus
- fixed appendURINode for excludeItems parents/roots

TODO:
- the Library does not show children nor properties for livemarks
- fix import/export
- split dynamic containers bugs to smaller patches on dependant bugs

you should be able to use livemarks on toolbar/menus/sidebar, as to migrate from old profiles.

We will want/need the distribute patch in before taking such a change. I see a little slowdown when updating livemarks in a bunch, i think it would be completely transparent if done with a little "pause" after every feed. I think we can stay under 1s though, and discard the pref about "how many at once", doing always 1 each time. So probably update1->0.5s->update1->0.5s could be enough. Hwv i hope Ayakawa can unbitrot that fast, so i can build upon that.

I have a little doubt about extensibility. Taking away livemarks from the bookmarks table is also taking them away from extension developers, it's true they can create a local dynamic container and ask that, but should we provide them some API in the livemark service to access single livemarks children? should i fill a followup or is the dyn container access enough?
Attachment #340324 - Attachment is obsolete: true
(Reporter)

Comment 11

9 years ago
i had to create a nsNavHistoryDynamicContainerResultNode, it's derived from base container, but inherit also from nsNavHistoryQueryContainerResultNode.
That's needed for the Library since on the right pane we generate a query from the node and set pane location to that query, so we are doing asQuery(node), all node types support this (folders and queries), dynamic containers were basic containers so they could not inherit from nsNavHistoryQueryContainerResultNode that is a derived class. that will generate a place:folder=ID with the id of the dynamic container that will work flawlessy.
In future we could move dynamic container work to overloaded methods, removing that from the basic container impl when possible.

will fix import/export and post a new patch that should be good to create a tryServer build
(Reporter)

Comment 12

9 years ago
Created attachment 340900 [details] [diff] [review]
wip 0.4

updated with livemark load distribute, we reload 1 livemark at a time, with 2s delay between each, all tests PASS

the only issue i actually see is that the details pane in the library does not show the title of livemark children, will look into it later, instead i'll start splitting needed dynamic containers bug fixes now.

advantages with this approach are:
- no fsyncs
- no need to filter them out in queries (speed)
- no heavy disk usage
- no db fragmentation (due to continuous inserts and deletes).

disadvantages:
- Have to reload all livemarks every time you start the browser even if expiration time (15mins by default) has not passed.
- Memory usage goes up, we're storing livemark children into a memory array (uri, title, date), if you try this bookmarks.html https://bugzilla.mozilla.org/attachment.cgi?id=326686, containing so many livemarks, the memory usage goes up to more than 100MB and the OS starts swapping, with all livemarks loaded i get about 100MB used. As a compare current nigthly with all livemarks loaded uses about 50MB. That case is an edge case since i doubt a user could have so many livemarks, however is something to log here.
Attachment #340561 - Attachment is obsolete: true
(Reporter)

Comment 13

9 years ago
Created attachment 340913 [details] [diff] [review]
patch v1

fixed titles in the Library details pane. apart previous comment disadvantages i can't find any other issue.
Attachment #340900 - Attachment is obsolete: true
(Reporter)

Updated

9 years ago
Depends on: 457677
(Reporter)

Updated

9 years ago
Depends on: 457681
(Reporter)

Updated

9 years ago
Depends on: 457686
(Reporter)

Updated

9 years ago
Depends on: 457698
(Reporter)

Updated

9 years ago
Depends on: 457708
(Reporter)

Comment 14

9 years ago
Created attachment 340940 [details] [diff] [review]
patch v1.1

splitted out dynamic containers bugs/patches to dependancies. This is what is left.
Attachment #340913 - Attachment is obsolete: true
(Reporter)

Updated

9 years ago
Blocks: 449640
No longer depends on: 405921, 405922, 449640

Comment 15

9 years ago
Hi Marco have you considered some way for a user to disable/enable the polling of specific livemarks?
That is... I may only want to actively update a small number of the total livemarks I have subscribed to at any one time. 
An example would be the 27 that I am currently interested in out of 114 in total that I have subscribed to.

Not being able to disable the 87 livemarks that I am currently not following magnifies the delay for me 4 fold.

Some users may still want these updated but maybe once a day rather than every hour.

So instead of disable/enable maybe the way to go would be a Priority Value for each livemark.

0 is disabled
60 is every hour.
1440 is every day.
(Reporter)

Comment 16

9 years ago
(In reply to comment #15)
> Hi Marco have you considered some way for a user to disable/enable the polling
> of specific livemarks?

No, not for now, please fill the request like an enhancement bug, hwv i guess what UI would be needed to do efficiently that with a really big number of livemarks.
Is this still on the radar for 1.9.1?
(Reporter)

Comment 18

9 years ago
from what i can tell, change to use dynamic containers is not.

we could "workaround" some common issue trying to implement a sort of incremental update...
(Reporter)

Updated

8 years ago
Whiteboard: [going to 3.2, livemarks to dynamic containers, needs more dynamic containers work]
(Reporter)

Updated

8 years ago
Summary: distributed feed updating will cause frequent temp table syncs (and fsyncs) → Distributed feed updating will cause frequent temp table syncs (and fsyncs), make livemarks dynamic containers
(Reporter)

Updated

8 years ago
Blocks: 488968
(Reporter)

Comment 19

8 years ago
Comment on attachment 340940 [details] [diff] [review]
patch v1.1

current patch is not what we want.
We need to have a separate table/database for livemarks children, and an API that allows extension developers to annotate them with custom properties through livemarks service (properties could then be pushed into the nodes' propertyBag)
Also all of this should use storage async API.
Attachment #340940 - Attachment is obsolete: true
(Reporter)

Updated

8 years ago
Flags: wanted-firefox3.6?
Whiteboard: [going to 3.2, livemarks to dynamic containers, needs more dynamic containers work]
(Reporter)

Updated

8 years ago
Blocks: 322068
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
(Reporter)

Updated

7 years ago
Status: ASSIGNED → NEW
(Reporter)

Updated

7 years ago
Assignee: mak77 → nobody
(Reporter)

Comment 21

5 years ago
this is fixed by 613588
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Depends on: 613588
Resolution: --- → FIXED
(Reporter)

Updated

4 years ago
Flags: wanted-firefox3.6?
You need to log in before you can comment on or make changes to this bug.