Consolidate key, primary key, value cursor messages.

Consolidate key, primary key, value cursor messages.

When opening or advancing a cursor, send key, primary key, and value to the renderer in the cursor open message.  Remove separate messages that retrieve them synchronously.

This improves the readSeq benchmark by ~65%.

This is the chrome side patch.  It potentially needs the webkit patch at https://bugs.webkit.org/show_bug.cgi?id=69131 to go in first.

BUG=
TEST=new-run-webkit-tests --debug --chromium storage/indexeddb; loaded each cursor test into chrome manually


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

[dgrogan, 2011-09-02 23:35:47]

Patch set 2 should say "remove unused _methods_".  This one isn't a big deal,
but is there a way to edit those?

[michaeln, 2011-09-04 23:08:15]

http://codereview.chromium.org/7834006/diff/16/content/renderer/indexed_db_di...
File content/renderer/indexed_db_dispatcher.cc (right):

http://codereview.chromium.org/7834006/diff/16/content/renderer/indexed_db_di...
content/renderer/indexed_db_dispatcher.cc:387: void
IndexedDBDispatcher::OnSuccessSerializedScriptValue(
OnSuccessOpenCursor() is the callback for the various create or open calls, but
isn't OnSuccessSerializedScriptValue() the callback for the continue or advance
calls? If so, does that msg/method also need to carry the additional 'key' and
'primary key' values.

I guess you'd need to know what cursor_id and a way to lookup a
RendererWebIDBCursorImpl for that id, then it be a simple matter of calling...

  cursor->setCurrentKeyAndValue(key, primary_key, value);
  onSuccess(value);

http://codereview.chromium.org/7834006/diff/16/content/renderer/indexed_db_di...
content/renderer/indexed_db_dispatcher.cc:399: callbacks->onSuccess(new
RendererWebIDBCursorImpl(object_id), key,
Might be nice if the Cursor class contained a single method to update these
three values all in one go, is there any reason to set one w/o setting the
others...

   cursor = new Cursor(id);
   cursor->setCurrentKeyAndValue(key, primary_key, value);
   onSuccess(cursor);

That same method could be useful in the continue/advance case as well.

http://codereview.chromium.org/7834006/diff/16/content/renderer/renderer_webi...
File content/renderer/renderer_webidbcursor_impl.h (left):

http://codereview.chromium.org/7834006/diff/16/content/renderer/renderer_webi...
content/renderer/renderer_webidbcursor_impl.h:24: virtual
WebKit::WebSerializedScriptValue value() const;
This is an example of that 'difficult to review' thing mentioned in email.

Why are these methods being removed? The WebCore::IDBCursor class still needs to
retrieve these values in someway. Aren't these methods that way?

[dgrogan, 2011-09-07 01:15:19]

Michael,

Could you take another look?

The webkit patch is here
https://bugs.webkit.org/show_bug.cgi?id=67663

http://codereview.chromium.org/7834006/diff/16/content/renderer/indexed_db_di...
File content/renderer/indexed_db_dispatcher.cc (right):

http://codereview.chromium.org/7834006/diff/16/content/renderer/indexed_db_di...
content/renderer/indexed_db_dispatcher.cc:387: void
IndexedDBDispatcher::OnSuccessSerializedScriptValue(
On 2011/09/04 23:08:16, michaeln wrote:
> OnSuccessOpenCursor() is the callback for the various create or open calls,
but
> isn't OnSuccessSerializedScriptValue() the callback for the continue or
advance
> calls?

OnSuccessOpenCursor is also used for a successful continue.  Confusing.

It looks like cursor only calls OnSuccessSerializedScriptValue when the cursor
has run out of elements.  The value in that case is null, so I think it's ok to
not set the other things.

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/So...


> If so, does that msg/method also need to carry the additional 'key' and
> 'primary key' values.
> 
> I guess you'd need to know what cursor_id and a way to lookup a
> RendererWebIDBCursorImpl for that id, then it be a simple matter of calling...
> 
>   cursor->setCurrentKeyAndValue(key, primary_key, value);
>   onSuccess(value);

http://codereview.chromium.org/7834006/diff/16/content/renderer/renderer_webi...
File content/renderer/renderer_webidbcursor_impl.h (left):

http://codereview.chromium.org/7834006/diff/16/content/renderer/renderer_webi...
content/renderer/renderer_webidbcursor_impl.h:24: virtual
WebKit::WebSerializedScriptValue value() const;
On 2011/09/04 23:08:16, michaeln wrote:
> This is an example of that 'difficult to review' thing mentioned in email.
> 
> Why are these methods being removed? The WebCore::IDBCursor class still needs
to
> retrieve these values in someway. Aren't these methods that way?

The webkit patch stores key, primary key, and value in the WebCore::IDBCursor
frontend object.  So calls to key(), primaryKey(), and value() don't have to go
over the wire.

[michaeln, 2011-09-07 03:56:55]

> The webkit patch stores key, primary key, and value in the WebCore::IDBCursor
> frontend object.  So calls to key(), primaryKey(), and value() don't have to
go
> over the wire.

Something is not adding up for me? Maybe i don't get the semantics of the
scriptable Cursor object?

I would expect that after successfully advancing a cursor instance, those
getters on the instance that was advanced would reflect the values behind the
new cursor position. But in the CL/patch, what i see is that a new cursor
instance is created and the instance which was originally advanced is left
unmodified (and will continue to reflect the old position's values forever which
would make it easy to fall into an infinite loop).

// indexed_db_dispatcher.cc
// after a step creation of a new 'backend' from 
void IndexedDBDispatcher::OnSuccessOpenCursor(int32 repsonse_id, ...) {
  callbacks->onSuccess(new RendererWebIDBCursorImpl(object_id), ...


// WebCore/storage/IDBRequest.cpp
// after a step creation of a new 'frontend' from  
void IDBRequest::onSuccess(PassRefPtr<IDBCursorBackendInterface> backend, ... {
   m_result = IDBAny::create(IDBCursor::create(backend, ...

Maybe i'm misreading things, or maybe my "expectation" is incorrect?

[hans, 2011-09-07 16:26:34]

Now that I see the WebKit side to, I was expecting the key and value to be
stored closer to the Chromium side, rather than in WebCore.

I was thinking that when the value() call reached RendererWebIDBCursorImpl, it
could go "hey, I already got that value from the last call to continue, so I can
just return it instead of doing IPC". Not really sure whether that's better,
though :S

[michaeln, 2011-09-07 22:59:57]

> Maybe i'm misreading things, or maybe my "expectation" is incorrect?

After looking more closely, sure enough, there is a new javascript 'cursor'
instance as a result of advancing an existing javascript 'cursor' instance. In
the current impl, the new and the old instances are functionally equivalent
since they ultimately refer to the same LevelDB cursor deep down in the
browser-side. In the new code being proposed, the new and the old instance will
no longer be functionally equivalent since the old one will still contain the
values associated with the old position. If that's right... I think that's a
bug.

Also, the new and the old instances won't pass a javascript equality test (in
the current impl or the CL/patch). I think that's probably a correctness issue
that we'll want to fix eventually.

[dgrogan, 2011-09-07 23:14:10]

On Wed, Sep 7, 2011 at 3:59 PM, <michaeln@chromium.org> wrote:

>  Maybe i'm misreading things, or maybe my "expectation" is incorrect?
>>
>
> After looking more closely, sure enough, there is a new javascript 'cursor'
> instance as a result of advancing an existing javascript 'cursor' instance.
> In
> the current impl, the new and the old instances are functionally equivalent
> since they ultimately refer to the same LevelDB cursor deep down in the
> browser-side.


Right.  Thanks for saving me the trouble of writing out such a succinct
summary :)


> In the new code being proposed, the new and the old instance will
> no longer be functionally equivalent since the old one will still contain
> the
> values associated with the old position. If that's right... I think that's
> a
> bug.
>

Ah, you're right, that's a bug.  If javascript stashes a cursor object
outside the IDBRequest's onsuccess handler, it will have old values when the
one inside the onsuccess handler will have new values.


>
> Also, the new and the old instances won't pass a javascript equality test
> (in
> the current impl or the CL/patch). I think that's probably a correctness
> issue
> that we'll want to fix eventually.


It looks like that also violates the spec, but you're right, it's not as
severe:
"There is only ever one
IDBCursor<http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#idl-def-IDBCursor>or
IDBCursorSync<http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#idl-def-IDBCursorSync>instance
representing a given
cursor<http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#dfn-cursor>.
"

So, I propose that I table this patch until one that reuses at least the
IDBCursor object, and maybe the RendererWebIDBCursor object, is committed.


>
>
>
http://codereview.chromium.**org/7834006/<http://codereview.chromium.org/7834...
>

[michaeln, 2011-09-08 19:16:31]

> So, I propose that I table this patch until one that reuses at least the
> IDBCursor object, and maybe the RendererWebIDBCursor object, is committed.

sgtm

[dgrogan, 2011-09-28 19:19:04]

On 2011/09/07 23:14:10, dgrogan wrote:
> On Wed, Sep 7, 2011 at 3:59 PM, <mailto:michaeln@chromium.org> wrote:
> 
> >  Maybe i'm misreading things, or maybe my "expectation" is incorrect?
> >>
> >
> > After looking more closely, sure enough, there is a new javascript 'cursor'
> > instance as a result of advancing an existing javascript 'cursor' instance.
> > In
> > the current impl, the new and the old instances are functionally equivalent
> > since they ultimately refer to the same LevelDB cursor deep down in the
> > browser-side.

Turns out this is not true.  Even in the current impl the new and old instances
return a different value.  They return the same key though.  There's something
going on in the JS bindings where the value is deserialized when
V8IDBCursorWithValue is constructed and the result is cached in the V8 object. 
So the V8 object only calls down to the WebCore object to get the value once.  I
wrote a small layout test that demonstrates this in
https://bugs.webkit.org/show_bug.cgi?id=69012.  I've also got an e-mail thread
going with the guys who installed this caching behavior.

So, given that this performance change doesn't actually introduce any buggy
behavior, I'd like to commit that layout test while in discussion about the
inconsistency bug and try to get the performance fix committed.  I'll upload an
updated patch to this issue in a bit.

> 
> 
> Right.  Thanks for saving me the trouble of writing out such a succinct
> summary :)
> 
> 
> > In the new code being proposed, the new and the old instance will
> > no longer be functionally equivalent since the old one will still contain
> > the
> > values associated with the old position. If that's right... I think that's
> > a
> > bug.
> >
> 
> Ah, you're right, that's a bug.  If javascript stashes a cursor object
> outside the IDBRequest's onsuccess handler, it will have old values when the
> one inside the onsuccess handler will have new values.
> 
> 
> >
> > Also, the new and the old instances won't pass a javascript equality test
> > (in
> > the current impl or the CL/patch). I think that's probably a correctness
> > issue
> > that we'll want to fix eventually.
> 
> 
> It looks like that also violates the spec, but you're right, it's not as
> severe:
> "There is only ever one
>
IDBCursor<http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#idl-def-IDBCursor>or
>
IDBCursorSync<http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#idl-def-IDBCursorSync>instance
> representing a given
> cursor<http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#dfn-cursor>.
> "
> 
> So, I propose that I table this patch until one that reuses at least the
> IDBCursor object, and maybe the RendererWebIDBCursor object, is committed.
> 
> 
> >
> >
> >
>
http://codereview.chromium.**org/7834006/%3Chttp://codereview.chromium.org/78...>
> >

[michaeln, 2011-09-28 23:14:50]

> > > After looking more closely, sure enough, there is a new javascript
'cursor'
> > > instance as a result of advancing an existing javascript 'cursor'
instance.
> > > In
> > > the current impl, the new and the old instances are functionally
equivalent
> > > since they ultimately refer to the same LevelDB cursor deep down in the
> > > browser-side.
> 
> Turns out this is not true.  Even in the current impl the new and old
instances
> return a different value.  They return the same key though.  There's something
> going on in the JS bindings where the value is deserialized when
> V8IDBCursorWithValue is constructed and the result is cached in the V8 object.

Yikes, crsr.key from current pos + crsr.value from old position, thats a bug. If
that v8 construct has the property of being thereafter cached in v8 and not
retrieved from the backing c++ object, maybe the 'value' attribute shouldn't be
defined in terms of that construct. The cursors value definitely needs to change
over time. Do you know why it was defined as it is?

Seems like a significant bug that savedCursors are not trustworthy. Unless
consumers know to avoid that, seems like this will be trouble for them. We
should make sure they know to avoid this until we have a fix for it.

[dgrogan, 2011-09-30 07:03:29]

I reworked this to be similar to what you guys were describing, where we stash
the key and value in RendererWebIDBCursor instead of in the IDBCursor object.

The only webkit change needed is this two-liner:
https://bugs.webkit.org/show_bug.cgi?id=69131

[michaeln, 2011-09-30 21:55:00]

lgtm

To be clear, to go forwards on performance, we're going backwards a little in
another place. The layout tests you recently added will show additional failures
with storedCursor.key (in addition to storedCursor.value) permanently reflecting
the first step results. Is that right?

[dgrogan, 2011-09-30 22:56:23]

On 2011/09/30 21:55:00, michaeln wrote:
> lgtm
> 
> To be clear, to go forwards on performance, we're going backwards a little in
> another place. The layout tests you recently added will show additional
failures
> with storedCursor.key (in addition to storedCursor.value) permanently
reflecting
> the first step results. Is that right?

That's right.

Though it's arguable that this way is a little better way because at least the
stored cursor gives key/value pairs that are associated and not, for example,
key3/value1.

[dgrogan, 2011-09-30 23:44:37]

On 2011/09/30 22:56:23, dgrogan wrote:
> On 2011/09/30 21:55:00, michaeln wrote:
> > lgtm
> > 
> > To be clear, to go forwards on performance, we're going backwards a little
in
> > another place. The layout tests you recently added will show additional
> failures
> > with storedCursor.key (in addition to storedCursor.value) permanently
> reflecting
> > the first step results. Is that right?
> 
> That's right.
> 
> Though it's arguable that this way is a little better way because at least the
> stored cursor gives key/value pairs that are associated and not, for example,
> key3/value1.

PS: Could you informally review the dependent webkit patch:
https://bugs.webkit.org/show_bug.cgi?id=69131 ?

[hans, 2011-10-10 15:23:11]

This is ready to land now, right?

[commit-bot: I haz the power, 2011-10-11 04:01:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/7834006/13001

[commit-bot: I haz the power, 2011-10-11 04:01:20]

Can't apply patch for file content/renderer/renderer_webidbcursor_impl.cc.
While running patch -p1 --forward --force;
patching file content/renderer/renderer_webidbcursor_impl.cc
Hunk #2 FAILED at 39.
1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/renderer_webidbcursor_impl.cc.rej

[commit-bot: I haz the power, 2011-10-11 08:04:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/7834006/29001

[commit-bot: I haz the power, 2011-10-11 08:04:41]

Presubmit check for 7834006-29001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/renderer/indexed_db_dispatcher.h,content/common/indexed_db_messages.h,content/renderer/renderer_webidbcursor_impl.cc,content/renderer/renderer_webidbcursor_impl.h,content/renderer/indexed_db_dispatcher.cc

Presubmit checks took 2.0s to calculate.

[dgrogan, 2011-10-11 08:05:51]

jam, could you look this over as an owner of content/renderer and content/common
?  Thanks.

[jam, 2011-10-11 20:29:30]

lgtm

[commit-bot: I haz the power, 2011-10-12 00:05:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/7834006/29001

[commit-bot: I haz the power, 2011-10-12 05:23:43]

Change committed as 105019