Bug 82201 - [V8][Performance] Optimize createTextNode(), createElement(), cloneNode() etc
Summary: [V8][Performance] Optimize createTextNode(), createElement(), cloneNode() etc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-26 07:04 PDT by Kentaro Hara
Modified: 2012-03-27 00:49 PDT (History)
5 users (show)

See Also:


Attachments
Performance test (1.53 KB, text/html)
2012-03-26 07:04 PDT, Kentaro Hara
no flags Details
Patch (7.22 KB, patch)
2012-03-26 07:05 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (7.19 KB, patch)
2012-03-26 07:08 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (8.71 KB, patch)
2012-03-26 17:42 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (8.69 KB, patch)
2012-03-26 17:47 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (8.79 KB, patch)
2012-03-26 21:58 PDT, Kentaro Hara
abarth: commit-queue-
Details | Formatted Diff | Diff
patch for landing (8.25 KB, patch)
2012-03-26 23:48 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-03-26 07:04:27 PDT
Created attachment 133799 [details]
Performance test

createTextNode(), createElement(), cloneNode() etc, which creates a new DOM object every time, are much slower in V8 than in JSC. There are the main methods used in Dromaeo/dom-modify.html.

The results of the attached performance test are as follows:

Chromium/V8/Mac:
createTextNode : median=317ms (mean=439.08ms, min=303ms, max=3126ms)
createElement : median=403ms (mean=695.70ms, min=398ms, max=5615ms)
cloneNode : median=384ms (mean=577.96ms, min=372ms, max=5313ms)

Safari/JSC/Mac:
createTextNode : median=142ms (mean=141.04ms, min=110ms, max=168ms)
createElement : median=234ms (mean=245.74ms, min=219ms, max=305ms)
cloneNode : median=210ms (mean=213.36ms, min=204ms, max=284ms)

Chromium results are really noisy due to the unsteadiness of V8's GC. Let us focus on the median of the measured times. I'll discuss the GC issues with the V8 team later.
Comment 1 Kentaro Hara 2012-03-26 07:05:31 PDT
Created attachment 133800 [details]
Patch
Comment 2 Kentaro Hara 2012-03-26 07:08:39 PDT
Created attachment 133802 [details]
Patch
Comment 3 Erik Arvidsson 2012-03-26 09:33:39 PDT
Comment on attachment 133802 [details]
Patch

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

Please don't forget to run run-binding-tests

> Source/WebCore/bindings/v8/V8Proxy.cpp:648
> +    windowShell()->initContextIfNeeded();
> +    return windowShell()->context();

I know we currently only get here from the code generated by the code generator which tests for IsNodeSubType so we cannot get here from a worker. But I'm concerned about us special casing so many things. For now I would be OK with an assert. In the long term I think it is bad that we tie the context to the windowShell like this.
Comment 4 Adam Barth 2012-03-26 10:35:45 PDT
Comment on attachment 133802 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Proxy.cpp:642
> +        RefPtr<SharedPersistent<v8::Context> > context = isolatedContext->sharedContext();

Why is this a RefPtr?

> Source/WebCore/bindings/v8/V8Proxy.h:248
> +        // context() and persistentContext() return the same context.
> +        // While context() returns a newly created Local handle,
> +        // persistentContext() returns an existing Persistent handle.
>          v8::Local<v8::Context> context();
> +        v8::Handle<v8::Context> persistentContext();

I'm pretty sure we use a new local handle here because we're worried about our persistent handle getting disposed while we've entered it.  Consider what happens if we run page JavaScript while having entered our persistent handle and that JavaScript causes the Frame and everything to be torn down.  We'll dispose the persistent handle, which could cause the context to be destroyed.

I'm pretty sure we've had crashes like that before and that's why we use a new local handle (to ensure that the context stays around).
Comment 5 Kentaro Hara 2012-03-26 17:42:23 PDT
Created attachment 133941 [details]
Patch
Comment 6 Kentaro Hara 2012-03-26 17:47:35 PDT
Created attachment 133943 [details]
Patch
Comment 7 Kentaro Hara 2012-03-26 17:49:42 PDT
(In reply to comment #4)
> > Source/WebCore/bindings/v8/V8Proxy.cpp:642
> > +        RefPtr<SharedPersistent<v8::Context> > context = isolatedContext->sharedContext();
> 
> Why is this a RefPtr?

Fixed.

> > Source/WebCore/bindings/v8/V8Proxy.h:248
> > +        // context() and persistentContext() return the same context.
> > +        // While context() returns a newly created Local handle,
> > +        // persistentContext() returns an existing Persistent handle.
> >          v8::Local<v8::Context> context();
> > +        v8::Handle<v8::Context> persistentContext();
> 
> I'm pretty sure we use a new local handle here because we're worried about our persistent handle getting disposed while we've entered it.  Consider what happens if we run page JavaScript while having entered our persistent handle and that JavaScript causes the Frame and everything to be torn down.  We'll dispose the persistent handle, which could cause the context to be destroyed.

Fixed. Just fixed a code in a cold path, and thus no performance change.


(In reply to comment #3)
> Please don't forget to run run-binding-tests

Updated.

> > Source/WebCore/bindings/v8/V8Proxy.cpp:648
> > +    windowShell()->initContextIfNeeded();
> > +    return windowShell()->context();
> 
> I know we currently only get here from the code generated by the code generator which tests for IsNodeSubType so we cannot get here from a worker. For now I would be OK with an assert.

How should we write the assert, to check if we are not in a worker?
Comment 8 Adam Barth 2012-03-26 18:28:26 PDT
> How should we write the assert, to check if we are not in a worker?

ASSERT(isMainThread());

?
Comment 9 Adam Barth 2012-03-26 18:35:23 PDT
Comment on attachment 133943 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3160
> +        // For performance, we do not create a Local handle of the context
> +        // until we really need to enter the context.
> +        if (proxy)
> +            context = proxy->context();

Is there actually any way of getting here if proxy is false?  It seems like context.IsEmpty() will be true in that case and we'll never reach this line.

Another idea: Maybe we should move this code into V8Proxy?

v8::Handle<v8::Context> context;
if (proxy && !proxy->matchesCurrentContext()) {
    context = proxy->context();
    if (context)
        context->Enter();
}

where V8Proxy::matchesCurrentContext is something like:

v8::Handle<v8::Context> context = windowShell()->context();
return !context->IsEmpty() && context == context->GetCurrent();

(Although likely more complicated to deal with isolated worlds.)

That would save all the trickiness around having two accessors on V8Proxy for the context.
Comment 10 Kentaro Hara 2012-03-26 21:58:17 PDT
Created attachment 133972 [details]
Patch
Comment 11 Kentaro Hara 2012-03-26 21:59:38 PDT
(In reply to comment #9)
> v8::Handle<v8::Context> context;
> if (proxy && !proxy->matchesCurrentContext()) {
>     context = proxy->context();
>     if (context)
>         context->Enter();
> }
> 
> where V8Proxy::matchesCurrentContext is something like:
> 
> v8::Handle<v8::Context> context = windowShell()->context();
> return !context->IsEmpty() && context == context->GetCurrent();
> 
> That would save all the trickiness around having two accessors on V8Proxy for the context.

Done.
Comment 12 Adam Barth 2012-03-26 23:43:47 PDT
Comment on attachment 133972 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3150
> +    bool contextEntered = false;

Can't we just use context.IsEmpty() instead of introducing this bool?
Comment 13 Kentaro Hara 2012-03-26 23:48:27 PDT
Created attachment 133986 [details]
patch for landing
Comment 14 WebKit Review Bot 2012-03-27 00:48:27 PDT
Comment on attachment 133986 [details]
patch for landing

Clearing flags on attachment: 133986

Committed r112218: <http://trac.webkit.org/changeset/112218>