Bug 30599 – V8 binding optimizations (redundant refs, unlikely errors)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jens Alfke]

Some optimization opportunities in the V8/WebCore bindings:

1. There are redundant ref/deref calls in bindings that return objects, when the native method returns a direct pointer (e.g. Node*) but the binding code stores it temporarily in a RefPtr before returning it.
2. Since exceptions are unlikely, the calls to V8Proxy::setDOMException should be conditional (some of them weren't) and should use the UNLIKELY() macro to move the exception-handling block out of the main flow of control.

In opportunity 1 we don't know if the return type of the native method is X* or PassRefPtr<X>. So to fix the problem you have to avoid generating a temporary variable at all, instead inlining the call into the return statement itself. So the binding for Node::firstChild stops doing
    RefPtr<Node> temp = imp->firstChild();
    return V8DOMWrapper::convertNodeToV8Object(temp);
and instead uses
    return V8DOMWrapper::convertNodeToV8Object(imp->firstChild());
This works because convertNodeToV8Object is overloaded to take either an X* or a PassRefPtr<X> as a parameter.

[Jens Alfke]

Created attachment 41523 [details]
patch 1

[David Levin]

Comment on attachment 41523 [details]
patch 1

I don't know that I'll be able to do the final review of this, but I can give some quick feedback on a part of this.


> Index: WebCore/ChangeLog
> +        (WebCore::V8DOMWrapper::convertToV8Object): Added overload that takes a Ref<>
Answering "Why?" is much better than answering "what?" (Answering "What was done?" is pretty useless because it can be determined by looking at the code.)


Functions shouldn't take a RefPtr. They should take either a PassRefPtr<T> or a T* (http://webkit.org/coding/RefPtr.html).

[Jens Alfke]

In general RefPtrs shouldn't be used as parameters, but this is a special case to handle generated code like the below (from V8Document.cpp):
    RefPtr<NodeIterator> result = imp->createNodeIterator(root, whatToShow, filter.get(), expandEntityReferences, ec);
    ...
    return V8DOMWrapper::convertToV8Object(V8ClassIndex::NODEITERATOR, result);

The normal convention would be to add ".release()" after the result parameter in the second line, but the first line may or may not declare 'result' as a RefPtr and the second line doesn't know which it is.

That said, I think I can add a bit of code to the binding generator to get that .release() call in there...

[Jens Alfke]

Created attachment 41605 [details]
patch 2

This updated patch avoids creating a new method that takes a Ref<>. As a bonus, it now changes only one source file.

[Dimitri Glazkov (Google)]

Comment on attachment 41605 [details]
patch 2

r=me.

[WebKit Commit Bot]

Comment on attachment 41605 [details]
patch 2

Clearing flags on attachment: 41605

Committed r49996: <http://trac.webkit.org/changeset/49996>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.