714964 - Reduce writes in current DOM Storage implementation

Status: RESOLVED FIXED

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

Description

[Marco Bonardo [:mak]]

This bug is to investigate a temporary workaround while I finish looking into the more comprehensive bug 712009.
May be built on top of bug 711972, bug 711970 and bug 712006, just to avoid bitrotting, plus those provide some nice code cleanup.

Currently we copy all data for a scope to the temp table, and then we flush back it to disk, even if it has not been modified.

The idea is not so fancy, the temp table may have an indicator column for "modified", and when flushing down data we may flush only modified rows. Adding a column to temp means we should never use SELECT * since the 2 partitioned tables would have different columns, but that's a good thing, after all.

Comment 1

[Vladan Djeric (:vladan)]

Attachment: Added a "Modified" column to webappsstore_temp

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 593882 [details] [diff] [review]
Added a "Modified" column to webappsstore_temp

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

::: dom/src/storage/nsDOMStoragePersistentDB.cpp
@@ +345,5 @@
>      nsCOMPtr<mozIStorageStatement> stmt =
>        data->mDB->mStatements.GetCachedStatement(
>          "INSERT OR REPLACE INTO webappsstore2 "
> +         "SELECT scope, key, value, secure, owner FROM webappsstore2_temp "
> +         "WHERE scope = :scope AND modified = 1"

You may want to also create another index for (scope, modified) on the temp table to speed this up, but that would need some benchmark since the index may slow inserts down a bit.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> You may want to also create another index for (scope, modified) on the temp
> table to speed this up, but that would need some benchmark since the index
> may slow inserts down a bit.

I don't think it's worth it. Sure it would speed up the search but scope restricts the resultset at a point that likely a simple scan is cheaper than making a btree for the additional index. Plus as you said would slowdown inserts. I'd not complicate the thing for now, we should evaluate further improvements in future but this one is alread a great IO win with really simple changes.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 593882 [details] [diff] [review]
Added a "Modified" column to webappsstore_temp

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

This looks absolutely fine to me. Honza may do the final review.

::: dom/src/storage/nsDOMStoragePersistentDB.cpp
@@ +188,5 @@
>           "key TEXT, "
>           "value TEXT, "
>           "secure INTEGER, "
> +         "owner TEXT, "
> +         "modified INTEGER)"));

you may define it as a DEFAULT 0 column, so you don't actually have to explicitly set it to 0 on insert.

@@ +204,2 @@
>          "UNION ALL "
>          "SELECT * FROM webappsstore2 "

just for sanity, I'd prefer if you'd also specify the columns here. It happened in the past in Places that we forgot behind a SELECT * and changing the schema broke downgrades.
Using SELECT * is generally an error-prone idea with partitioned tables, personally I'd prefer if we'd remove all of them from this file, though I'll for sure want this one.

Comment 5

[Vladan Djeric (:vladan)]

Attachment: Added a "Modified" column to webappsstore2_temp

Applied mak's review

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 594167 [details] [diff] [review]
Added a "Modified" column to webappsstore2_temp

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

Thanks

r=honzab

Comment 7

[Vladan Djeric (:vladan)]

Comment on attachment 594167 [details] [diff] [review]
Added a "Modified" column to webappsstore2_temp

https://hg.mozilla.org/integration/mozilla-inbound/rev/2986d2923fa9

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2986d2923fa9

Comment 9

[Vladan Djeric (:vladan)]

Comment on attachment 594167 [details] [diff] [review]
Added a "Modified" column to webappsstore2_temp

[Approval Request Comment]
Regression caused by (bug #): None
User impact if declined: This is a straightforward fix for a major longstanding UI freeze
Testing completed (on m-c, etc.): This patch has been on m-c for about a week, tested manually and on TBPL
Risk to taking this patch (and alternatives if risky): If there are bugs in this patch, users' sessionStorage store could become corrupted or sessionStorage data could be lost. This patch has been on trunk for about a week.
String changes made by this patch: None

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 594167 [details] [diff] [review]
Added a "Modified" column to webappsstore2_temp

[Triage Comment]
Fixes a longstanding freeze and we're early enough in the cycle to approve for Aurora 12. We expect that any fallout will become apparent quickly enough to back this change out prior to release if necessary. This doesn't pass the bar for beta, however.

Comment 11

[(dormant account)]

Comment on attachment 594167 [details] [diff] [review]
Added a "Modified" column to webappsstore2_temp

(In reply to Alex Keybl [:akeybl] from comment #10)
> Comment on attachment 594167 [details] [diff] [review]
> Added a "Modified" column to webappsstore2_temp
> 
> [Triage Comment]
> Fixes a longstanding freeze and we're early enough in the cycle to approve
> for Aurora 12. We expect that any fallout will become apparent quickly
> enough to back this change out prior to release if necessary. This doesn't
> pass the bar for beta, however.

This has baked on trunk for > 2 weeks with zero issues. Prior to this fix firefox had extremely poor performance on top websites compared to the competition. Not taking this fix could cost us some market share.

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 594167 [details] [diff] [review]
Added a "Modified" column to webappsstore2_temp

[Triage Comment]
Apologies, but this doesn't meet the requirements of landing for our sixth beta. We're only taking sg:crit bugs, chemspill-worthy issues, and backouts to known states for sake of risk mitigation.

Comment 13

[Justin Wood (:Callek)]

Can someone please either unbitrot this patch or verify that it did indeed land on aurora (with cset and setting status-Firefox-12:fixed)

Bitrot just now for me trying to land this:

$ hg qpush
applying modifiedBit2.patch
patching file dom/src/storage/nsDOMStoragePersistentDB.cpp
Hunk #1 FAILED at 182
Hunk #2 FAILED at 296
Hunk #3 FAILED at 338
Hunk #4 FAILED at 533
Hunk #5 FAILED at 582
5 out of 5 hunks FAILED -- saving rejects to file dom/src/storage/nsDOMStoragePersistentDB.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh modifiedBit2.patch

Comment 14

[Marco Bonardo [:mak]]

this indeed landed
https://hg.mozilla.org/releases/mozilla-aurora/rev/2211daf0acc7