The default bug view has changed. See this FAQ.

DOMStorageImpl::GetKey performance regression

VERIFIED FIXED in Firefox 8

Status

()

Core
DOM
--
major
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

({perf})

8 Branch
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox8+ fixed, firefox9+)

Details

(Whiteboard: [qa!])

Attachments

(2 attachments, 3 obsolete attachments)

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.
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?

Updated

6 years ago
tracking-firefox8: ? → +
tracking-firefox9: ? → +
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
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #560934 - Flags: review?(bzbarsky)
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.
Attachment #560934 - Flags: review?(bzbarsky) → review+
(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.
Created attachment 560991 [details] [diff] [review]
v1.1

Ready to land (waits for bug 686049 to land).
Attachment #560934 - Attachment is obsolete: true
Attachment #560991 - Flags: review+
Depends on: 687755
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.
Attachment #561160 - Flags: review?(jst)
Attachment #561160 - Flags: approval-mozilla-aurora?
Created attachment 561249 [details] [diff] [review]
central v1.2

Updated according to bug 687755 comment 5.
Attachment #560991 - Attachment is obsolete: true
Attachment #561249 - Flags: review+
Comment on attachment 561249 [details] [diff] [review]
central v1.2

https://hg.mozilla.org/integration/mozilla-inbound/rev/f94b4d73777f
Attachment #561249 - Attachment description: central v1.2 → central v1.2 [Landed comment 9]
Attachment #561249 - Flags: checkin+
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
Attachment #561249 - Attachment description: central v1.2 [Landed comment 9] → central v1.2
Attachment #561249 - Flags: review-
Attachment #561249 - Flags: review+
Attachment #561249 - Flags: checkin-
Attachment #561249 - Flags: checkin+
Oh hell.. I messed up my queue apparently.  Sorry for that.

Updated

6 years ago
Attachment #561160 - Flags: review?(jst) → review+
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.
Attachment #561160 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Created attachment 562005 [details] [diff] [review]
central v1.3 [as landed comment 14]

https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/e38b3957d001
Attachment #561249 - Attachment is obsolete: true
Attachment #562005 - Flags: review+
Attachment #562005 - Flags: checkin+
Comment on attachment 561160 [details] [diff] [review]
aurora backout v1 [landed comment 16]

https://hg.mozilla.org/releases/mozilla-aurora/rev/de96bbe87419
Attachment #561160 - Attachment description: aurora backout v1 → aurora backout v1 [landed comment 16]
Attachment #561160 - Flags: checkin+

Updated

6 years ago
status-firefox8: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/e38b3957d001
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Comment 18

6 years ago
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/

Comment 20

6 years ago
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa!]

Updated

5 years ago
Depends on: 715700
You need to log in before you can comment on or make changes to this bug.