[webkit-reviews] review denied: [Bug 83526] [V8] IndexedDB: Cursor value modifications should be preserved until cursor iterates : [Attachment 146942] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Alec Flett
<alecflett at chromium.org>'s request for review:
Bug 83526: [V8] IndexedDB: Cursor value modifications should be preserved until
cursor iterates
https://bugs.webkit.org/show_bug.cgi?id=83526

Attachment 146942: Patch
https://bugs.webkit.org/attachment.cgi?id=146942&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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;