Closed Bug 512854 Opened 15 years ago Closed 15 years ago

VACUUM places.sqlite database on daily idle once a month

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: mak, Assigned: mak)

References

()

Details

Attachments

(2 files, 3 obsolete files)

From first measurements actually a vacuum of an 86MB database took about 140ms, using the "memory journal" trick, it can go down to 60ms.
Based on these numbers, doing this on daily idle should be possible and wanted.

actually we can try to get a guess about the fact the db needs to be vacuumed or not, just because vacuuming too much could reduce speed of INSERTs.

this will actually be done for Places that is usually the bigger and more changed db and its slowdown is immediately visible in the UI responsiveness, if everything goes fine we can port the thing more globally (or just wait for Sqlite to implement complete incrementa vacuum)
I like the idea of vacuuming during idle time, but IMO once per day seems like overkill.  Once per month sounds better.
Marco, how exactly did you measure these numbers? Even for a 14 MB database, I get much higher numbers:

   time (echo "VACUUM;" | sqlite3 places.sqlite)

   real	0m2.207s
   user	0m0.620s
   sys	0m0.310s

Times are not much shorter if the database was just vacuumed before. I guess this depends a lot on the hardware involved (mine was with a Core2Duo laptop with a ST9250320AS disk on Linux).

Only writing 86 MB to disk in 140ms corresponds to a transfer rate of 614 MB/s which seems a bit high.

Still, to me even a delay of a few seconds could be acceptable when idle.
yeah i'm still getting measurements since numbers were strange to me, but ideally even 2s would not be bad. Today i'll try to get more consistent numbers.

vacuuming on daily idle does not mean we will do that every day, i'd like to get some guess if the db needs to be vacuumed or not, and check that daily.
(In reply to comment #2)
> Times are not much shorter if the database was just vacuumed before.

This isexactly one of the things i was interested in.
So effectively i've found the error in my test, real time took is about 28s for an 86MB db. Effectively this changes things a bit. But probably doing it in idle and async won't hurt badly. Just we need a good strategy to detect if a db needs to be vacuumed.
VACUUM with TRUNCATE JOURNAL 37s 
VACUUM with MEMORY JOURNAL 22s
VACUUM with OFF JOURNAL 21s
VACUUM with MEMORY JOURNAL AND SYNC OFF 17s

the last one isthe fasest, but it's just too risky, journal OFF does not sounds giving back an interesting gain. i'd go for MEMORY JOURNAL, we tested this "cheat" in 3.0->3.5 DB schema update, and worked quite well.
Attached patch patch v1.0 (obsolete) — Splinter Review
still needs test and cleanup, but looks like working.
Flags: in-testsuite?
OS: Windows XP → All
Hardware: x86 → All
Summary: VACUUM sqlite database on daily idle → VACUUM places.sqlite database on daily idle
Blocks: 489173
Attached patch patch v1.1 (obsolete) — Splinter Review
This should be reviewable.
Notice the three tests are practically identical, i had to split them because vacuum is async, so i cannot rely on the db status after a vacuum happens. Btw reviewing one test should be enough since others are copies with just changed test cases.
Attachment #397371 - Attachment is obsolete: true
Attachment #398366 - Flags: review?(sdwilsh)
Comment on attachment 398366 [details] [diff] [review]
patch v1.1

For more detailed review comments, please see http://reviews.visophyte.org/r/398366/

on file: toolkit/components/places/src/nsNavHistory.h line 101
> #define PLACES_INIT_COMPLETE_EVENT_TOPIC "places-init-complete"
> #define PLACES_DB_LOCKED_EVENT_TOPIC "places-database-locked"
> #define PLACES_AUTOCOMPLETE_FEEDBACK_UPDATED_TOPIC "places-autocomplete-feedback-updated"
> #define PLACES_VACUUM_STARTING_TOPIC "places-vacuum-starting"

we have two forms here - *_EVENT_TOPIC and *_TOPIC.  Pick one and make them
all use it please.


on file: toolkit/components/places/src/nsNavHistory.h line 435
>   /**
>    * Analyzes the database and VACUUM it, if needed.
>    */
>   nsresult DecayFrecency();
>   /**
>    * Decays frecency and inputhistory values.
>    */
>   nsresult VacuumDatabase();

These should really be NS_HIDDEN_(nsresult) (along with all of our other class
methods really).


on file: toolkit/components/places/src/nsNavHistory.cpp line 214
> // Percentage of free pages in the database to force a vacuum between
> // MAX_TIME_BEFORE_VACUUM and MIN_TIME_BEFORE_VACUUM.
> #define VACUUM_FREEPAGES_THRESHOLD 0.1

I think you mean fraction instead of percentage (.1% sounds trivially small)


on file: toolkit/components/places/src/nsNavHistory.cpp line 217
> // This is the maximum time that can pass between 2 VACUUM operations.
> #define MAX_TIME_BEFORE_VACUUM (PRInt64)60 * 24 * 60 * 60 * 1000 * 1000
> // This is the minimum time that should pass between 2 VACUUM operations.
> #define MIN_TIME_BEFORE_VACUUM (PRInt64)30 * 24 * 60 * 60 * 1000 * 1000

Can you specify the time units on this please?


on file: toolkit/components/places/src/nsNavHistory.cpp line 5625
>     } else if (NS_LITERAL_STRING(NS_PRIVATE_BROWSING_LEAVE).Equals(aData)) {

this bracing style is different from the code around it


on file: toolkit/components/places/src/nsNavHistory.cpp line 5644
> nsNavHistory::VacuumDatabase() {

nit: brace on newline like rest of file please


on file: toolkit/components/places/src/nsNavHistory.cpp line 5645
>   // Ideally we would like SQLite to tell us if the database is fragmented.
>   // But this is actually not supported, we could analyze the database file
>   // page by page, and count fragmented space, but that would be slow and not
>   // maintainable across different SQLite versions.
>   // For this reason we just take a guess using the freelist count.

I was talking to drh about this the other day.  There really is no way to know
if the database is fragmented.  The number that the sqlite analyzer gives is
just some arbitrary number, with no units.  He suggested that our method of
approach was good, but didn't see this being useful to take upstream since
every consumer would likely have different criteria.  Please update this
comment accordingly.


on file: toolkit/components/places/src/nsNavHistory.cpp line 5657
>   nsresult rv;
>   PRInt32 lastVacuumPref;

declare these at first use please, not before


on file: toolkit/components/places/src/nsNavHistory.cpp line 5659
>   nsCOMPtr<nsIPrefBranch> prefSvc =
>     do_GetService("@mozilla.org/preferences-service;1", &rv);
>   NS_ENSURE_SUCCESS(rv, NS_ERROR_OUT_OF_MEMORY);

Just do NS_ENSURE_TRUE(prefSvc, NS_ERROR_OUT_OF_MEMORY); and don't pass in rv
to do_GetService


on file: toolkit/components/places/src/nsNavHistory.cpp line 5663
>     // Value are seconds till epoch, convert it to us.

Please actually use mu in the source or spell it out.  I thought this was "us"
first, and not microseconds.


on file: toolkit/components/places/src/nsNavHistory.cpp line 5673
>     PRBool hasResult = PR_FALSE;
>     PRInt32 pageCount = 0;
>     PRInt32 freelistCount = 0;

I would prefer these to be declared at first use as well please.


on file: toolkit/components/places/src/nsNavHistory.cpp line 5678
>         "PRAGMA page_count"),

Why are you doubly indenting here?


on file: toolkit/components/places/src/nsNavHistory.cpp line 5686
>         "PRAGMA freelist_count"),

ditto on double indentation


on file: toolkit/components/places/src/nsNavHistory.cpp line 5712
>     nsCOMPtr<mozIStorageStatement> journalToMemory;
>     rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>       "PRAGMA journal_mode = MEMORY"),
>       getter_AddRefs(journalToMemory));
>     NS_ENSURE_SUCCESS(rv, rv);

Add a comment explaining why we are doing this please.


on file: toolkit/components/places/src/nsNavHistory.cpp line 5739
>     rv = prefSvc->SetIntPref(PREF_LAST_VACUUM,
>                              (PRInt32)(PR_Now() / PR_USEC_PER_SEC));
>     NS_ENSURE_SUCCESS(rv, rv);

Technically, the vacuum could fail and we'd still update the preference.  I'm
not sure we care too much about that case though.


on file: toolkit/components/places/src/nsNavHistory.cpp line 5748
> nsNavHistory::DecayFrecency() {

nit: brace on newline please


on file: toolkit/components/places/tests/unit/test_vacuum.js line 139
>     dump("PLACES TEST: " + test.desc + "\n");

nit: you should use print, which adds the newline for you

r=sdwilsh
Attachment #398366 - Flags: review?(sdwilsh) → review+
(In reply to comment #8)
> we have two forms here - *_EVENT_TOPIC and *_TOPIC.  Pick one and make them
> all use it please.

i choose *_TOPIC

> These should really be NS_HIDDEN_(nsresult) (along with all of our other class
> methods really).

fixing these, but this is not the right bug to fix all the others.
Can this cause any issue? should i file a followup?

> on file: toolkit/components/places/src/nsNavHistory.cpp line 5657
> >   nsresult rv;
> >   PRInt32 lastVacuumPref;
> 
> declare these at first use please, not before

i can't use all of them at first use, because the first uses are inside a different scope. But in some case i don't know why i did that, could be it was just late and i did not cleanup enough :\

> on file: toolkit/components/places/src/nsNavHistory.cpp line 5678
> >         "PRAGMA page_count"),
> 
> Why are you doubly indenting here?

We always did that, it's the common code style in places when we define a statement with NS_LITERAL_CSTRING, because we open 2 braces:

   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
        "PRAGMA journal_mode = MEMORY"),
      getter_AddRefs(journalToMemory));

I'm just staying consistent with our code.

> on file: toolkit/components/places/src/nsNavHistory.cpp line 5739
> >     rv = prefSvc->SetIntPref(PREF_LAST_VACUUM,
> >                              (PRInt32)(PR_Now() / PR_USEC_PER_SEC));
> >     NS_ENSURE_SUCCESS(rv, rv);
> 
> Technically, the vacuum could fail and we'd still update the preference.  I'm
> not sure we care too much about that case though.

I'm not sure if we want to complicate the code to take in count this case. it's quite uncommon that it could fail, and won't bring much, if we won't vacuum this month we will the next one. If the time between vacuum is too high we can just reduce it. I'm not adding this error check for now, if you don't have anything against it.

fixed remaining comments.
Attached patch patch v1.2 (obsolete) — Splinter Review
Attachment #398366 - Attachment is obsolete: true
Summary: VACUUM places.sqlite database on daily idle → VACUUM places.sqlite database on daily idle once a month
(In reply to comment #9)
> fixing these, but this is not the right bug to fix all the others.
> Can this cause any issue? should i file a followup?
Just exposes more symbols in the library.  It's not a big deal, but we should fix these eventually.

> We always did that, it's the common code style in places when we define a
> statement with NS_LITERAL_CSTRING, because we open 2 braces:
Then you are inconsistent in your own patch (and we aren't consistent in nsNavHistory or our other files for that matter).  I'd prefer to not have the double indentation myself.
Attached patch patch v1.3Splinter Review
just be consistent! right, i missed those because was only a code move :\
Attachment #400056 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/18a6c37bb588
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Flags: in-testsuite? → in-testsuite+
Comment on attachment 400156 [details] [diff] [review]
patch v1.3

We would like to see this in FX3.6 after some baking on trunk.
Attachment #400156 - Flags: approval1.9.2?
Comment on attachment 400156 [details] [diff] [review]
patch v1.3

i had to disable the 3 tests for now, because the sparse dbs generator is randomly failing (sometimes it does not generate a fragmented enough db), i'll have to provide pre-built databases and a new roll-up patch.

http://hg.mozilla.org/mozilla-central/rev/a05d4b372d3a
Attachment #400156 - Flags: approval1.9.2?
Attachment #400525 - Flags: review?(sdwilsh)
Are wwe going to get any kind of documentation on this?  The bug title says daily idle and also once a month.  Will the VACUUM happen once a day or once a month on idle?  What exactly is idle?  What if I am streaming video?
once a month if the db is heavy used, once every 2 months otherwise, on daily idle we just check if we should vacuum. Idle is the idle returned by the idle service https://developer.mozilla.org/en/nsIIdleService "The idle service lets you monitor how long the user has been 'idle', i.e. not used their mouse or keyboard."

It is possible we hit during a movie, but once every 2 months should not be an hard issue, against having a faster browser.
Do we want this on 1.9.2 as well? If so, we should get that in for beta, which means before Marco leaves, which means today :)
Fine for me, but i need at least approval.
Attachment #400156 - Flags: approval1.9.2?
Comment on attachment 400156 [details] [diff] [review]
patch v1.3

Tests should be disabled until new ones are reviewed
Comment on attachment 400156 [details] [diff] [review]
patch v1.3

a192=beltzner with the understanding that this bug won't be marked fixed until the tests come through
Attachment #400156 - Flags: approval1.9.2? → approval1.9.2+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7b4f35e3a442
Whiteboard: [tests faling on tbox (pass locally)][new fixed tests need review sdwilsh]
Attachment #400525 - Flags: review?(sdwilsh) → review+
Comment on attachment 400525 [details] [diff] [review]
fix tests, use a pre-built db

r=sdwilsh
tests on trunk
http://hg.mozilla.org/mozilla-central/rev/01fda800b5f6
Whiteboard: [tests faling on tbox (pass locally)][new fixed tests need review sdwilsh] → [checkin-needed for tests on 1.9.2]
tests on 1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b70d0aff4d85
Whiteboard: [checkin-needed for tests on 1.9.2]
I hate to spam a dead bug, but the patch mentioned doesn't seem to include actually reindexing the database -- just vacuum. Would it be possible to add a reindex step as well?
Indeed this is not the right place for a similar question, btw why should we reindex?
You need to log in before you can comment on or make changes to this bug.