Closed Bug 1142269 Opened 5 years ago Closed 5 years ago

Vacuum the cache database

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
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.
Attachment #8576273 - Flags: review?(bkelly)
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.
Attachment #8576273 - Flags: review?(bkelly) → review-
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.
Status: NEW → ASSIGNED
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?
Status: ASSIGNED → NEW
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
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.
Attachment #8576273 - Attachment is obsolete: true
Attachment #8577245 - Flags: review?(bkelly)
Comment on attachment 8577245 [details] [diff] [review]
Auto-vacuum the DOM Cache database

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

Thank you!
Attachment #8577245 - Flags: review?(bkelly) → review+
Depends on: 1133763
Relanded as this is irrelevant:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc8ba757c59
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/bdc8ba757c59
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.