I wrote a web app for collection and organization of notes. That web app is heavily using localStorage and the key property to read the list of keys and iterate their names. After bug 662511 has been fixed, load of my 300 notes takes almost a minute (before that it was just fraction of second). This affects the Aurora branch I recently started to use. Depends also on more feedback of web developers on localStorage.key performance but I would propose to re-cache keys only when the storage has been modified. We currently don't track that information and it could also complicate the e10s migration. If we agree to do that change, we should back the patch for bug 662511 out from the Aurora branch. If there is no negative feedback, let's close this as WONTFIX. Applications like my web app should migrate to structured database.

We must not ship major performance regressions. A testcase would be good here.

Based on: - each storage instance is bound to its own DOMStorageImpl ; there maybe more pairs also for just a single origin (scope) in case of localStorage - DOMStorageImpl keeps cache of data for its scope and communicates with the database through the wrapper singleton ...the patch does: - each DOMStorageImpl remembers the version of the data it has cached (MarkScopeCached) - on change in the scope (set/delete) the version is bumped up in the database class instance (mem and persist) (MarkScopeDirty) - when storage (DOMStorageImpl) is checking "dirtiness" of its cached data it just compares the cache version number has remembered previously with the current one in the database for its scope (IsScopeDirty) This approach works for infinite number of storage instances for a single origin and also works when switching between PB and session-only mode. Currently under try, tests that I know touch DOM storage locally passes: https://tbpl.mozilla.org/?tree=Try&rev=e492b0e35e0d

Comment on attachment 560934 [details] [diff] [review] v1 >+ // 0 initially or a positive data version number assgined by gStorageDB s/assgined/assigned/ I assume the == 0 check in NextGlobalVersion is meant to deal with overflow? I'd prefer we make the version number 64-bit so we can worry less about overflow. >+ // We may do this becuase the storage updates its cache along with s/becuase/because/ It looks like this is written on top of the first patch in bug 686049. Should I spin that out into a separate bug and land it for now so it's not blocked on the second patch? >+ * Marks the storage as "cached" after the DOMStorageImpl object has loaded >+ * all items to its memory copy of the entries - IsScopeDirty returns true >+ * after call of this method for this storage. IsScopeDirty returns _false_ after a call to MarkScopeCache, right? Please fix that. >+ * again as "dirty" and makes DOMStorageImpl object recache its keys >+ * on next access regardless the object's state of cache - IsScopeDirty >+ * then returns true again. again as "dirthy" and makes the DOMStorageImpl object recache its keys on next access, because IsScopeDirty returns true again. >+ * Test if the storage for the scope (i.e. origin or host) has not been >+ * changed since last caching. Test whether the storage for the scope (i.e. origin or host) has been changed since the last MarkScopeCached call. Why does MarkScopeCached return nsresult? Should it not return void? >+++ b/dom/src/storage/nsDOMStorageDBWrapper.h The comments here still mention mItemsCached, and should not. Just copy the ones from the new class? r=me with the above nits fixed.

(In reply to Boris Zbarsky (:bz) from comment #3) > Comment on attachment 560934 [details] [diff] [review] > v1 > I'd prefer we make the version number 64-bit so we can worry less about > overflow. I was thinking of it too. > It looks like this is written on top of the first patch in bug 686049. > Should I spin that out into a separate bug and land it for now so it's not > blocked on the second patch? Yes, I wanted to combine it with your patches. I think it is OK to land your patch now. It is r+'ed too. > IsScopeDirty returns _false_ after a call to MarkScopeCache, right? Please > fix that. Ah! Yes. > Why does MarkScopeCached return nsresult? Should it not return void? Then your new macro won't work :( It expects methods return something. Maybe have a 'void' variant of it or a modifier via an argument? > r=me with the above nits fixed. Thanks for the quick review.

> I think it is OK to land your patch now. OK, I'll do that today. > Maybe have a 'void' variant of it Yes, please.

Ready to land (waits for bug 686049 to land).

This is a simple back out of bug 662511 from Aurora branch. I think the real fix is too complicated for Aurora, but please Johny, as a driver, express your opinion on this and potentially directly approve the reviewed "v1" mozilla-central patch for Aurora.

Updated according to bug 687755 comment 5.

Comment on attachment 561249 [details] [diff] [review] central v1.2 https://hg.mozilla.org/integration/mozilla-inbound/rev/f94b4d73777f

Leaving open, we yet need to figure the Aurora state.

That should be tracked via the branch flags, no? This bug should be resolved once it lands on m-c (next inbound merge). Luckily, the merge viking will do that. ;)

Comment on attachment 561249 [details] [diff] [review] central v1.2 Backed out for failing to build on all platforms: { ../../../../dom/src/storage/nsDOMStorage.cpp:1076:3: error: 'mItemsCached' was not declared in this scope make[7]: *** [nsDOMStorage.o] Error 1 } https://hg.mozilla.org/integration/mozilla-inbound/rev/1e41259daf67

Oh hell.. I messed up my queue apparently. Sorry for that.

Comment on attachment 561160 [details] [diff] [review] aurora backout v1 [landed comment 16] I think we should take this backout for aurora, a=drivers per todays driver meeting.

Comment on attachment 561160 [details] [diff] [review] aurora backout v1 [landed comment 16] https://hg.mozilla.org/releases/mozilla-aurora/rev/de96bbe87419

Can anyone please give an example of a web app that this bug could be reproduced with?

(In reply to Ioana Budnar [QA] from comment #18) > Can anyone please give an example of a web app that this bug could be > reproduced with? http://scrip.it/

Verified as fixed on: Mozilla/5.0 (X11; Linux i686; rv:8.0) Gecko/20100101 Firefox/8.0 Mozilla/5.0 (X11; Linux i686; rv:9.0a2) Gecko/20111007 Firefox/9.0a2 Mozilla/5.0 (X11; Linux i686; rv:10.0a1) Gecko/20111007 Firefox/10.0a1 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a2) Gecko/20111006 Firefox/9.0a2 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a1) Gecko/20111006 Firefox/10.0a1 Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0 Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111004 Firefox/9.0a2 Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111004 Firefox/10.0a1 Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0 Mozilla/5.0 (Windows NT 5.1; rv:9.0a2) Gecko/20111002 Firefox/9.0a2 Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111003 Firefox/10.0a1 I created over 350 notes using http://scrip.it/. They loaded in all the Fx versions in at most 3 seconds. 90% of the time they loaded in less than a second.