Bug 109972 – IDB Cursor continue moves one item at a time

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

IDB Cursor continue moves one item at a time

Created attachment 188643 [details]
Patch

[Alec Flett]

Comment on attachment 188643 [details]
Patch

lgtm - thanks!

I don't think we have any particular perf tests for this unfortunately.

[Joshua Bell]

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.

[Joshua Bell]

Created attachment 192767 [details]
Patch

[Joshua Bell]

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?

[David Grogan]

Comment on attachment 192767 [details]
Patch

lgtm

[Joshua Bell]

Chromium-side performance test posted to: https://codereview.chromium.org/12780007/

[Joshua Bell]

tony@ - r?

I won't land this until the Chromium perf test is in and has some data collected, though.

[Tony Chang]

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.

[Joshua Bell]

Created attachment 193665 [details]
Patch for landing

[WebKit Review Bot]

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

[Joshua Bell]

Created attachment 193687 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 193687 [details]
Patch for landing

Clearing flags on attachment: 193687

Committed r146152: <http://trac.webkit.org/changeset/146152>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.