IndexedDB: Cursor pre-fetching.

IndexedDB: Cursor pre-fetching.

Add support for cursor pre-fetching.

BUG=88651
TEST=browser_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112675

[hans, 2011-11-23 18:16:59]

This is not ready for landing, just uploading to get some early feedback. Please
take a look.

WebKit side is here: https://bugs.webkit.org/show_bug.cgi?id=73025
It will need to be broken up somehow so we can land it two-sided without
breaking stuff.

The secret sauce is mainly in RendererWebIDBCursorImpl.

I have a few more test cases I want to add.

There are a couple of lines running longer than 80 characters, etc. I will fix
those.

[hans, 2011-11-24 15:19:17]

This is ready for review now. The WebKit side has to go in first, then this side
can be landed.

Please take a look.

Darin: since you'll look at the WebKit side anyway, maybe you can look here too
since you're in the OWNERS files of content/common and content/renderer?

[jsbell, 2011-11-28 20:29:16]

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
File chrome/test/data/indexeddb/common.js (right):

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
chrome/test/data/indexeddb/common.js:9: span.innerHTML = message + '<br />';
We should go the rest of the way and avoid using innerHTML to avoid escaping
problems e.g. if someone writes debug("this<that and that>this"); make this:

var span = document.createElement("span");
span.appendChild(document.createTextNode(message));
span.appendChild(document.createElement("br"));
document.getElementById('status').appendChild(span);

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
File chrome/test/data/indexeddb/cursor_prefetch.js (right):

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
chrome/test/data/indexeddb/cursor_prefetch.js:5: var kPrefetchThreshold = 2;
Include a comment that this must match the value in renderer_webidbcursor_impl.h
for the test to be valid? And maybe a comment in that file pointing back here?

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
chrome/test/data/indexeddb/cursor_prefetch.js:7: var kNumberOfItems = 100;
Since this is == kMaxPrefetchAmount in renderer_webidbcursor_impl.h these tests
never exercise maxing out the prefetch amount. Bump this to 200 or 300?

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
chrome/test/data/indexeddb/cursor_prefetch.js:10: request =
webkitIndexedDB.open('cursor-prefetch');
Can you add the indexedDB = indexedDB || webkitIndexedDB; preamble (and ditto
for IDBCursor, IDBTransaction, etc) to make the rest of the test more readable?

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
chrome/test/data/indexeddb/cursor_prefetch.js:355: 
Add a comment about what this test is for.

http://codereview.chromium.org/8662017/diff/5001/content/renderer/indexed_db_...
File content/renderer/indexed_db_dispatcher.cc (right):

http://codereview.chromium.org/8662017/diff/5001/content/renderer/indexed_db_...
content/renderer/indexed_db_dispatcher.cc:555: void
IndexedDBDispatcher::ResetCursorPrefetchCaches(int32 exception_cursor_id) {
Should this at least be scoped to database_id? Is there a need to invalidate
caches across all IDB instances in this renderer?

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
File content/renderer/renderer_webidbcursor_impl.cc (right):

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:86: prefetch_amount_ *= 2;
Is there any way to check that this hasn't grown too big for a single IPC
message? (Is that a valid concern?) My worry is that this will lead to seemingly
intermittent failures if e.g. 50 key/value pairs fit in an IPC message but 100
don't, which would be hard to debug. I'm thinking of crbug.com/96826 made much
worse

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:110:
pending_onsuccess_callbacks_--;
Add DCHECK that pending_onsuccess_callbacks_ >= 0?

[dgrogan, 2011-11-29 09:08:12]

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
File content/renderer/renderer_webidbcursor_impl.cc (right):

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:110:
pending_onsuccess_callbacks_--;
On 2011/11/28 20:29:16, jsbell wrote:
> Add DCHECK that pending_onsuccess_callbacks_ >= 0?

Can pending_onsuccess_callbacks_ ever be > 2?

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:114: //
pending_onsuccess_callacks_ would be incremented.
callbacks_ typo

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:167: // No prefetch cache, so no
need to reset the cursor in the back-end.
I might be showing ignorance here, but would this not also be triggered if
ResetPrefetchCache were called after javascript had called continue precisely
the same number of times as the number of cached elements?

[hans, 2011-11-29 14:44:34]

Thanks very much for the review! New version coming up...

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
File chrome/test/data/indexeddb/common.js (right):

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
chrome/test/data/indexeddb/common.js:9: span.innerHTML = message + '<br />';
On 2011/11/28 20:29:16, jsbell wrote:
> We should go the rest of the way and avoid using innerHTML to avoid escaping
> problems e.g. if someone writes debug("this<that and that>this"); make this:
> 
> var span = document.createElement("span");
> span.appendChild(document.createTextNode(message));
> span.appendChild(document.createElement("br"));
> document.getElementById('status').appendChild(span);

Done.

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
File chrome/test/data/indexeddb/cursor_prefetch.js (right):

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
chrome/test/data/indexeddb/cursor_prefetch.js:5: var kPrefetchThreshold = 2;
On 2011/11/28 20:29:16, jsbell wrote:
> Include a comment that this must match the value in
renderer_webidbcursor_impl.h
> for the test to be valid? And maybe a comment in that file pointing back here?

Done.

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
chrome/test/data/indexeddb/cursor_prefetch.js:7: var kNumberOfItems = 100;
On 2011/11/28 20:29:16, jsbell wrote:
> Since this is == kMaxPrefetchAmount in renderer_webidbcursor_impl.h these
tests
> never exercise maxing out the prefetch amount. Bump this to 200 or 300?

Done. Bumping to 200.

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
chrome/test/data/indexeddb/cursor_prefetch.js:10: request =
webkitIndexedDB.open('cursor-prefetch');
On 2011/11/28 20:29:16, jsbell wrote:
> Can you add the indexedDB = indexedDB || webkitIndexedDB; preamble (and ditto
> for IDBCursor, IDBTransaction, etc) to make the rest of the test more
readable?

Done.

http://codereview.chromium.org/8662017/diff/5001/chrome/test/data/indexeddb/c...
chrome/test/data/indexeddb/cursor_prefetch.js:355: 
On 2011/11/28 20:29:16, jsbell wrote:
> Add a comment about what this test is for.

Done.

http://codereview.chromium.org/8662017/diff/5001/content/renderer/indexed_db_...
File content/renderer/indexed_db_dispatcher.cc (right):

http://codereview.chromium.org/8662017/diff/5001/content/renderer/indexed_db_...
content/renderer/indexed_db_dispatcher.cc:555: void
IndexedDBDispatcher::ResetCursorPrefetchCaches(int32 exception_cursor_id) {
On 2011/11/28 20:29:16, jsbell wrote:
> Should this at least be scoped to database_id? Is there a need to invalidate
> caches across all IDB instances in this renderer?

That would make sense, but it doesn't make any difference in practice as we're
only running one transaction at the time anyway. I'm not sure it's worth the
added complexity.

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
File content/renderer/renderer_webidbcursor_impl.cc (right):

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:86: prefetch_amount_ *= 2;
On 2011/11/28 20:29:16, jsbell wrote:
> Is there any way to check that this hasn't grown too big for a single IPC
> message? (Is that a valid concern?) My worry is that this will lead to
seemingly
> intermittent failures if e.g. 50 key/value pairs fit in an IPC message but 100
> don't, which would be hard to debug. I'm thinking of crbug.com/96826 made much
> worse

Right, this could be a problem. I'll add code to limit the amount of
pre-fetching based on the approximate sizes of the keys and values on the WebKit
side.

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:110:
pending_onsuccess_callbacks_--;
On 2011/11/28 20:29:16, jsbell wrote:
> Add DCHECK that pending_onsuccess_callbacks_ >= 0?

It can be negative. The number is decreased for each onsuccess handler on a
cursor. If the continue doesn't hit the cache, then there is nothing to
increment the number.

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:110:
pending_onsuccess_callbacks_--;
On 2011/11/29 09:08:13, dgrogan wrote:
> On 2011/11/28 20:29:16, jsbell wrote:
> > Add DCHECK that pending_onsuccess_callbacks_ >= 0?
> 
> Can pending_onsuccess_callbacks_ ever be > 2?

No, it's always <= 2. Not sure DCHECKing for that would make anything clearer,
though?

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:114: //
pending_onsuccess_callacks_ would be incremented.
On 2011/11/29 09:08:13, dgrogan wrote:
> callbacks_ typo

Done.

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:167: // No prefetch cache, so no
need to reset the cursor in the back-end.
On 2011/11/29 09:08:13, dgrogan wrote:
> I might be showing ignorance here, but would this not also be triggered if
> ResetPrefetchCache were called after javascript had called continue precisely
> the same number of times as the number of cached elements?

You're completely right.

If continue has been called precisely the same number of times as the number of
cached elements, that means the cache is now empty and the backend cursor is in
the right position, i.e. there is no need to reset.

[dgrogan, 2011-11-30 05:11:06]

As one of the folks who will be responsible to pick up the pieces if this
breaks, I feel ok saying LG.

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
File content/renderer/renderer_webidbcursor_impl.cc (right):

http://codereview.chromium.org/8662017/diff/5001/content/renderer/renderer_we...
content/renderer/renderer_webidbcursor_impl.cc:110:
pending_onsuccess_callbacks_--;
On 2011/11/29 14:44:34, hans wrote:
> On 2011/11/29 09:08:13, dgrogan wrote:
> > On 2011/11/28 20:29:16, jsbell wrote:
> > > Add DCHECK that pending_onsuccess_callbacks_ >= 0?
> > 
> > Can pending_onsuccess_callbacks_ ever be > 2?
> 
> No, it's always <= 2. Not sure DCHECKing for that would make anything clearer,
> though?

Agreed, I was asking to make sure I understood what was going on.

[hans, 2011-11-30 09:16:31]

Thanks, David!

Darin: OWNERS review for content/common and content/renderer?

[darin (slow to review), 2011-11-30 20:51:55]

LGTM

http://codereview.chromium.org/8662017/diff/17001/content/renderer/renderer_w...
File content/renderer/renderer_webidbcursor_impl.cc (right):

http://codereview.chromium.org/8662017/diff/17001/content/renderer/renderer_w...
content/renderer/renderer_webidbcursor_impl.cc:163: continue_count_ = 0;
nit: indentation is wrong

[hans, 2011-12-01 11:04:02]

Thanks very much everyone for looking!

This is now ready to go in once WebKit rolls.

http://codereview.chromium.org/8662017/diff/17001/content/renderer/renderer_w...
File content/renderer/renderer_webidbcursor_impl.cc (right):

http://codereview.chromium.org/8662017/diff/17001/content/renderer/renderer_w...
content/renderer/renderer_webidbcursor_impl.cc:163: continue_count_ = 0;
On 2011/11/30 20:51:55, darin wrote:
> nit: indentation is wrong

Done.