Bug 73025 – IndexedDB: Cursor pre-fetching

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

IndexedDB: Cursor pre-fetching

[Hans Wennborg]

Created attachment 116378 [details]
Patch

[Hans Wennborg]

Comment on attachment 116378 [details]
Patch

Not ready for real review yet.

[Hans Wennborg]

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

The Chromium side is here: http://codereview.chromium.org/8662017/
I'll need to figure out how to land this on both sides without breaking stuff.

The pre-submit script had a few style nits. I'll fix those next time I upload.

[Hans Wennborg]

Created attachment 116508 [details]
Patch

[Hans Wennborg]

This is ready for review now. Please take a look.

This should be landed first, and the Chromium side patch afterwards.

Adding Darin since this touches the WebKit API.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Hans Wennborg]

Created attachment 116597 [details]
Patch

IDBCursorBackendImpl needs to hold on to a copy of the cursor. We can't do save/restore inside the IDBBackingStore::Cursor object, becuase we actually throw that away if we iterate to the end of the range.

[WebKit Review Bot]

Comment on attachment 116597 [details]
Patch

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

[Hans Wennborg]

Created attachment 116618 [details]
Patch

[David Levin]

Comment on attachment 116618 [details]
Patch

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

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:129
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Just a drive by comment: Both of these items (this, prpCallbacks)  should go directly to createCallbackTask without being put in a RefPtr. (Note at one point, createCallbackTask couldn't compile with a PassRefPtr but now it can. I suspect you're following an old pattern from elsewhere that can be cleaned up now.)

> Source/WebCore/storage/IDBRequest.h:87
> +    virtual void onSuccessWithPrefetch(Vector<RefPtr<IDBKey> > keys, Vector<RefPtr<IDBKey> > primaryKeys, Vector<RefPtr<SerializedScriptValue> > values) { ASSERT_NOT_REACHED(); } // Not implemented. Callback should not reach the renderer side.

I suspect that having variable names here that are unused is going to cause compile errors on some platforms. (In this place and a few others.)

[Hans Wennborg]

Created attachment 116722 [details]
Patch

Thanks David! Uploading new patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 116722 [details]
Patch

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

> Source/WebKit/chromium/public/WebIDBCallbacks.h:59
> +    virtual void onSuccessWithPrefetch(WebVector<WebIDBKey> keys, WebVector<WebIDBKey> primaryKeys, WebVector<WebSerializedScriptValue> values) { WEBKIT_ASSERT_NOT_REACHED(); }

nit: maybe it would be nice to list all of the onSuccess* methods next to one another?

> Source/WebKit/chromium/public/WebIDBTransaction.h:64
> +    virtual void addPendingEvents(int) { WEBKIT_ASSERT_NOT_REACHED(); }

nit: shouldn't this parameter be named?  it is not immediately obvious what this parameter means.

[Hans Wennborg]

(In reply to comment #12)
> (From update of attachment 116722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116722&action=review
> 
> > Source/WebKit/chromium/public/WebIDBCallbacks.h:59
> > +    virtual void onSuccessWithPrefetch(WebVector<WebIDBKey> keys, WebVector<WebIDBKey> primaryKeys, WebVector<WebSerializedScriptValue> values) { WEBKIT_ASSERT_NOT_REACHED(); }
> 
> nit: maybe it would be nice to list all of the onSuccess* methods next to one another?

Will fix.

> 
> > Source/WebKit/chromium/public/WebIDBTransaction.h:64
> > +    virtual void addPendingEvents(int) { WEBKIT_ASSERT_NOT_REACHED(); }
> 
> nit: shouldn't this parameter be named?  it is not immediately obvious what this parameter means.

Yes, but then David Levin pointed out that since the parameter is unused in the function body, some compilers might warn. I suppose I could cast it to void, but then it starts getting messy :S

[David Levin]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 116722 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=116722&action=review

> > 
> > > Source/WebKit/chromium/public/WebIDBTransaction.h:64
> > > +    virtual void addPendingEvents(int) { WEBKIT_ASSERT_NOT_REACHED(); }
> > 
> > nit: shouldn't this parameter be named?  it is not immediately obvious what this parameter means.
> 
> Yes, but then David Levin pointed out that since the parameter is unused in the function body, some compilers might warn. I suppose I could cast it to void, but then it starts getting messy :S

Sounds like I was wrong for chromium. Sorry about that.

[Joshua Bell]

Comment on attachment 116722 [details]
Patch

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

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:124
> +void IDBCursorBackendImpl::prefetchContinue(int n, PassRefPtr<IDBCallbacks> prpCallbacks, ExceptionCode& ec)

Should a more explicit name than "n" be used, e.g. numberToFetch? In the method signature it's pretty obvious, but...

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:141
> +    for (int i = 0; i < n; ++i) {

...here in the middle of the code "n" reads like a poorly named local variable, not an argument.

Also, since i isn't used, would it be more readable as: for ( ; numberToFetch; --numberToFetch)

> Source/WebCore/storage/IDBRequest.cpp:352
> +        if (m_result->type() == IDBAny::IDBCursorWithValueType)

Use "else if" for readability?

> Source/WebCore/storage/IDBTransactionBackendImpl.cpp:152
> +    m_pendingEvents += n;

Should m_pendingEvents ever go negative? If not, add assertion?

[David Grogan]

Comment on attachment 116722 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:29
> +        * src/WebIDBCallbacksImpl.cpp:

There don't seem to be any changes to WebIDBCallbacksImpl in here.

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:143
> +            cursor->m_cursor = 0;

How does this work?  If something else set the backingstore's cursor to null, break.  Or if the backingstore's cursor ran out of elements, break.  What else sets the backingstore's cursor = null?

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:169
> +            bool ok = m_cursor->continueFunction();

I suppose we never implemented cursor->continue(int) that could be used here.

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:950
> +            m_iterator = m_transaction->createIterator();

I'm guessing that one of lines 948/950 is unneeded.

[Hans Wennborg]

Thanks very much for the comments!

(In reply to comment #15)
> (From update of attachment 116722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116722&action=review
> 
> > Source/WebCore/storage/IDBCursorBackendImpl.cpp:124
> > +void IDBCursorBackendImpl::prefetchContinue(int n, PassRefPtr<IDBCallbacks> prpCallbacks, ExceptionCode& ec)
> 
> Should a more explicit name than "n" be used, e.g. numberToFetch? In the method signature it's pretty obvious, but...

numberToFetch sounds good. Changing to that.

> > Source/WebCore/storage/IDBCursorBackendImpl.cpp:141
> > +    for (int i = 0; i < n; ++i) {
> 
> ...here in the middle of the code "n" reads like a poorly named local variable, not an argument.
> 
> Also, since i isn't used, would it be more readable as: for ( ; numberToFetch; --numberToFetch)

Unless you feel strongly about it, I prefer the for loop with the i variable even though it's not actually used in the loop body. I just find that quicker to read.

> 
> > Source/WebCore/storage/IDBRequest.cpp:352
> > +        if (m_result->type() == IDBAny::IDBCursorWithValueType)
> 
> Use "else if" for readability?

Done.

> 
> > Source/WebCore/storage/IDBTransactionBackendImpl.cpp:152
> > +    m_pendingEvents += n;
> 
> Should m_pendingEvents ever go negative? If not, add assertion?

Adding the assertion.


(In reply to comment #16)
> (From update of attachment 116722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116722&action=review
> 
> > Source/WebKit/chromium/ChangeLog:29
> > +        * src/WebIDBCallbacksImpl.cpp:
> 
> There don't seem to be any changes to WebIDBCallbacksImpl in here.

I'll re-generate the changelogs.

> 
> > Source/WebCore/storage/IDBCursorBackendImpl.cpp:143
> > +            cursor->m_cursor = 0;
> 
> How does this work?  If something else set the backingstore's cursor to null, break.  Or if the backingstore's cursor ran out of elements, break.  What else sets the backingstore's cursor = null?

m_cursor could be null when this function is called if the cursor has previously run out of elements. This check could be broken out of the for-loop if you think that would make it more readable?

> 
> > Source/WebCore/storage/IDBCursorBackendImpl.cpp:169
> > +            bool ok = m_cursor->continueFunction();
> 
> I suppose we never implemented cursor->continue(int) that could be used here.

Do you mean the IDBCursor advance() function? Yes, that's still on the todo list.

> 
> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:950
> > +            m_iterator = m_transaction->createIterator();
> 
> I'm guessing that one of lines 948/950 is unneeded.

Right. Removed 948.



I'm also making a change to IDBRequest::dispatchEvent(), because it wasn't actually working as I expected. We need to hold on to the cursor m_result, because if the onsuccess handler calls continue() then it will reset m_result.

[Hans Wennborg]

Created attachment 116961 [details]
Patch

[Hans Wennborg]

Created attachment 116976 [details]
Patch

Fix another s/n/numberToFetch/

[Hans Wennborg]

Tony: would you like to take a look?

[Hans Wennborg]

Created attachment 117237 [details]
Patch

Don't set cursor->m_cursor = 0 when sizeEstimate > kMaxSizeEstimate, as pointed out by Joshua.

[Tony Chang]

Comment on attachment 117237 [details]
Patch

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

Just some minor style nits.  @jsbell: Does this look good to you?

> Source/WebCore/storage/IDBCallbacks.h:59
> +    virtual void onSuccessWithPrefetch(Vector<RefPtr<IDBKey> > keys, Vector<RefPtr<IDBKey> > primaryKeys, Vector<RefPtr<SerializedScriptValue> > values) = 0;

Can these be const references to avoid copying the vectors?

> Source/WebCore/storage/IDBCursorBackendImpl.h:73
> +    static void prefetchContinueInternal(ScriptExecutionContext*, PassRefPtr<IDBCursorBackendImpl>, int, PassRefPtr<IDBCallbacks>);

Nit: I would name the int since it's not obvious from the name what it is for.

> Source/WebCore/storage/IDBKey.h:83
> +        idbKey->m_sizeEstimate = 100;

Maybe add a comment of why 100 here?

[Joshua Bell]

(In reply to comment #22)
> (From update of attachment 117237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117237&action=review
> 
> Just some minor style nits.  @jsbell: Does this look good to you?

LGTM, although since you point it out I'm also unsure about the 100; string and array should have overhead for the length prefix... looking at ipc/ipc_message_utils.h: and base/pickle.cc only sizeof(int) extra needs to be accounted for in both cases.

[Hans Wennborg]

(In reply to comment #22)
> (From update of attachment 117237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117237&action=review

Thanks very much for the review!

> 
> Just some minor style nits.  @jsbell: Does this look good to you?
> 
> > Source/WebCore/storage/IDBCallbacks.h:59
> > +    virtual void onSuccessWithPrefetch(Vector<RefPtr<IDBKey> > keys, Vector<RefPtr<IDBKey> > primaryKeys, Vector<RefPtr<SerializedScriptValue> > values) = 0;
> 
> Can these be const references to avoid copying the vectors?

Done.

> 
> > Source/WebCore/storage/IDBCursorBackendImpl.h:73
> > +    static void prefetchContinueInternal(ScriptExecutionContext*, PassRefPtr<IDBCursorBackendImpl>, int, PassRefPtr<IDBCallbacks>);
> 
> Nit: I would name the int since it's not obvious from the name what it is for.

Done.

> 
> > Source/WebCore/storage/IDBKey.h:83
> > +        idbKey->m_sizeEstimate = 100;
> 
> Maybe add a comment of why 100 here?

As per Josh's comment, I broke this out into a named constant that I'm adding to each key object. I don't think the value is that important, I just want to add a little something to each key to reflect that there's some overhead to each of them.

[Hans Wennborg]

Created attachment 117253 [details]
Patch

[Hans Wennborg]

Committed r101645: <http://trac.webkit.org/changeset/101645>