Bug 94023

Summary: IndexedDB: Use ScriptValue instead of SerializedScriptValue for put/add/update
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: WebCore Misc.Assignee: Alec Flett <alecflett>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alecflett, charles.wei, dglazkov, dgrogan, dpranke, haraken, japhet, jiyeon0402.kim, michael, ojan, oliver, peter+ews, webkit.review.bot
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 91125    
Bug Blocks: 88287, 95409    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Joshua Bell 2012-08-14 15:03:40 PDT
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.
Comment 1 Alec Flett 2012-08-17 17:39:54 PDT
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
Comment 2 Alec Flett 2012-08-29 17:34:05 PDT
Created attachment 161369 [details]
Patch
Comment 3 Alec Flett 2012-08-29 17:35:21 PDT
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.
Comment 4 Peter Beverloo (cr-android ews) 2012-08-29 20:03:05 PDT
Comment on attachment 161369 [details]
Patch

Attachment 161369 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13691505
Comment 5 WebKit Review Bot 2012-08-29 22:53:58 PDT
Comment on attachment 161369 [details]
Patch

Attachment 161369 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13683622
Comment 6 Joshua Bell 2012-08-31 17:16:49 PDT
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.
Comment 7 Alec Flett 2012-09-05 13:17:03 PDT
Created attachment 162315 [details]
Patch
Comment 8 Alec Flett 2012-09-05 13:17:50 PDT
sorry I just realized that josh had already added review comments. ignore that last patch.
Comment 9 Alec Flett 2012-09-05 13:31:17 PDT
Created attachment 162317 [details]
Patch
Comment 10 Alec Flett 2012-09-05 13:34:44 PDT
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)
Comment 11 Peter Beverloo (cr-android ews) 2012-09-05 16:05:10 PDT
Comment on attachment 162317 [details]
Patch

Attachment 162317 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13757756
Comment 12 Alec Flett 2012-09-05 16:58:53 PDT
Created attachment 162372 [details]
Patch
Comment 13 Alec Flett 2012-09-07 11:33:22 PDT
Created attachment 162832 [details]
Patch
Comment 14 Alec Flett 2012-09-10 15:44:17 PDT
Created attachment 163236 [details]
Patch
Comment 15 Alec Flett 2012-09-10 15:45:48 PDT
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.
Comment 16 Alec Flett 2012-09-10 15:47:47 PDT
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?
Comment 17 Adam Barth 2012-09-10 16:14:21 PDT
Comment on attachment 163236 [details]
Patch

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

> Source/WebCore/ChangeLog:42
> +2012-09-06  Alec Flett  <alecflett@chromium.org>

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.
Comment 18 Kentaro Hara 2012-09-11 02:30:16 PDT
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)
Comment 19 Alec Flett 2012-09-11 08:52:50 PDT
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.
Comment 20 Alec Flett 2012-09-11 10:54:26 PDT
Created attachment 163402 [details]
Patch
Comment 21 Alec Flett 2012-09-11 10:57:10 PDT
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)
Comment 22 Kentaro Hara 2012-09-11 11:01:58 PDT
(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?
Comment 23 Alec Flett 2012-09-12 13:36:13 PDT
Created attachment 163681 [details]
Patch
Comment 24 Alec Flett 2012-09-12 14:18:26 PDT
abarth@ or haraken@ - v8 stuff is in, can you review the updated version of the patch?
Comment 25 Adam Barth 2012-09-12 14:32:01 PDT
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.
Comment 26 Adam Barth 2012-09-12 14:32:40 PDT
I'll let haraken do the final review since he gave you more substantial comments earlier.
Comment 27 Kentaro Hara 2012-09-12 14:53:00 PDT
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.
Comment 28 Alec Flett 2012-09-12 15:09:49 PDT
Created attachment 163713 [details]
Patch
Comment 29 Joshua Bell 2012-09-12 15:19:15 PDT
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. :)
Comment 30 Kentaro Hara 2012-09-12 15:23:49 PDT
Comment on attachment 163713 [details]
Patch

Please just fix a nit pointed out by jsbell, before landing.
Comment 31 Alec Flett 2012-09-12 15:25:35 PDT
Created attachment 163718 [details]
Patch
Comment 32 WebKit Review Bot 2012-09-12 16:20:01 PDT
Comment on attachment 163718 [details]
Patch

Clearing flags on attachment: 163718

Committed r128379: <http://trac.webkit.org/changeset/128379>
Comment 33 WebKit Review Bot 2012-09-12 16:20:06 PDT
All reviewed patches have been landed.  Closing bug.