Bug 97943 – [V8] The concept of forceNewObject is unneeded (dom-traverse gets 3% faster)

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

[V8] Male Dromaeo/dom-traverse.html 3% faster

[Adam Barth]

Created attachment 166318 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 166318 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [V8] Male Dromaeo/dom-traverse.html 3% faster

Why only male?

[Adam Barth]

Created attachment 166320 [details]
Patch for landing

[Adam Barth]

Comment on attachment 166320 [details]
Patch for landing

Actually, there might be something even better to do.  /me investigates.

[Adam Barth]

Created attachment 166333 [details]
Patch

[Kentaro Hara]

Comment on attachment 166333 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        We don't need the concept of forceNewObject. It doen't do anything

Great observation! I think JSC is using a similar concept too. Maybe we can remove it from JSC too (in another patch)?

Typo: doen't

> Source/WebCore/ChangeLog:11
> +        * bindings/scripts/CodeGeneratorV8.pm:

Maybe you need to update run-binding-tests?

> Source/WebCore/bindings/v8/custom/V8NodeCustom.cpp:135
> -    if (!forceNewObject) {
> -        v8::Handle<v8::Value> wrapper = V8DOMWrapper::getCachedWrapper(impl);
> -        if (!wrapper.IsEmpty())
> -            return wrapper;
> -    }
> +    v8::Handle<v8::Value> wrapper = V8DOMWrapper::getCachedWrapper(impl);
> +    if (!wrapper.IsEmpty())
> +        return wrapper;

Before landing, please just double-check that we are clearing the cached wrapper correctly. If we kept a stale cached wrapper, we will end up returning a wrong wrapper object.

[Adam Barth]

(In reply to comment #6)
> (From update of attachment 166333 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166333&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        We don't need the concept of forceNewObject. It doen't do anything
> 
> Great observation! I think JSC is using a similar concept too. Maybe we can remove it from JSC too (in another patch)?

JSC uses this IDL attribute to call toJSNewlyCreated, which is what we should do too.  That's for a future patch though.

> Typo: doen't

Fixed.

> > Source/WebCore/ChangeLog:11
> > +        * bindings/scripts/CodeGeneratorV8.pm:
> 
> Maybe you need to update run-binding-tests?

I thought so too, but it seemed to work ok.  I would have expected TestNode to show some differences, but it didn't.

> > Source/WebCore/bindings/v8/custom/V8NodeCustom.cpp:135
> > -    if (!forceNewObject) {
> > -        v8::Handle<v8::Value> wrapper = V8DOMWrapper::getCachedWrapper(impl);
> > -        if (!wrapper.IsEmpty())
> > -            return wrapper;
> > -    }
> > +    v8::Handle<v8::Value> wrapper = V8DOMWrapper::getCachedWrapper(impl);
> > +    if (!wrapper.IsEmpty())
> > +        return wrapper;
> 
> Before landing, please just double-check that we are clearing the cached wrapper correctly. If we kept a stale cached wrapper, we will end up returning a wrong wrapper object.

I'm pretty sure we're doing this correctly.  The common case was to have forceNewObject be false anyway.  Is there something in particular you'd like me to check?

[Adam Barth]

Created attachment 166340 [details]
Patch for landing

[Kentaro Hara]

(In reply to comment #7)
> I'm pretty sure we're doing this correctly.  The common case was to have forceNewObject be false anyway.  Is there something in particular you'd like me to check?

I checked V8DOMDataStore and V8DOMMap, it looks OK. Thanks.

[WebKit Review Bot]

Comment on attachment 166340 [details]
Patch for landing

Clearing flags on attachment: 166340

Committed r129971: <http://trac.webkit.org/changeset/129971>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Erik Arvidsson]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 166333 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=166333&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        We don't need the concept of forceNewObject. It doen't do anything
> > 
> > Great observation! I think JSC is using a similar concept too. Maybe we can remove it from JSC too (in another patch)?
> 
> JSC uses this IDL attribute to call toJSNewlyCreated, which is what we should do too.  That's for a future patch though.
> 
> > Typo: doen't
> 
> Fixed.
> 
> > > Source/WebCore/ChangeLog:11
> > > +        * bindings/scripts/CodeGeneratorV8.pm:
> > 
> > Maybe you need to update run-binding-tests?
> 
> I thought so too, but it seemed to work ok.  I would have expected TestNode to show some differences, but it didn't.

The test idl files don't have any method with [ReturnNewObject]. You could have added a line to it to ensure that the generated code looks right.

[Adam Barth]

> The test idl files don't have any method with [ReturnNewObject]. You could have added a line to it to ensure that the generated code looks right.

Good idea. I'll add one when we add toV8NewlyCreated.

[Adam Barth]

Turns out not to have effected the perf test on the bot.  This perf test is more noisy than I realized, so the perf change might just be noise.  :(