IndexedDB: Cursor pre-fetching
Created attachment 116378 [details] Patch
Comment on attachment 116378 [details] Patch Not ready for real review yet.
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.
Created attachment 116508 [details] Patch
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.
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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.
Comment on attachment 116597 [details] Patch Attachment 116597 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10618430
Created attachment 116618 [details] Patch
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.)
Created attachment 116722 [details] Patch Thanks David! Uploading new patch.
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.
(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
(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.
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?
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.
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.
Created attachment 116961 [details] Patch
Created attachment 116976 [details] Patch Fix another s/n/numberToFetch/
Tony: would you like to take a look?
Created attachment 117237 [details] Patch Don't set cursor->m_cursor = 0 when sizeEstimate > kMaxSizeEstimate, as pointed out by Joshua.
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?
(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.
(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.
Created attachment 117253 [details] Patch
Committed r101645: <http://trac.webkit.org/changeset/101645>