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 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.

(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 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.

Applied mak's review

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 on attachment 594167 [details] [diff] [review] Added a "Modified" column to webappsstore2_temp https://hg.mozilla.org/integration/mozilla-inbound/rev/2986d2923fa9

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 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 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 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.

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

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