Last Comment Bug 683316 - DOMStorageImpl::GetKey performance regression
: DOMStorageImpl::GetKey performance regression
Status: VERIFIED FIXED
[qa!]
: perf
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 8 Branch
: All All
: -- major (vote)
: mozilla9
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on: 687755 715700
Blocks: 662511
  Show dependency treegraph
 
Reported: 2011-08-30 14:17 PDT by Honza Bambas (:mayhemer)
Modified: 2012-01-06 09:10 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+


Attachments
v1 (21.90 KB, patch)
2011-09-19 09:13 PDT, Honza Bambas (:mayhemer)
bzbarsky: review+
Details | Diff | Review
v1.1 (23.38 KB, patch)
2011-09-19 13:02 PDT, Honza Bambas (:mayhemer)
honzab.moz: review+
Details | Diff | Review
aurora backout v1 [landed comment 16] (4.67 KB, patch)
2011-09-20 04:03 PDT, Honza Bambas (:mayhemer)
jst: review+
jst: approval‑mozilla‑aurora+
honzab.moz: checkin+
Details | Diff | Review
central v1.2 (25.09 KB, patch)
2011-09-20 11:20 PDT, Honza Bambas (:mayhemer)
emorley: review-
emorley: checkin-
Details | Diff | Review
central v1.3 [as landed comment 14] (25.57 KB, patch)
2011-09-23 03:16 PDT, Honza Bambas (:mayhemer)
honzab.moz: review+
honzab.moz: checkin+
Details | Diff | Review

Description Honza Bambas (:mayhemer) 2011-08-30 14:17:59 PDT
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.
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-31 02:35:08 PDT
We must not ship major performance regressions.

A testcase would be good here.
Comment 2 Honza Bambas (:mayhemer) 2011-09-19 09:13:32 PDT
Created attachment 560934 [details] [diff] [review]
v1

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 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-19 10:17:17 PDT
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.
Comment 4 Honza Bambas (:mayhemer) 2011-09-19 12:18:32 PDT
(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.
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-19 12:21:34 PDT
> 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.
Comment 6 Honza Bambas (:mayhemer) 2011-09-19 13:02:48 PDT
Created attachment 560991 [details] [diff] [review]
v1.1

Ready to land (waits for bug 686049 to land).
Comment 7 Honza Bambas (:mayhemer) 2011-09-20 04:03:28 PDT
Created attachment 561160 [details] [diff] [review]
aurora backout v1 [landed comment 16]

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.
Comment 8 Honza Bambas (:mayhemer) 2011-09-20 11:20:24 PDT
Created attachment 561249 [details] [diff] [review]
central v1.2

Updated according to bug 687755 comment 5.
Comment 9 Honza Bambas (:mayhemer) 2011-09-20 11:46:11 PDT
Comment on attachment 561249 [details] [diff] [review]
central v1.2

https://hg.mozilla.org/integration/mozilla-inbound/rev/f94b4d73777f
Comment 10 Honza Bambas (:mayhemer) 2011-09-20 11:47:02 PDT
Leaving open, we yet need to figure the Aurora state.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-20 12:01:12 PDT
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 12 Ed Morley [:emorley] 2011-09-20 12:19:16 PDT
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
Comment 13 Honza Bambas (:mayhemer) 2011-09-20 12:23:51 PDT
Oh hell.. I messed up my queue apparently.  Sorry for that.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2011-09-22 14:20:36 PDT
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 15 Honza Bambas (:mayhemer) 2011-09-23 03:16:42 PDT
Created attachment 562005 [details] [diff] [review]
central v1.3 [as landed comment 14]

https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/e38b3957d001
Comment 16 Honza Bambas (:mayhemer) 2011-09-23 09:13:14 PDT
Comment on attachment 561160 [details] [diff] [review]
aurora backout v1 [landed comment 16]

https://hg.mozilla.org/releases/mozilla-aurora/rev/de96bbe87419
Comment 17 Ed Morley [:emorley] 2011-09-23 20:57:45 PDT
https://hg.mozilla.org/mozilla-central/rev/e38b3957d001
Comment 18 Ioana (away) 2011-10-05 06:03:37 PDT
Can anyone please give an example of a web app that this bug could be reproduced with?
Comment 19 Honza Bambas (:mayhemer) 2011-10-05 14:10:28 PDT
(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/
Comment 20 Ioana (away) 2011-10-07 07:31:44 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.