Need to implement IDBKeyPathBackendImpl::createIDBKeysFromSerializedValuesAndKeyPath(), IDBKeyPathBackendImpl::injectIDBKeyIntoSerializedValue() for JSC binding.
Created attachment 148289 [details] Patch
Comment on attachment 148289 [details] Patch Attachment 148289 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12974686
Comment on attachment 148289 [details] Patch Sorry, my local repository is a bit out of date. I will rebase to the latest code and will submit a new patch.
Created attachment 148320 [details] Patch
Comment on attachment 148320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148320&action=review > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:43 > + m_context = JSGlobalContextCreate(0); Just an aside, context creation ended up being expensive in V8 so we recycle a utility context across the IDBKey create/inject calls. Not urgent, but you may want to profile to see if that'll be a performance issue for JSC. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:74 > +void createIDBKeysFromSerializedScriptValuesAndKeyPath(const Vector<RefPtr<SerializedScriptValue>, 0>& values, const IDBKeyPath& keyPath, Vector<RefPtr<IDBKey>, 0>& keys) You may have noticed the multiple values/multiple keys. I don't think that functionality of the API is being used at the moment, but we may start to. In the Chromium port this is often run in a different process, and for bulk operations (like creating an index with an already populated store) we will want to avoid the IPC overhead when iterating over the entire store, so we could use this to compute the index keys in batches. Basically, there's a lot of cleanup work to do. :) Keep these patches coming, it's awesome. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:118 > + // FIXME: we might need to support ArrayType The only key type that injectXXX() needs to support is NumberType (as a result of using a key generator and key path). You can safely ASSERT on that; we should be able to simplify the V8 version too. Support for other types in this API probably dates from earlier versions of the spec.
Comment on attachment 148320 [details] Patch You shouldn't be using a temporary context -- why are we serializing to one form when we want another? Alternatively why are we deserializing? If it's absolutely critical we should either add logic to deserialize directly to IDB types, or not be serializing to an unrelated format in the first place.
(In reply to comment #6) > (From update of attachment 148320 [details]) > You shouldn't be using a temporary context -- why are we serializing to one form when we want another? Alternatively why are we deserializing? > > If it's absolutely critical we should either add logic to deserialize directly to IDB types, or not be serializing to an unrelated format in the first place. Oliver: thanks for the comments. Let me explain why this design -- using a temporary context to deserialize the SerializedScriptValue to JSValue. 1. For IndexedDB, we need to access a certain property of an object before putting to the database or after we read from the database. We need to do that again the SerializedScriptValue. If you doubt the justification of this, the existing code in Modules/indexeddb is expecting for that. 2. To do that , there are multiple-ways. 2.1 -- directly parsing and manipulating the SerializedScriptValue, to get property of the object, or to inject a new property and value to the object. Theoratically this is possible, but a) It's a lot of coding; b) we need to invent another data structure to represent the object; c) we are not re-using the JSValue (JSObject) and the serialization/deserialization code between JS <==> SerializedScriptValue. 2.2 -- SerializedScriptValue is just a string representation of an JSObject for cross-boder passing (cross-process, persistence, etc.), it's not good at object property accessing. But it can be easily converted to/from an JSValue/JSObject, which has very easy way to manipulate the object properties. So it's good to deserialize it to a temporary JSValue/JSObject for property manipulation and then convert back to a SerializedScriptValue. To deserialize the SerializedScriptValue to JSValue, we need an Javascript Context. There are 2 ways to get an Javascript Context: 2.2.1 --- Re-using the JavaScriptContext that initialized the indexedDB operation. This not only requires a lot of code change to pass the JavaScript Context from class to class, object to object, function to function ( please have a look at bug: 45110, most of which is trying to do this JavaScript Context passing ), but also not work well because it's shared code between JSC and V8 porting, while V8 porting is not using them at all and the change might break Chromium/V8. 2.2.2 --- Create an temporary JavaScript Context, same as V8, which also creates a temporary JavaScript Context for this. This makes little code change, the code looks cleaner and easier to maintain. 2.2.2 is now the approach this patch is pursuing. Oliver: what negative impacts do you see with a temporary JavaScript Context except potential performance hits pointed out at comment #5, which I am going to make the temporary JavaScript context recyclable among IndexedDB operations?
Created attachment 148516 [details] Patch
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 148320 [details] [details]) > > You shouldn't be using a temporary context -- why are we serializing to one form when we want another? Alternatively why are we deserializing? > > > > If it's absolutely critical we should either add logic to deserialize directly to IDB types, or not be serializing to an unrelated format in the first place. > > Oliver: thanks for the comments. > > Let me explain why this design -- using a temporary context to deserialize the SerializedScriptValue to JSValue. > > 1. For IndexedDB, we need to access a certain property of an object before putting to the database or after we read from the database. We need to do that again the SerializedScriptValue. If you doubt the justification of this, the existing code in Modules/indexeddb is expecting for that. > Can you clarify this? What property are you trying to access? Why can't you just access it off of the unserialised object? serialising and then deserialising an entire object just to access a single property could be horrendously painful, so why aren't we just reading off the deserialised object (on read) or the unserialised object (for writes)? That's what is confusing me. I agree whole heartedly that you don't want to try and do individual property reads from the serialised form.
> > > > 1. For IndexedDB, we need to access a certain property of an object before putting to the database or after we read from the database. We need to do that again the SerializedScriptValue. If you doubt the justification of this, the existing code in Modules/indexeddb is expecting for that. > > > Can you clarify this? What property are you trying to access? Why can't you just access it off of the unserialised object? serialising and then deserialising an entire object just to access a single property could be horrendously painful, so why aren't we just reading off the deserialised object (on read) or the unserialised object (for writes)? > > That's what is confusing me. > > I agree whole heartedly that you don't want to try and do individual property reads from the serialised form. Oliver: The IDBObjectStore.idl definition explains this: interface IDBObjectStore { readonly attribute [TreatReturnedNullStringAs=Null] DOMString keyPath; [CallWith=ScriptExecutionContext] IDBRequest put(in SerializedScriptValue value, in [Optional] IDBKey key) raises (IDBDatabaseException); [CallWith=ScriptExecutionContext] IDBRequest add(in SerializedScriptValue value, in [Optional] IDBKey key) raises (IDBDatabaseException); ...... }; From which you can see, the put() and add() methods ( put or add a record to the indexedDB) are taking SerializedScriptValue as inputs, that means, the JSValue will be automatically converted to the native SerializeScriptValue, and all following operations (putting to the database) are operated against this SerializedScriptValue. Before putting to the database, we might need to get the key from the SerializedScriptValue, the key is the property indicated by the keyPath of the object. That's why we need to access the property of an object in SerializedScriptValue format. Joshua Bell might be able to give a more convincible explanation since they designed the IndexedDB initially for V8.
(In reply to comment #10) > > > > > > 1. For IndexedDB, we need to access a certain property of an object before putting to the database or after we read from the database. We need to do that again the SerializedScriptValue. If you doubt the justification of this, the existing code in Modules/indexeddb is expecting for that. > > > > > Can you clarify this? What property are you trying to access? Why can't you just access it off of the unserialised object? serialising and then deserialising an entire object just to access a single property could be horrendously painful, so why aren't we just reading off the deserialised object (on read) or the unserialised object (for writes)? > > > > That's what is confusing me. > > > > I agree whole heartedly that you don't want to try and do individual property reads from the serialised form. > > > Oliver: The IDBObjectStore.idl definition explains this: > > interface IDBObjectStore { > > readonly attribute [TreatReturnedNullStringAs=Null] DOMString keyPath; > > [CallWith=ScriptExecutionContext] IDBRequest put(in SerializedScriptValue value, in [Optional] IDBKey key) raises (IDBDatabaseException); > [CallWith=ScriptExecutionContext] IDBRequest add(in SerializedScriptValue value, in [Optional] IDBKey key) raises (IDBDatabaseException); > ...... > }; > > From which you can see, the put() and add() methods ( put or add a record to the indexedDB) are taking SerializedScriptValue as inputs, that means, the JSValue will be automatically converted to the native SerializeScriptValue, and all following operations (putting to the database) are operated against this SerializedScriptValue. Before putting to the database, we might need to get the key from the SerializedScriptValue, the key is the property indicated by the keyPath of the object. That's why we need to access the property of an object in SerializedScriptValue format. > > Joshua Bell might be able to give a more convincible explanation since they designed the IndexedDB initially for V8. I think a better interface would be to take any rather than serialized value. IIRC that should give you a ScriptValue. You can then introspect the ScriptValue (ask for properties, etc) without the need for the [de]serialise shenanigans, and only serialise once you go to store the value.
. > > I think a better interface would be to take any rather than serialized value. > > IIRC that should give you a ScriptValue. > > You can then introspect the ScriptValue (ask for properties, etc) without the need for the [de]serialise shenanigans, and only serialise once you go to store the value. That seems a fundamental Change,and requires almost re-write of Modules/indexeddb, which is centered around SerializedScriptValue instead of ScriptValue. Further, will replace SerialiedScriptValue with ScriptValue will make things better ? Can we easily access attributes of a ScriptValue object without converting to/from JSValue ? I am looking at Chromium guys who initially implemented indexeddb for v8 binding. We need a consensus on this.
(In reply to comment #12) > . > > > > I think a better interface would be to take any rather than serialized value. > > > > IIRC that should give you a ScriptValue. > > > > You can then introspect the ScriptValue (ask for properties, etc) without the need for the [de]serialise shenanigans, and only serialise once you go to store the value. > > That seems a fundamental Change,and requires almost re-write of Modules/indexeddb, which is centered around SerializedScriptValue instead of ScriptValue. Further, will replace SerialiedScriptValue with ScriptValue will make things better ? Can we easily access attributes of a ScriptValue object without converting to/from JSValue ? > > I am looking at Chromium guys who initially implemented indexeddb for v8 binding. We need a consensus on this. ScriptValue (and ScriptObject) are WebCore's abstraction over the JS engine's value types. There is no conversion cost.
(In reply to comment #13) > (In reply to comment #12) > > . > > > > > > I think a better interface would be to take any rather than serialized value. > > > > > > IIRC that should give you a ScriptValue. > > > > > > You can then introspect the ScriptValue (ask for properties, etc) without the need for the [de]serialise shenanigans, and only serialise once you go to store the value. > > > > That seems a fundamental Change,and requires almost re-write of Modules/indexeddb, which is centered around SerializedScriptValue instead of ScriptValue. Further, will replace SerialiedScriptValue with ScriptValue will make things better ? Can we easily access attributes of a ScriptValue object without converting to/from JSValue ? > > > > I am looking at Chromium guys who initially implemented indexeddb for v8 binding. We need a consensus on this. > > ScriptValue (and ScriptObject) are WebCore's abstraction over the JS engine's value types. There is no conversion cost. Ok, but does that justify re-write the whole indexedDB code?
(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > . > > > > > > > > I think a better interface would be to take any rather than serialized value. > > > > > > > > IIRC that should give you a ScriptValue. > > > > > > > > You can then introspect the ScriptValue (ask for properties, etc) without the need for the [de]serialise shenanigans, and only serialise once you go to store the value. > > > > > > That seems a fundamental Change,and requires almost re-write of Modules/indexeddb, which is centered around SerializedScriptValue instead of ScriptValue. Further, will replace SerialiedScriptValue with ScriptValue will make things better ? Can we easily access attributes of a ScriptValue object without converting to/from JSValue ? > > > > > > I am looking at Chromium guys who initially implemented indexeddb for v8 binding. We need a consensus on this. > > > > ScriptValue (and ScriptObject) are WebCore's abstraction over the JS engine's value types. There is no conversion cost. > > Ok, but does that justify re-write the whole indexedDB code? 1) If a feature has been implemented with a bad design, we should not be worrying about the annoyance of re-implementing it. 2) You keep saying that IDB is built entirely around SerializedScriptValue, but I don't understand how that can be the case -- serializedscriptvalue is not a queryable type. It's intended for serialisation (hence the name), so should only be being used for actual storage and nothing else. What is the usage that makes IDB fundamentally tied to serializedscriptvalue being the in memory object representation?
> > (In reply to comment #13) > > > (In reply to comment #12) > > > > . > > > > > > > > > > I think a better interface would be to take any rather than serialized value. > > > > > > > > > > IIRC that should give you a ScriptValue. > > > > > > > > > > You can then introspect the ScriptValue (ask for properties, etc) without the need for the [de]serialise shenanigans, and only serialise once you go to store the value. > > > > > > > > That seems a fundamental Change,and requires almost re-write of Modules/indexeddb, which is centered around SerializedScriptValue instead of ScriptValue. Further, will replace SerialiedScriptValue with ScriptValue will make things better ? Can we easily access attributes of a ScriptValue object without converting to/from JSValue ? > > > > > > > > I am looking at Chromium guys who initially implemented indexeddb for v8 binding. We need a consensus on this. > > > > > > ScriptValue (and ScriptObject) are WebCore's abstraction over the JS engine's value types. There is no conversion cost. > > > > Ok, but does that justify re-write the whole indexedDB code? > > 1) If a feature has been implemented with a bad design, we should not be worrying about the annoyance of re-implementing it. > 2) You keep saying that IDB is built entirely around SerializedScriptValue, but I don't understand how that can be the case -- serializedscriptvalue is not a queryable type. It's intended for serialisation (hence the name), so should only be being used for actual storage and nothing else. What is the usage that makes IDB fundamentally tied to serializedscriptvalue being the in memory object representation? For 1), I agree that a bad-designed feature should not worry about re-implementing. I would like to leave that to the original author to comment about the design for using SerializedScriptValue as the input parameter and in-memory representation of the object, instead of ScriptValue. For 2), I am talking about the fact that now idb implementation is build around SerializedScriptValue instead of ScriptValue, I am not saying that idb SHOULD be bond to SerializedScriptValue instead of the other one. This patch tries to adapt to the current design and makes it work for JSC, it's not trying to re-write the design and implementation already there.
(In reply to comment #16) > > For 1), I agree that a bad-designed feature should not worry about re-implementing. I would like to leave that to the original author to comment about the design for using SerializedScriptValue as the input parameter and in-memory representation of the object, instead of ScriptValue. Unfortunately, the original authors are no longer active on the project. I'll try to fill in rationale as I understand it. The original implementation - which was based on very early iterations of the spec - needed to support the multi-process model. This was done by using sqlite as the backing store, and giving each process a handle to the store. This was deemed undesirable for security, performance, and maintainability reasons. So the implementation changed to use leveldb as a backing store, but only one process can speak to leveldb at a time. So the implementation was effectively split into "front end" objects that (in Chromium) live in the renderer processes and "back end" objects that (in Chromium) live in the browser process. The "front end" objects had very little intelligence, and most of the logic implementing the spec was present in the "back end". This meant that as soon as an API call was made from script the object would immediately be serialized for transmission across an IPC boundary to the back end. In some cases, the back end object would need to reason about the object (e.g. "extract a key" given the object and a key path, or "inject a key" given the object and a key path); in the Chromium port this requires other IPC hops. As an aside, the data stored in the backing store is a serialization of the script object. The IDB spec requires support for storing objects following the rules of the HTML5 "structured clone algorithm"; handily, these match (although Blobs are extra work and as yet unsupported in WebKit IDB) These "key" objects are a dramatic subset of what is permitted by script (number, string, date, or array-of key), and are stored and used independently of script objects (e.g. when walking over indexes to analyze keys, even without extracting the objects they refer to). Keys arise in two main places - explicitly passed from script, e.g. IDBObjectStore.get(key), where the WebIDL does indeed specify "any", and implicitly e.g. IDBObjectStore.put(object) where the object store has a "key path". The evaluation of the key path with the script object may occur directly during the API call, or may occur later and asynchronously - e.g. if the IDBObjectStore has an IDBIndex with a key path, then the key path is evaluated when the put() task is executed by the transaction to determine the index key. The cases where keys are explicitly passed in is currently handled by the binding layer, with WebKit IDL specifying IDBKey (a construct that does not exist in the spec). Custom binding logic is used to do the V8Value->IDBKey conversion. Although I'm not familiar with ScriptValue/ScriptObject, it sounds like this could be changed so that the IDL/binding layer specifies "any" and the value->IDBKey conversion is done at in the method instead. I'm agnostic to that change. However, it wouldn't change the need for the conversion functions, or obviate the use of SerializedScriptValue elsewhere in the current implementation. > For 2), I am talking about the fact that now idb implementation is build around SerializedScriptValue instead of ScriptValue, I am not saying that idb SHOULD be bond to SerializedScriptValue instead of the other one. > > This patch tries to adapt to the current design and makes it work for JSC, it's not trying to re-write the design and implementation already there. IMHO that's the right way to go. Those of us working on IDB with an eye towards the Chromium port are working on two fronts at the moment: highest priority is to get the implementation up to match the spec (our implementation was first, so has many old crufty bits that changed in the spec) so we're not introducing incompatibility to the Web; second priority is to untangle the "front end" / "back end" design specified above. It turns out that with an IPC jump in the middle that making changes to the code is incredibly time consuming (multi-sided patches) and performance is poor. We hope to get the WebKit implementation to the point where nearly all spec-specific logic resides in the "front end" and the "back end" is merely an async caching/write-batch system for arbitrary binary key/value pairs. This is a huge refactor - and especially exciting while other ports are attempting to implement the code, so we can't break them. It sounds like a move from SSV to SV would be most feasible once we're further along in this refactor, when the IDB spec logic isn't straddling an IPC barrier that requires SSVs in the middle.
(In reply to comment #17) > (In reply to comment #16) > > > > For 1), I agree that a bad-designed feature should not worry about re-implementing. I would like to leave that to the original author to comment about the design for using SerializedScriptValue as the input parameter and in-memory representation of the object, instead of ScriptValue. > > Unfortunately, the original authors are no longer active on the project. I'll try to fill in rationale as I understand it. > > The original implementation - which was based on very early iterations of the spec - needed to support the multi-process model. This was done by using sqlite as the backing store, and giving each process a handle to the store. This was deemed undesirable for security, performance, and maintainability reasons. So the implementation changed to use leveldb as a backing store, but only one process can speak to leveldb at a time. So the implementation was effectively split into "front end" objects that (in Chromium) live in the renderer processes and "back end" objects that (in Chromium) live in the browser process. The "front end" objects had very little intelligence, and most of the logic implementing the spec was present in the "back end". > > This meant that as soon as an API call was made from script the object would immediately be serialized for transmission across an IPC boundary to the back end. In some cases, the back end object would need to reason about the object (e.g. "extract a key" given the object and a key path, or "inject a key" given the object and a key path); in the Chromium port this requires other IPC hops. > > As an aside, the data stored in the backing store is a serialization of the script object. The IDB spec requires support for storing objects following the rules of the HTML5 "structured clone algorithm"; handily, these match (although Blobs are extra work and as yet unsupported in WebKit IDB) > > These "key" objects are a dramatic subset of what is permitted by script (number, string, date, or array-of key), and are stored and used independently of script objects (e.g. when walking over indexes to analyze keys, even without extracting the objects they refer to). > > Keys arise in two main places - explicitly passed from script, e.g. IDBObjectStore.get(key), where the WebIDL does indeed specify "any", and implicitly e.g. IDBObjectStore.put(object) where the object store has a "key path". The evaluation of the key path with the script object may occur directly during the API call, or may occur later and asynchronously - e.g. if the IDBObjectStore has an IDBIndex with a key path, then the key path is evaluated when the put() task is executed by the transaction to determine the index key. > > The cases where keys are explicitly passed in is currently handled by the binding layer, with WebKit IDL specifying IDBKey (a construct that does not exist in the spec). Custom binding logic is used to do the V8Value->IDBKey conversion. > > Although I'm not familiar with ScriptValue/ScriptObject, it sounds like this could be changed so that the IDL/binding layer specifies "any" and the value->IDBKey conversion is done at in the method instead. I'm agnostic to that change. However, it wouldn't change the need for the conversion functions, or obviate the use of SerializedScriptValue elsewhere in the current implementation. > > > For 2), I am talking about the fact that now idb implementation is build around SerializedScriptValue instead of ScriptValue, I am not saying that idb SHOULD be bond to SerializedScriptValue instead of the other one. > > > > This patch tries to adapt to the current design and makes it work for JSC, it's not trying to re-write the design and implementation already there. > > IMHO that's the right way to go. > > Those of us working on IDB with an eye towards the Chromium port are working on two fronts at the moment: highest priority is to get the implementation up to match the spec (our implementation was first, so has many old crufty bits that changed in the spec) so we're not introducing incompatibility to the Web; second priority is to untangle the "front end" / "back end" design specified above. > > It turns out that with an IPC jump in the middle that making changes to the code is incredibly time consuming (multi-sided patches) and performance is poor. We hope to get the WebKit implementation to the point where nearly all spec-specific logic resides in the "front end" and the "back end" is merely an async caching/write-batch system for arbitrary binary key/value pairs. This is a huge refactor - and especially exciting while other ports are attempting to implement the code, so we can't break them. It sounds like a move from SSV to SV would be most feasible once we're further along in this refactor, when the IDB spec logic isn't straddling an IPC barrier that requires SSVs in the middle. Joshua, thanks for the detailed explanation of the design and implementation background of IDB. Oliver: Any comments on this ?
Anyone can comment how to proceed ?
FYI, added https://bugs.webkit.org/show_bug.cgi?id=94023 to track moving the IDB code from using SerializedScriptValue everywhere to using ScriptValue (etc) where possible, marked it as blocking this issue. I still think it would be okay to land this as-is then do the rework in 94023, even though it will likely obsolete much of this patch.
(In reply to comment #20) > FYI, added https://bugs.webkit.org/show_bug.cgi?id=94023 to track moving the IDB code from using SerializedScriptValue everywhere to using ScriptValue (etc) where possible, marked it as blocking this issue. > > I still think it would be okay to land this as-is then do the rework in 94023, even though it will likely obsolete much of this patch. Thanks for pushing for this, Joshua, and good to see that you filed 94023 to move SSV to SV as the data in the IDB code. I agree we can push this for now while 94023 will try to overwrite some of the code.
we're almost ready for this - plus there is no more IDBKeyPathBackendImpl. The only thing left is some of the IDBBindingUtilities stuff. hold off any work there until that is implemented in bug 95409
Charles, we are ready to go on this - IDB has been totally converted over. Some of the test cases that came out of this were interesting though - in particular your port has to make sure that you use a consistent serialization format. JSC has one, but I don't know if it's an issue that it's likely not (currently) compatible with the V8 one.
After Bug 88287 fix, I'll continue.
jiyeon: this bug, as it is described, isn't quite what you need to implement now. Instead, you need to implement JSC versions of the methods described in Source/WebCore/bindings/v8/IDBBindingUtilities.h
(In reply to comment #25) > jiyeon: this bug, as it is described, isn't quite what you need to implement now. > Instead, you need to implement JSC versions of the methods described in Source/WebCore/bindings/v8/IDBBindingUtilities.h Sorry Alec for confusing. I have to write #24 comment in 92832 bugs. I just waiting finish this patch. After this patch finish, I'll continue 92828, 92832 bugs. You can ignore my comment #24.
I think this bug is now invalid - all the other stuff mentioned here was implemented in bug 103484, and sounds like details are being worked out in bug 96614 *** This bug has been marked as a duplicate of bug 96614 ***
Comment on attachment 148516 [details] Patch Cleared review? from attachment 148516 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).