Bug 83093 – [V8] Add a per context data store and use that for caching boiler plates as well as constructor functions

• Component: New Bugs
• Status: RESOLVED FIXED

[Erik Arvidsson]

[V8] Add a per context data store and use that for caching boiler plates as well as constructor functions

[Erik Arvidsson]

Created attachment 135460 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135460 [details]
Patch

Attachment 135460 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12320540

[Adam Barth]

Comment on attachment 135460 [details]
Patch

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

I'm being a bit more picky about naming than usual because the V8 bindings are a style disaster.  I'm hoping that through this sort of work we can improve the style and patterns in the code so that future folks working on this code will have good examples to work from.

One thing that's important is that v8::Persistent<T>::New must always be balanced with v8::Persistent<T>::Dispose.  They're like new and delete in C++.  It's lame that V8 invite us to manually call new and delete.  We have SharedPersistent and OwnHandle (maybe should be called OwnPersistent?) to help us here.  They're not widely used, but it would be good to use these rather than calling New and Dispose manually when convenient.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:47
> +            wrapper.Clear();

I don't think there's any need to call Clear() here.  That just sets the local variable to zero.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:63
> +bool V8BindingPerContextData::installHiddenObjectPrototype()

install probably isn't the right verb for this function anymore now that we're storing the object prototype ourselves (rather than "installing" it into a hidden variable in the context.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:79
> +    m_objectPrototype = v8::Persistent<v8::Value>::New(objectPrototype);

Where is the Dispose that balances this New?  Should we use http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/OwnHandle.h to make sure we don't leak?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:88
> +    v8::Local<v8::Function> function = getConstructor(type);

Is there a better verb we can use than "get" for getConstructor?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:97
> +v8::Local<v8::Function> V8BindingPerContextData::getConstructorSlowCase(WrapperTypeInfo* type)

Another "get".

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:121
> +    // Hotmail fix, see comments above.

Is this comment still needed?  I would remove it and rely on our test coverage to explain to us why this is needed.

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:53
> +    // WARNING: Call |installHiddenObjectPrototype| only on fresh contexts!

This comment is out of date.  Rather than having this comment, perhaps the function should just ASSERT(m_objectPrototype.isEmpty()) ?

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:65
> +    v8::Local<v8::Function> getConstructor(WrapperTypeInfo* type)

Ok, so I would call this function constructorForType

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:70
> +        return getConstructorSlowCase(type);

getConstructorSlowCase => constructorForTypeSlowCase ?

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:75
> +    V8BindingPerContextData(v8::Handle<v8::Context> context)
> +      : m_context(context) { }

Please mark one-argument constructors "explicit".  Also, these { } should be on their own lines.

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:90
> +    v8::Persistent<v8::Context> m_context;

Where are the New() and Dispose() calls for this persistent handle?  Should we use http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/SharedPersistent.h to make sure the context doesn't get Disposed out from under us?

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:83
> +    V8BindingPerContextData* perContextData() { return m_perContextData.get(); }

I was hoping we could get this from a v8::Context itself rather than hanging it off the window shell.  This might be a good intermediate state though.

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:107
> +    if (UNLIKELY(!!(isolatedContext = V8IsolatedContext::getEntered())))
> +        perContextData = isolatedContext->perContextData();
> +    else
> +        perContextData = V8Proxy::retrieve(frame)->windowShell()->perContextData();

Wouldn't this be better if we could get the perContextData directly from the v8::Context?

> Source/WebCore/bindings/v8/V8DOMWrapper.h:152
> +        static V8BindingPerContextData* getPerContextData(V8Proxy*);
> +#if ENABLE(WORKERS)
> +        static V8BindingPerContextData* getPerContextData(WorkerContext*);
> +#endif

Can we drop the "get" prefix?

> Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:155
> +    if (!m_perContextData->installHiddenObjectPrototype()) {

Why isn't this work done by the constructor?  It seems like everyone calls it immediately.  I guess they'd all need to check for failure...  Maybe this function should just be called init().

[Erik Arvidsson]

Thanks for the detailed comments.

(In reply to comment #3)
> (From update of attachment 135460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135460&action=review
> 
> I'm being a bit more picky about naming than usual because the V8 bindings are a style disaster.  I'm hoping that through this sort of work we can improve the style and patterns in the code so that future folks working on this code will have good examples to work from.

Make sense. I was just trying to keep the change small but I agree that there is a lot of room for improvement here.


> One thing that's important is that v8::Persistent<T>::New must always be balanced with v8::Persistent<T>::Dispose.  They're like new and delete in C++.  It's lame that V8 invite us to manually call new and delete.  We have SharedPersistent and OwnHandle (maybe should be called OwnPersistent?) to help us here.  They're not widely used, but it would be good to use these rather than calling New and Dispose manually when convenient.

Wow, I didn't get this before. I incorrectly assumed v8::Persistent worked like OwnHandle. I don't think we need to rename OwnHandle. It kind of is implied since the handle has to be persistent to be kept alive.
 
> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:63
> > +bool V8BindingPerContextData::installHiddenObjectPrototype()
> 
> install probably isn't the right verb for this function anymore now that we're storing the object prototype ourselves (rather than "installing" it into a hidden variable in the context.

I had it as init at one point but went for smaller change. I was a bit split on the design here. Your vote has convinced me for the init pattern.

> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:79
> > +    m_objectPrototype = v8::Persistent<v8::Value>::New(objectPrototype);
> 
> Where is the Dispose that balances this New?  Should we use http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/OwnHandle.h to make sure we don't leak?

Change to OwnHandle
 
> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:88
> > +    v8::Local<v8::Function> function = getConstructor(type);
> 
> Is there a better verb we can use than "get" for getConstructor?

Changing to constructorForType

> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:121
> > +    // Hotmail fix, see comments above.
> 
> Is this comment still needed?  I would remove it and rely on our test coverage to explain to us why this is needed.

Do you want me to remove the comment and add the comment to the tests?


> > Source/WebCore/bindings/v8/V8DOMWindowShell.h:83
> > +    V8BindingPerContextData* perContextData() { return m_perContextData.get(); }
> 
> I was hoping we could get this from a v8::Context itself rather than hanging it off the window shell.  This might be a good intermediate state though.

That was my initial plan too. However, v8::Context does not have a way to add meta data. v8::Context has a SetData but it takes a v8::String which is used for debugging. v8::Isolate has a SetData(void*) and v8::Object has internal properties. Maybe the right thing would be to set this on the global object of the context?

> 
> > Source/WebCore/bindings/v8/V8DOMWrapper.cpp:107
> > +    if (UNLIKELY(!!(isolatedContext = V8IsolatedContext::getEntered())))
> > +        perContextData = isolatedContext->perContextData();
> > +    else
> > +        perContextData = V8Proxy::retrieve(frame)->windowShell()->perContextData();
> 
> Wouldn't this be better if we could get the perContextData directly from the v8::Context?

I'll try and see what it ends up like.

> > Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:155
> > +    if (!m_perContextData->installHiddenObjectPrototype()) {
> 
> Why isn't this work done by the constructor?  It seems like everyone calls it immediately.  I guess they'd all need to check for failure...  Maybe this function should just be called init().

The reason it is not done in the constructor was because it might fail. I renamed it to init()

[Adam Barth]

> Do you want me to remove the comment and add the comment to the tests?

That would be great.

[Erik Arvidsson]

Created attachment 135695 [details]
Patch

[Adam Barth]

Comment on attachment 135695 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:746
> -    return V8DOMWrapper::getConstructor(type, V8DOMWindow::toNative(info.Holder()));
> +    return V8DOMWrapper::constructorForType(type, V8DOMWindow::toNative(info.Holder()));

Make sure to run-bindings-tests --reset-results before landing.

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:75
> +      : m_context(context)

Four-space indent.

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:76
> +    { }

nit: Technically each of { and } should be on its own lines.

> Source/WebCore/bindings/v8/V8BindingPerContextData.h:91
> +    v8::Handle<v8::Context> m_context;

I'm unclear who is responsible for keeping this handle alive.  This needs to be a v8::Persistent handle because this object outlives the current HandleScope.  Is there a reason not to use SharedPersistent ?

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:104
>      v8::Persistent<v8::Context> m_context;

I see.  This is where we have the strong reference to the context.  Hum...  So, I think this is ok, but not ideal.  It's like having two raw pointers to one piece of memory and having one of the folks be in charge of deleting it.  We can take this approach in this patch and add more automated memory management in a later patch.

[Erik Arvidsson]

Committed r113250: <http://trac.webkit.org/changeset/113250>