The IndexedDB spec http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html sayeth, re: IDBCursorWithValue "Returns the cursor's current value. Note that if this property returns an object, it returns the same object instance every time it is inspected, until the cursor's value is changed. This means that if the object is modified, those modifications will be seen by anyone inspecting the value of the cursor. However modifying such an object does not modify the contents of the database." The WebKit implementation does not currently support the "same object instance every time" semantics, meaning the following test would fail: // assume cursor.value = {a: 1, b: 2} cursor.value.a = 3; delete cursor.value.b; cursor.value.foo = "bar"; assert(cursor.value.a === 3); assert(cursor.value.b === undefined); assert(cursor.value.foo === "bar"); It may be possible to do this via [CachedAttribute] or, since IDBAny already has a custom V8 binding, with custom caching code. Or possible via an IDL attribute on IDBCursorWithValue that ensure the same IDBAny is returned each time?
Created attachment 136345 [details] Layout test for spec behavior
This is pesky. I thought it might be as simple as changing IDBCursorWithValue.idl - readonly attribute IDBAny value; + readonly attribute [CachedAttribute] SerializedScriptValue value; (plus appropriate .h/.cpp updates) but this is insufficient since the same IDBCursor object is returned across continue() calls and so the cached value gets re-used incorrectly. FWIW, if the above is done the autogenerated code (for V8) looks like: static v8::Handle<v8::Value> valueAttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info) { INC_STATS("DOM.IDBCursorWithValue.value._get"); v8::Handle<v8::String> propertyName = v8::String::NewSymbol("value"); v8::Handle<v8::Value> value = info.Holder()->GetHiddenValue(propertyName); if (!value.IsEmpty()) return value; IDBCursorWithValue* imp = V8IDBCursorWithValue::toNative(info.Holder()); SerializedScriptValue* serialized = imp->value(); value = serialized ? serialized->deserialize() : v8::Handle<v8::Value>(v8::Null(info.GetIsolate())); info.Holder()->SetHiddenValue(propertyName, value); return value; } There's no check that the SSV hasn't changed, nor a way to invalidate this cache outside the binding layer. Bleah. The V8IDBAnyCustom.cpp implementation of toV8 unfortunately doesn't get passed the AccessorInfo object so there's no place for it to stash a value. We may need to do custom binding for IDBCursorWithValue
(In reply to comment #0) > "Returns the cursor's current value. Note that if this property returns an object, it returns the same object instance every time it is inspected, until the cursor's value is changed. This means that if the object is modified, those modifications will be seen by anyone inspecting the value of the cursor. However modifying such an object does not modify the contents of the database." > > The WebKit implementation does not currently support the "same object instance every time" semantics I thought Hans added this for prefetching and that cursor-inconsistency.html tested it?
(In reply to comment #3) > (In reply to comment #0) > > "Returns the cursor's current value. Note that if this property returns an object, it returns the same object instance every time it is inspected, until the cursor's value is changed. This means that if the object is modified, those modifications will be seen by anyone inspecting the value of the cursor. However modifying such an object does not modify the contents of the database." > > > > The WebKit implementation does not currently support the "same object instance every time" semantics > > I thought Hans added this for prefetching and that cursor-inconsistency.html tested it? Similar but subtly different: the same IDBCursor is indeed returned each time (i.e. request.result), but the cursor's value (e.g. request.result.value) is deserialized fresh from the SSV on each property access.
I think I have a fix, I'll use the test here.
Created attachment 146900 [details] Patch
jsbell@ / dgrogan@ - quick review?
Comment on attachment 146900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146900&action=review > Source/WebCore/ChangeLog:8 > + Cache the 'value' attribute of IDBCursorWithValue with policy Should mention in the ChangeLog that this is explicit in the spec. > Source/WebCore/Modules/indexeddb/IDBCursor.h:86 > + bool valueIsDirty() { return m_valueIsDirty; } I think this method (or the field) merits a comment, since it's not obvious why this would exist and subtle in the spec. > Source/WebCore/Modules/indexeddb/IDBCursorWithValue.idl:31 > + readonly attribute [V8CustomGetter] IDBAny value; Would it simplify the code to change IDBAny here to SerializedScriptValue? > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:28 > + LOG_ERROR(".value => is dirty? %d", imp->valueIsDirty()); Remove LOG_ERROR calls here and below. > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:37 > + // either value is dirty, or the hidden value is empty Can remove this comment, it's clear from the code. > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:39 > + v8::Handle<v8::Value> wrapper = result.get() ? getDOMObjectMap(info.GetIsolate()).get(result.get()) : v8::Handle<v8::Object>(); Definitely get a V8/bindings expert to review this bit.
Created attachment 146933 [details] Patch
Comment on attachment 146933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146933&action=review > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:1 > + Copyright header? (Some but not all of the files in bindings/v8 appear to have them, I note.)
* I don't think it simplifies much to use SerializedScriptValue - just changes the transformation slightly, possibly making IDBCursor.cpp more complex in the process. haraken@ - I wrote this custom binding as a sort of merge of the standard binding code and the CachedAttribute code. I think I likely have one extra layer of caching in here but I don't understand this completely.. can you take a look at IDBCustomBindings.cpp in this patch? Do I need all the goop calling getDOMObjectMap() and such? (And the final patch will need to update the other build systems too, I will get to that before webkit-review)
Created attachment 146941 [details] Patch
Created attachment 146942 [details] Patch
Comment on attachment 146942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146942&action=review At a high level question: Does the cache really improve performance? We need to care that GetHiddenValue() and SetHiddenValue() are not so light. In case of SSV, since the serialization overhead is large, the cache management cost will pay. On the other hand, in case of 'value', I am not sure if the cache management cost pays or not. Did you confirm any performance gain in some benchmark (artificial one is OK)? > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:14 > + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY Please use the correct copyright of Google. > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:41 > +#include "ContextEnabledFeatures.h" > +#include "RuntimeEnabledFeatures.h" > +#include "V8Binding.h" > +#include "V8BindingState.h" > +#include "V8DOMWrapper.h" > +#include "V8HiddenPropertyName.h" > +#include "V8IDBAny.h" > +#include "V8IDBCursor.h" > +#include "V8IsolatedContext.h" > +#include "V8Proxy.h" > +#include <wtf/UnusedParam.h> It seems that unused headers are included. e.g. "ContextEnabledFeatures.h", "RuntimeEnabledFeatures.h", etc. Please check each of them if it is needed or not. > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:46 > + const v8::AccessorInfo& info) Nit: you do not need to wrap the code. > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:65 > + RefPtr<IDBAny> result = imp->value(); > + v8::Handle<v8::Value> wrapper = result.get() ? getDOMObjectMap(info.GetIsolate()).get(result.get()) : v8::Handle<v8::Object>(); > + if (wrapper.IsEmpty()) { > + wrapper = toV8(result.get(), info.GetIsolate()); > + if (!wrapper.IsEmpty()) > + V8DOMWrapper::setNamedHiddenReference(info.Holder(), "value", wrapper); > + } > + info.Holder()->SetHiddenValue(propertyName, wrapper); getDOMObjectMap() and setNamedHiddenReference() are used for keeping the lifetime of 'wrapper' over GC, which is sometimes required by semantics. But in this case, you just want to cache the value for performance (i.e. cache miss won't violate semantics), and you do not need to care the lifetime management. So I think that this code can be: RefPtr<IDBAny> result = imp->value(); v8::Handle<v8::Value> wrapper = toV8(result.get(), info.GetIsolate()); info.Holder()->SetHiddenValue(propertyName, wrapper); return wrapper;
Created attachment 146974 [details] Patch
Comment on attachment 146974 [details] Patch kentaro@ - thanks - this isn't for performance, this is for spec compliance - like you I suspect this will have a negligible effect on performance. We actually need the JS object to survive mutations during the lifetime of the attribute value. specifically imagine code like this: cursor.value.foo = "bar"; ... if (cursor.value.foo == "bar") { ....} cursor.advance() // changes the value of cursor.value here if (cursor.value.foo == "bar") { ... } New patch is up. r? cq?
[I just realized I still need to do XCode project files.. ]
Comment on attachment 146974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146974&action=review > this isn't for performance, this is for spec compliance I am sorry! I should have read your ChangeLog carefully... For now let me mark r- due to missing necessary tests to confirm the change. > Source/WebCore/ChangeLog:9 > + determined by IDBCursor.cpp, to follow spec behavior of keeping a Link to the spec please. > Source/WebCore/ChangeLog:12 > + Test: storage/indexeddb/cursor-value.html Would you add the following test cases? (Case 1) cursor.value.foo = "bar"; shouldBe('cursor.value.foo', ...); cursor.advance(); shouldBe('cursor.value.foo', ...); (Case 2) cursor.value.foo = "bar"; shouldBe('cursor.value.foo', ...); gc(); shouldBe('cursor.value.foo', ...); > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:2 > + * Copyright (C) 2011, Google Inc. All rights reserved. 2011 => 2012 > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:13 > + * THIS SOFTWARE IS PROVIDED BY GOOGLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND Please do not change the line manually. Please just copy the whole copyright from bindings/v8/custom/V8TrackEventCustom.cpp, and just change "2011" to "2012". > Source/WebCore/bindings/v8/IDBCustomBindings.cpp:50 > + IDBCursorWithValue* imp = V8IDBCursorWithValue::toNative(info.Holder()); > + v8::Handle<v8::String> propertyName = v8::String::NewSymbol("value"); > + if (!imp->valueIsDirty()) { > + v8::Handle<v8::Value> value = info.Holder()->GetHiddenValue(propertyName); > + if (!value.IsEmpty()) > + return value; > + } > + > + RefPtr<IDBAny> result = imp->value(); > + v8::Handle<v8::Value> wrapper = toV8(result.get(), info.GetIsolate()); > + info.Holder()->SetHiddenValue(propertyName, wrapper); > + return wrapper; Confirmation: The semantics you need is: - If the value is not yet invalidated, return the same wrapper object keeping JavaScript properties. - If the value is already invalidated, create a new wrapper object. Is my understanding correct? If so, the code looks correct.
Created attachment 147113 [details] Patch
Comment on attachment 147113 [details] Patch kentaro@ - all issues addressed - cursor advancing/continuing is async, so the tests can be awkward, but I handled both cases, and rolled gc'ing into each case (to be fair I copied that copyright notice in whole from another file, sounds like I should fix that separately) in any case, r? cq?
Comment on attachment 147113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147113&action=review Looks OK except for nits. > Source/WebCore/WebCore.gypi:1884 > + 'bindings/v8/IDBCustomBindings.cpp', Is this correct? IDBCustomBindings.cpp is added twice. > LayoutTests/storage/indexeddb/resources/cursor-value.js:56 > + index += 2; > + } else if (index == 3) { Nit: why are you using index+=2 instead of index++?
Created attachment 147350 [details] Patch for landing
jsbell / dgrogan - cq?
haraken: the += 2 was just to keep the index in sync with the actual index of the items we're iterating, since we're calling advance(2) - it's in sync mainly for test readability/maintenance
Comment on attachment 147350 [details] Patch for landing Clearing flags on attachment: 147350 Committed r120241: <http://trac.webkit.org/changeset/120241>
All reviewed patches have been landed. Closing bug.