For the origin of the change: https://bugs.webkit.org/show_bug.cgi?id=41372#c18 For concerns with the implementation, noted while creating a similar method: https://bugs.webkit.org/show_bug.cgi?id=60197#c13 Short version - this pattern does not look threadsafe: Source/WebCore/binding/v8/SerializedScriptValue.cpp(1932): > SerializedScriptValue* SerializedScriptValue::nullValue() > { > DEFINE_STATIC_LOCAL(RefPtr<SerializedScriptValue>, nullValue, (0)); > if (!nullValue) { > Writer writer; > writer.writeNull(); > String wireData = StringImpl::adopt(writer.data()); > nullValue = adoptRef(new SerializedScriptValue(wireData)); > } > return nullValue.get(); > } If this will be called from multiple threads, caching should move to the callers. If it will not be called from multiple threads there should be an assertion. Since SSV is used from Workers, the former is more likely to be true.
(In reply to comment #0) > For the origin of the change: > https://bugs.webkit.org/show_bug.cgi?id=41372#c18 > > For concerns with the implementation, noted while creating a similar method: > https://bugs.webkit.org/show_bug.cgi?id=60197#c13 > > Short version - this pattern does not look threadsafe: > > Source/WebCore/binding/v8/SerializedScriptValue.cpp(1932): > > SerializedScriptValue* SerializedScriptValue::nullValue() > > { > > DEFINE_STATIC_LOCAL(RefPtr<SerializedScriptValue>, nullValue, (0)); > > if (!nullValue) { > > Writer writer; > > writer.writeNull(); > > String wireData = StringImpl::adopt(writer.data()); > > nullValue = adoptRef(new SerializedScriptValue(wireData)); > > } > > return nullValue.get(); > > } > > If this will be called from multiple threads, caching should move to the callers. If it will not be called from multiple threads there should be an assertion. Since SSV is used from Workers, the former is more likely to be true. But I don't think this method is used from Workers (At least not by postMessage -- I have no idea about idb).
(In reply to comment #1) > > But I don't think this method is used from Workers (At least not by postMessage -- I have no idea about idb). SerializedScriptValue::nullValue() is used by IDB and Worker support for IDB is being added by dgrogan.
Guys, I believe Vitaly is an initial implementer and he can provide a lot of details. I do agree that caching looks somewhat fishy. I'd rather drop it for now and reintroduce in thread safe manner later if need be. Vitaly, any thoughts?
Created attachment 140363 [details] Patch
IndexedDB is only using this method from one thread at the moment (even with workers), but that may change soon. ojan@ - any idea who would be good to review the dom/Document change in this, and/or evaluate the need for caching in call sites?
(In reply to comment #5) > ojan@ - any idea who would be good to review the dom/Document change in this, and/or evaluate the need for caching in call sites? Best is to look at the blame for the code in question and add whoever modified/reviewed the most recent changes. At first glance, this change looks fine, but the nullValue thing needs explanation from someone who knows why the code was written that way to begin with.
I followed up with Vitaly - he doesn't recall any performance reason for the caching. It looks like the source of the non-threadsafe change is: http://trac.webkit.org/changeset/77558/trunk/Source/WebCore/bindings/v8/SerializedScriptValue.cpp The old code initialized the value inside the DEFINE_STATIC_LOCAL() macro. The new code wanted do more than a single call, so initialized the static to null then used an (unsafe) if (!nullValue) { ... } block to do the initialization. An alternative to my patch would be to introduce a createNull() method that is non-threadsafe and call it from DEFINE_STATIC_LOCAL(). However, caching could just as easily move to the callers. haraken@ - could you review the change?
Comment on attachment 140363 [details] Patch Let me r+ it based on the following observations: 1. "I followed up with Vitaly - he doesn't recall any performance reason for the caching." 2. booleanValue() and numberValue() are not caching the values. If there were any performance concern, it would be best to cache the value in V8BindingsPerIsolateData (You can access V8BindingsPerIsolateData just by isolate->GetData()).
Comment on attachment 140363 [details] Patch Clearing flags on attachment: 140363 Committed r117377: <http://trac.webkit.org/changeset/117377>
All reviewed patches have been landed. Closing bug.