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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

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)

Comment 1

[Shawn Wilsher :sdwilsh]

Maybe we should consider making feeds children not real bookmarks?  It seems like this is a prime candidate for a smart container, IMHO.

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Marco Bonardo [:mak]]

Attachment: 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.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Marco Bonardo [:mak]]

Attachment: 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.

Comment 6

[Shawn Wilsher :sdwilsh]

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)?

Comment 7

[Marco Bonardo [:mak]]

(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

Comment 8

[Marco Bonardo [:mak]]

Attachment: 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.

Comment 9

[Marco Bonardo [:mak]]

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?

Comment 10

[Marco Bonardo [:mak]]

Attachment: 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?

Comment 11

[Marco Bonardo [:mak]]

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

Comment 12

[Marco Bonardo [:mak]]

Attachment: 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.

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1

fixed titles in the Library details pane. apart previous comment disadvantages i can't find any other issue.

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v1.1

splitted out dynamic containers bugs/patches to dependancies. This is what is left.

Comment 15

[SilverWave]

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.

Comment 16

[Marco Bonardo [:mak]]

(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.

Comment 17

[Shawn Wilsher :sdwilsh]

Is this still on the radar for 1.9.1?

Comment 18

[Marco Bonardo [:mak]]

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...

Comment 19

[Marco Bonardo [:mak]]

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.

Comment 20

[Gervase Markham [:gerv]]

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

Comment 21

[Marco Bonardo [:mak]]

this is fixed by 613588