Bug 109972 - IDB Cursor continue moves one item at a time
Summary: IDB Cursor continue moves one item at a time
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-15 14:19 PST by pankaj
Modified: 2013-03-18 16:56 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.17 KB, patch)
2013-02-15 14:24 PST, pankaj
no flags Details | Formatted Diff | Diff
Patch (7.56 KB, patch)
2013-03-12 10:55 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (7.33 KB, patch)
2013-03-18 15:08 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (7.13 KB, patch)
2013-03-18 16:34 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description pankaj 2013-02-15 14:19:33 PST
IDB Cursor continue moves one item at a time
Comment 1 pankaj 2013-02-15 14:24:38 PST
Created attachment 188643 [details]
Patch
Comment 2 Alec Flett 2013-02-15 14:49:45 PST
Comment on attachment 188643 [details]
Patch

lgtm - thanks!

I don't think we have any particular perf tests for this unfortunately.
Comment 3 Joshua Bell 2013-02-19 12:54:41 PST
Comment on attachment 188643 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        optimization.

Include your performance numbers, even if it's a manual A/B test. I think you had a case where the cost of ~700 continues went from ~500ms to ~80ms ?

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:1273
> +    bool firstIteration = true;

It looks like a follow-up patch could potentially simplify firstSeek()/IteratorState/firstIteration a bit, but I think this is good for now.

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:1284
> +            if (firstIteration && key && forward) {

Maybe add FIXME comment to optimize key seeking for reverse cursors?

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:1414
> +        return ObjectStoreDataKey::encode(

No line wrap here.

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:1476
> +        return ObjectStoreDataKey::encode(

No line wrap here.

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:1546
> +        return IndexDataKey::encode(

No line wrap here.

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:1638
> +        return IndexDataKey::encode(

No line wrap here.
Comment 4 Joshua Bell 2013-03-12 10:55:06 PDT
Created attachment 192767 [details]
Patch
Comment 5 Joshua Bell 2013-03-12 10:58:03 PDT
Tweaked the patch based on feedback. Added perf numbers to CL.

Note that I wasn't able to replicate quite as dramatic a speedup as Pankaj communicated in email. With a basic test in a release build in Chrome - over an index w/ 100 items, open a cursor, then seek to item 50 - I saw "only" a 2x speedup.

dgrogan@, alecflett@ - one last look?
Comment 6 David Grogan 2013-03-12 15:32:03 PDT
Comment on attachment 192767 [details]
Patch

lgtm
Comment 7 Joshua Bell 2013-03-15 12:49:05 PDT
Chromium-side performance test posted to: https://codereview.chromium.org/12780007/
Comment 8 Joshua Bell 2013-03-15 12:49:32 PDT
tony@ - r?

I won't land this until the Chromium perf test is in and has some data collected, though.
Comment 9 Tony Chang 2013-03-15 13:22:21 PDT
Comment on attachment 192767 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:1325
> +            } else {
> +                if (forward)
> +                    m_iterator->next();
> +                else
> +                    m_iterator->prev();
> +            }

Nit: You could merge the condition into an "else if/else" with the outer if.
Comment 10 Joshua Bell 2013-03-18 15:08:21 PDT
Created attachment 193665 [details]
Patch for landing
Comment 11 WebKit Review Bot 2013-03-18 15:47:59 PDT
Comment on attachment 193665 [details]
Patch for landing

Rejecting attachment 193665 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 193665, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
ebkit-commit-queue/Source/WebKit/chromium/third_party/ots --revision 97 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
43>At revision 97.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://webkit-commit-queue.appspot.com/results/17209397
Comment 12 Joshua Bell 2013-03-18 16:34:39 PDT
Created attachment 193687 [details]
Patch for landing
Comment 13 WebKit Review Bot 2013-03-18 16:56:26 PDT
Comment on attachment 193687 [details]
Patch for landing

Clearing flags on attachment: 193687

Committed r146152: <http://trac.webkit.org/changeset/146152>
Comment 14 WebKit Review Bot 2013-03-18 16:56:30 PDT
All reviewed patches have been landed.  Closing bug.