Bug 94023 – IndexedDB: Use ScriptValue instead of SerializedScriptValue for put/add/update

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

The WebKit IDB implementation uses SerializedScriptValue throughout, which incurs overhead and complexity:

* Converting values to IDBKeys is port specific (e.g. v8/IDBBindingUtilities.cpp's createIDBKeyFromValue)
* When evaluating key paths, SSVs are deserialized in a script context, then the value is groveled over (port-specific)
* When injecting keys into values, SSVs are deserialized, the value is groveled over, then the object is reserialized

Ideally, the values would remain as ScriptValues rather than SerializedScriptValues until they are transmitted over the wire or persisted to storage. In the Chromium port, transmission between processes used to occur almost immediately, but code has moved into the front-end so there is now benefit to retaining the objects "live" for longer.

[Alec Flett]

I'll take this, I've got a work-in-progress.

Minor details:
1) In the IDL we have to use DOMObject and we get a ScriptObject, and you have to use ScriptState instead of ScriptExecutionContext

2) ScriptValue / ScriptObject are not abstract enough to avoid writing any v8-specific code, but I suspect they could be improved upon - I don't see anything on the V8 side that would make that too hard though.

So I'll do a first cut which does all v8-specific stuff (I can reuse most of the stuff in IDBBindingUtilities) and then try to abstract it away into ScriptObject/ScriptValue

[Alec Flett]

Created attachment 161369 [details]
Patch

[Alec Flett]

This patch only covers the put/add/update side of things - the get/cursor side of things was separate (and complicated) enough to justify solving this in two phases.

[Peter Beverloo (cr-android ews)]

Comment on attachment 161369 [details]
Patch

Attachment 161369 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13691505

[WebKit Review Bot]

Comment on attachment 161369 [details]
Patch

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

[Joshua Bell]

Comment on attachment 161369 [details]
Patch

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

Overall logic lgtm.

> Source/WebCore/ChangeLog:10
> +        steps while storing values.

Maybe mention/reference the follow-up bug for the "get" side? (Just be careful not to start the line with http://... or the patch system gets confused)

> Source/WebCore/ChangeLog:12
> +        No new tests, this is a performance refactor and existing tests cover

Please make sure you capture performance data before/after this lands!

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:164
> +        ec = IDBDatabaseException::DATA_ERR;

Shouldn't this be DATA_CLONE_ERR to match what would come out of the binding layer's serialization?

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:127
> +static v8::Handle<v8::Value> getNthValueOnKeyPath(v8::Handle<v8::Value>& rootValue, const Vector<String>& keyPathElements, size_t index)

Nice. Can the "static" qualifier be sprinkled on the other functions, too?

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:151
> +        if (!get(currentValue, keyPathElement))

This appears correct, but the get() function has a poorly designed API, which leads to this function being hard to read. Maybe take the opportunity to improve it (e.g. bool get(source, name, dest) ?)

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:185
> +static PassRefPtr<IDBKey> createIDBKeyFromScriptValueAndKeyPath(ScriptValue& value, const String& keyPath)

Should this be in the anonymous namespace?

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:221
>  static PassRefPtr<IDBKey> createIDBKeyFromSerializedValueAndKeyPath(PassRefPtr<SerializedScriptValue> prpValue, const String& keyPath)

Move this to anonymous namespace too?

> Source/WebCore/bindings/v8/IDBBindingUtilities.h:44
> +// FIXME: remove the SerializedValue versions when their use is finally removed

Capitalize and punctuate.

Maybe reference the "get" side bug? Or will there be 3 patches total?

> Source/WebCore/bindings/v8/IDBBindingUtilities.h:50
> +ScriptValue deserializeIDBValue(PassRefPtr<SerializedScriptValue>);

It seems odd to have this here rather than e.g. as a static on SerializedScriptValue, since it's not IDB specific.

[Alec Flett]

Created attachment 162315 [details]
Patch

[Alec Flett]

sorry I just realized that josh had already added review comments. ignore that last patch.

[Alec Flett]

Created attachment 162317 [details]
Patch

[Alec Flett]

ok, the latest patch addresses most of the comments.


regarding deserializeIDBValue - yeah this bugged me, but I need to talk to someone who knows more about this stuff - there are like 3 different ways to deserialize a SerializedScriptValue and IDB uses the V8AuxiliaryContext approach. Nobody else uses that, and I need to find out why. (for instance SerializedScriptValue::deserializeForInspector takes a different approach)

But once bug 95409 is fixed, there will be only one use of V8AuxiliaryContext, and I'd like to delay that cleanup until after that one lands. (That will involve a cleanup purely of SerializedScriptValue and ScriptValue proper, and just a one-liner in IDB code)

[Peter Beverloo (cr-android ews)]

Comment on attachment 162317 [details]
Patch

Attachment 162317 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13757756

[Alec Flett]

Created attachment 162372 [details]
Patch

[Alec Flett]

Created attachment 162832 [details]
Patch

[Alec Flett]

Created attachment 163236 [details]
Patch

[Alec Flett]

Comment on attachment 163236 [details]
Patch

This updated version does away with V8AuxiliaryContext entirely, for new code. In particular, it will reuse the ScriptExecutionContext to get a V8 context, when available.

[Alec Flett]

haraken@ - when you pointed me to toV8Context, it turns out that it helped more than I expected. This patch is now ready for a full review, can you take a look?

[Adam Barth]

Comment on attachment 163236 [details]
Patch

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

> Source/WebCore/ChangeLog:42
> [phone]  Alec Flett  <[e-mail]>

Looks like you've got two changelogs here.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.idl:37
> -        [CallWith=ScriptExecutionContext] IDBRequest put(in SerializedScriptValue value, in [Optional] IDBKey key)
> +        [CallWith=ScriptExecutionContext] IDBRequest put(in DOMObject value, in [Optional] IDBKey key)

DOMObject -> any?

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:234
> +    context->Enter();

Please use v8::Context::Scope rather than calling Enter and Exit manually.

[Kentaro Hara]

Comment on attachment 163236 [details]
Patch

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

It looks like you are doing a couple of things in this patch rather than just replacing SerializedScriptValues with ScriptValues. Although I don't fully understand the IDB part changes, the logic of deserializeIDBValue() and the SSV replacement logic look good to me except for abarth's comment (i.e. please use v8::Context::Scope).

> Source/WebCore/ChangeLog:17
> +        No new tests, this is a performance refactor and existing tests cover
> +        correctness.

Please list up a couple of tests that might be potentially affected by this change. You can just say:

xxx.html (No behavior in change)

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:-149
> -    if (value->blobURLs().size() > 0) {
> -        // FIXME: Add Blob/File/FileList support
> -        ec = IDBDatabaseException::IDB_DATA_CLONE_ERR;
> -        return 0;
> -    }

Why did you remove it?

> Source/WebCore/Modules/indexeddb/IDBCursor.idl:42
> +        [CallWith=ScriptExecutionContext] IDBRequest update(in DOMObject value)

DOMObject => any

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:166
> +    ScriptState* state = ScriptState::current();
> +    RefPtr<SerializedScriptValue> serializedValue = value.serialize(state);
> +    if (state->hadException()) {
> +        ec = IDBDatabaseException::IDB_DATA_CLONE_ERR;
> +        return 0;
> +    }

Shall we make this change in another patch? (before landing this patch)

[Alec Flett]

Comment on attachment 163236 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:-149
>> -    }
> 
> Why did you remove it?

it's actually covered by the call below to IDBObjectStore::put() - it was redundant (I missed its redundancy in an earlier refactoring)

>> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:166
>> +    }
> 
> Shall we make this change in another patch? (before landing this patch)

No this case doesn't really exist in the world before we use ScriptValue.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:168
> +    if (serializedValue->blobURLs().size() > 0) {

(this is the check that was removed from IDBCursor::update - update now calls into this function)

>> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:234
>> +    context->Enter();
> 
> Please use v8::Context::Scope rather than calling Enter and Exit manually.

ah! I knew there had to be something like that. Thanks.

thanks for the update guys, new patch coming up soon.

[Alec Flett]

Created attachment 163402 [details]
Patch

[Alec Flett]

ok, new patch is ready - the only thing I couldn't address was the use of 'any' - as far as I can tell there is no special support of this type, which means it's trying to include "V8any.h"

I looked through the V8 binding / code generator, and there's no specific support for 'any' (though it's mentioned in CodeGeneratorJS.pm)

abarth@ / haraken@ - your preference here? I can try my hand at adding 'any' support (I think it's just a one-liner to go along with DOMObject)

[Kentaro Hara]

(In reply to comment #21)
> abarth@ / haraken@ - your preference here? I can try my hand at adding 'any' support (I think it's just a one-liner to go along with DOMObject)

Would you add the 'any' support to CodeGeneratorV8.pm?

[Alec Flett]

Created attachment 163681 [details]
Patch

[Alec Flett]

abarth@ or haraken@ - v8 stuff is in, can you review the updated version of the patch?

[Adam Barth]

Comment on attachment 163681 [details]
Patch

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

All the v8-related stuff LGTM.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:108
> +                                      ScriptValue& objectValue,

Does it make sense to pass the ScriptValue by const reference?

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:237
> +    v8::Handle<v8::Value> v8Value(prpValue->deserialize());
> +    return ScriptValue(v8Value);

I would have just combined these lines.

[Adam Barth]

I'll let haraken do the final review since he gave you more substantial comments earlier.

[Kentaro Hara]

Comment on attachment 163681 [details]
Patch

The V8 part LGTM. If jsbell@ says that the IDB part is OK, we're happy to r+ it.

[Alec Flett]

Created attachment 163713 [details]
Patch

[Joshua Bell]

Comment on attachment 163713 [details]
Patch

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

IDB changes lgtm

> Tools/Scripts/webkitpy/common/net/credentials.py:44
> +    #import keyring

I don't think you meant to include this change. :)

[Kentaro Hara]

Comment on attachment 163713 [details]
Patch

Please just fix a nit pointed out by jsbell, before landing.

[Alec Flett]

Created attachment 163718 [details]
Patch

[WebKit Review Bot]

Comment on attachment 163718 [details]
Patch

Clearing flags on attachment: 163718

Committed r128379: <http://trac.webkit.org/changeset/128379>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.