1142269 - Vacuum the cache database

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[(no longer active)]

Attachment: Auto-vacuum the DOM Cache database

It's easiest if we use auto_vaccum for now.  In the future if this
proves to be insufficient we can look into more sophisticated
vacuuming strategies.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8576273 [details] [diff] [review]
Auto-vacuum the DOM Cache database

Review of attachment 8576273 [details] [diff] [review]:
-----------------------------------------------------------------

So, the auto_vacuum = INCREMENTAL still needs a separate command invoked later.  See my comments below.

I think, though, we should instead just do a full VACUUM in DBSchema::CreateSchema() if the schema is already present.  Once bug 1110487 lands we will only be doing this once per browser session when the origin first accesses Cache.

This starts us at the most safe and conservative point, although not optimally fast.  We can optimize to a "only VACUUM if DB modified" or using auto_vacuum later if we need to based on measurements.

I'm sorry I didn't come to this conclusion earlier.  I know I suggested auto_vacuum to you when we spoke.

::: dom/cache/DBSchema.cpp
@@ +42,5 @@
>  #endif
>      "PRAGMA foreign_keys = ON; "
> +    // Enable auto-vaccum but in incremental mode in order to avoid doing a lot
> +    // of work at the end of each transaction.
> +    "PRAGMA auto_vacuum = INCREMENTAL; "

If we are going to use this value then we must invoke the incremental_vacuum pragma to actually trigger the event:

  http://sqlite.org/pragma.html#pragma_auto_vacuum
  http://sqlite.org/pragma.html#pragma_incremental_vacuum

We could do this in SetupAction::RunSyncWithDBOnTarget() or in DBSchema::CreateSchema() if the database already exists.  Once the nsIOfflineStorage changes land we will not be running CreateSchema() very often.  So this would be a one time hit the first time you access.

However, if we are going to do that, then I think we should just do a vacuum instead.  Lets be conservative and optimize to auto_vacuum or another scheme if the vacuum-once-at-startup is too expensive.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Err... or not.  With bug 1110487, we will still trigger the DBSchema::CreateSchema() call again in the same browser session if the origin window is closed and a new window is opened later.

So I guess just leave auto_vacuum = INCREMENTAL in there and then trigger an incremenal_vacuum in CreateSchema() instead.

Comment 4

[(no longer active)]

I disagree.  Delaying the opening of the database even once per browser session is absolutely the worst time to do this work.  Note that if the database is big, a vacuum can take a *long* time, and for apps that use a SW in order to cache their offline resource, this will probably amount to a huge and noticeable delay when loading the page the first time in the browser session.

Thinking more about this, I think the best course of action for now is to do nothing.  We still don't have any evidence on how much space we can reclaim by vacumming the database in real work loads, and those real work loads don't even exist yet!  Instead of guessing and potentially paying a significant cost up front for potentially questionable gains, I'd prefer to hold back and see how much of a problem this will be in practice.

Also, looking at the rest of our code, it seems like the only databases that we occasionally vaccum are:

* Places, using the mozIStorageVacuumParticipant API.
* Form history database.
* FHR database.
* IndexedDB on Android and B2G only.

It seems like we have gotten away without needing to vaccum in any other database that we maintain.

What do you think?

Comment 5

[(no longer active)]

We discussed this on IRC, and neither of us changed our minds.  I'm going to do what Ben asked me to do but we should revisit once we have real websites using this stuff by measuring on real world data how much space vaccums save and how long incremental auto vaccums take in SetupAction.  That is bug 1142983.

Comment 6

[(no longer active)]

Attachment: Auto-vacuum the DOM Cache database

It's easiest if we use auto_vaccum for now.  In the future if this
proves to be insufficient we can look into more sophisticated
vacuuming strategies.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8577245 [details] [diff] [review]
Auto-vacuum the DOM Cache database

Review of attachment 8577245 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d2fd1bb7f5

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fd08e442bccd for making test_cache_match_request.html frequently fail.


https://treeherder.mozilla.org/logviewer.html#?job_id=7609806&repo=mozilla-inbound

Comment 10

[(no longer active)]

Relanded as this is irrelevant:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc8ba757c59

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bdc8ba757c59