Bug 82201 – [V8][Performance] Optimize createTextNode(), createElement(), cloneNode() etc

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 133800 [details]
Patch

[Kentaro Hara]

Created attachment 133802 [details]
Patch

[Erik Arvidsson]

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.

[Adam Barth]

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).

[Kentaro Hara]

Created attachment 133941 [details]
Patch

[Kentaro Hara]

Created attachment 133943 [details]
Patch

[Kentaro Hara]

(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?

[Adam Barth]

> How should we write the assert, to check if we are not in a worker?

ASSERT(isMainThread());

?

[Adam Barth]

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.

[Kentaro Hara]

Created attachment 133972 [details]
Patch

[Kentaro Hara]

(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.

[Adam Barth]

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?

[Kentaro Hara]

Created attachment 133986 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 133986 [details]
patch for landing

Clearing flags on attachment: 133986

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