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.
Created attachment 162612 [details] Patch
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?
Comment on attachment 162612 [details] Patch Attachment 162612 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13786222
Comment on attachment 162612 [details] Patch Attachment 162612 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13778545
(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.
Created attachment 162806 [details] Patch
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?
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.
> > 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.
(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.
Created attachment 163397 [details] Patch
Comment on attachment 163397 [details] Patch tony@ - r? cq?
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?
Created attachment 163404 [details] Patch for landing
> 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.
Comment on attachment 163404 [details] Patch for landing Clearing flags on attachment: 163404 Committed r128217: <http://trac.webkit.org/changeset/128217>
All reviewed patches have been landed. Closing bug.