460822 - Init all the fsync stuff lazily

Status: RESOLVED WONTFIX

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

Attachment: wip 0.1

Most of the Ts hit we took is due to us having to create 18 new sqlite items, particularly views and triggers are expensive.

So the basic idea about this approach is: "init views and triggers lazily, and use the old method (write to disk) till they are ready".

Practically all queries referencing to {table}_view are served through a getter that knows about current view status, on status changes current cached statements are thrown away, so next getter run will generate new ones.

Ideally this should remove the Ts hit almost completely and could show a Ts gain, i pushed to the tryserver to have some number.

This will most likely need refinements and fixes, it's not a finalized patch, i've seen some random test_expiration.js fail when the mode is switched during expiration, so that needs investigation.

Comment 1

[Marco Bonardo [:mak]]

(In reply to comment #0)
> i've seen some random test_expiration.js fail when the mode is switched during
> expiration, so that needs investigation.

this finished being a bug in current fsync patches, FindVisits was not applying the page cap correctly, see https://bugzilla.mozilla.org/show_bug.cgi?id=450705#c42.

Comment 2

[Marco Bonardo [:mak]]

Attachment: wip 0.2

Comment 3

[Shawn Wilsher :sdwilsh]

So, I'm not really sure what this wins us, other than gaming the benchmark.

We cannot create any of our queries that use the temporary tables or views until we do the lazy initialization.  So, then we need to have a copy of the queries around that just hit the permanent tables until we init the fsync stuff.  Doing all that initialization is going to hit the user hard when we finally do it, and it's going to lock up the UI for everyone - not just platforms that have issues with slow writes or fsyncs.

The end result is that we make our code much more complicated to beat a benchmark, but the user is still going to see this delay, but (in my opinion,) at a more inconvenient time.

Comment 4

[Marco Bonardo [:mak]]

(In reply to comment #3)
> So, I'm not really sure what this wins us, other than gaming the benchmark.

well from tryserver numbers we get a better Ts than before, it's for sure a benchmark gain, but will also gain on real browser startup.

> We cannot create any of our queries that use the temporary tables or views
> until we do the lazy initialization.  So, then we need to have a copy of the
> queries around that just hit the permanent tables until we init the fsync
> stuff.

i don't get this, queries are the same, yes we have to create the statement twice once to write to disk, and once to write to memory, that's transparent thanks to getters, i agree is more work but we're talking about 1ms

Doing all that initialization is going to hit the user hard when we
> finally do it, and it's going to lock up the UI for everyone - not just
> platforms that have issues with slow writes or fsyncs.

That could be true once per session, we're however talking about 20ms, with changes that could be done async, in that case it would not lock up anything.

> The end result is that we make our code much more complicated to beat a
> benchmark, but the user is still going to see this delay, but (in my opinion,)
> at a more inconvenient time.

I agree with you and i would prefer taking the thing as it is, but if that's not possible, this is something we could make better (using async queries maybe), from my tests i didn't notice hangs or issues, all seem to happen transparently, but for sure we will hit a bit when the switch happens.

However this change has win us a bug fix in fsync stuff :)

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #4)
> > We cannot create any of our queries that use the temporary tables or views
> > until we do the lazy initialization.

hwv this is true only for the views, temporary tables are still created on startup but they are empty, so all queries that don't refer to views works correctly without any change

Comment 6

[Marco Bonardo [:mak]]

findVisits change splitted to bug 460947

Comment 7

[Marco Bonardo [:mak]]

actually, this is not a good idea

Comment 8

[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