Bug 96033 – IndexedDB: IDBObjectStore.count() is slow

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

Over in http://crbug.com/146595 it is reported that IDBObjectStore.count() is slow; the original reporter claims exponential slowdowns, but I can't repro that.

Code inspection does show a lot of extraneous work being done which could be optimized, even a full iteration is still required.

[Joshua Bell]

Created attachment 162612 [details]
Patch

[David Grogan]

Comment on attachment 162612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162612&action=review

> Source/WebCore/ChangeLog:15
> +        (WebCore): Factor out common CursorOptions initialization code.

Looks like indexCursorOptions is in the same vein as objectStoreCursorOptions but not necessary for this patch, is that right?  (Just so that I understand what's going on - I'm not saying punt it.)  What do the return values of those methods mean?  That cursor with those options will actually have entries to iterate over?

[Peter Beverloo (cr-android ews)]

Comment on attachment 162612 [details]
Patch

Attachment 162612 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13786222

[WebKit Review Bot]

Comment on attachment 162612 [details]
Patch

Attachment 162612 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13778545

[Joshua Bell]

(In reply to comment #2)
> Looks like indexCursorOptions is in the same vein as objectStoreCursorOptions but not necessary for this patch, is that right?  (Just so that I understand what's going on - I'm not saying punt it.) 

Correct - when refactoring the objectstore options I realized the index options generation was duplicated code.

> What do the return values of those methods mean?  That cursor with those options will actually have entries to iterate over?

Correct - if false, no cursor is returned from the openXXXCursor methods (just as today) and the caller is responsible for e.g. sending null back in the eventual onSuccess. Should be no behavior change.

(In reply to comment #3)
> Attachment 162612 [details] did not pass cr-android-ews (chromium-android):

Derp, forgot about the webkit unit tests. Will re-up.

[Joshua Bell]

Created attachment 162806 [details]
Patch

[Joshua Bell]

I did some performance runs with this patch and also http://webkit.org/b/96037 which also improves performance for count().

Details at: http://code.google.com/p/chromium/issues/detail?id=146595

Pretty graph at: https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AoyP0VKkrFm8dE1VanBfR2Jpa1MwUkxCM1RsZ0RZTnc#gid=1

Numbers indicate it makes sense to land both patches.

dgrogan@ - thoughts? how's the patch look?

[David Grogan]

Comment on attachment 162806 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162806&action=review

LGTM

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1329
> +    virtual int64_t indexDataId() { ASSERT_NOT_REACHED(); return 0; }

Do you know why we have this method? It looks to not be called or implemented anywhere.

[Joshua Bell]

> > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1329
> > +    virtual int64_t indexDataId() { ASSERT_NOT_REACHED(); return 0; }
> 
> Do you know why we have this method? It looks to not be called or implemented anywhere.

Looks like it went in with http://trac.webkit.org/changeset/80220 - I don't see it being used directly, but there's a similar objectStoreDataId which was used for the SQLite backing store. It was probably held the autoincrement primary key ID for the index record in the SQL impl, and is dead code now.

I'll whack it in a follow-up patch.

[Joshua Bell]

 (In reply to comment #9)
> I'll whack it in a follow-up patch.

Scratch that - I will re-up this patch with it removed here and elsewhere.

[Joshua Bell]

Created attachment 163397 [details]
Patch

[Joshua Bell]

Comment on attachment 163397 [details]
Patch

tony@ - r? cq?

[Tony Chang]

Comment on attachment 163397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163397&action=review

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1358
> +    const char* p = m_iterator->key().begin();

Nit: Can you pick better variable names than p and q? Maybe key and value or currentKey and currentValue?

[Joshua Bell]

Created attachment 163404 [details]
Patch for landing

[Joshua Bell]

> Nit: Can you pick better variable names than p and q? Maybe key and value or currentKey and currentValue?

Done. The loadCurrentRow method was a copy/paste, so I updated them in the new method and the other variants.

There's already https://bugs.webkit.org/show_bug.cgi?id=85293 tracking the need to refactor the loadCurrentRow methods to eliminate duplicate code.

[WebKit Review Bot]

Comment on attachment 163404 [details]
Patch for landing

Clearing flags on attachment: 163404

Committed r128217: <http://trac.webkit.org/changeset/128217>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.