Bug 30599 - V8 binding optimizations (redundant refs, unlikely errors)
Summary: V8 binding optimizations (redundant refs, unlikely errors)
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Enhancement
Assignee: Jens Alfke
Depends on:
Reported: 2009-10-20 13:48 PDT by Jens Alfke
Modified: 2009-10-23 13:43 PDT (History)
1 user (show)

See Also:

patch 1 (7.06 KB, patch)
2009-10-20 13:54 PDT, Jens Alfke
levin: review-
Details | Formatted Diff | Diff
patch 2 (5.57 KB, patch)
2009-10-21 13:35 PDT, Jens Alfke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jens Alfke 2009-10-20 13:48:41 PDT
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.
Comment 1 Jens Alfke 2009-10-20 13:54:19 PDT
Created attachment 41523 [details]
patch 1
Comment 2 David Levin 2009-10-20 15:28:06 PDT
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).
Comment 3 Jens Alfke 2009-10-20 16:31:00 PDT
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...
Comment 4 Jens Alfke 2009-10-21 13:35:02 PDT
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.
Comment 5 Dimitri Glazkov (Google) 2009-10-23 09:54:17 PDT
Comment on attachment 41605 [details]
patch 2

Comment 6 WebKit Commit Bot 2009-10-23 13:43:26 PDT
Comment on attachment 41605 [details]
patch 2

Clearing flags on attachment: 41605

Committed r49996: <http://trac.webkit.org/changeset/49996>
Comment 7 WebKit Commit Bot 2009-10-23 13:43:30 PDT
All reviewed patches have been landed.  Closing bug.