Bug 157604 – Avoid unnecessary null checks in toJS() when the implementation returns a reference or Ref<>

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Avoid unnecessary null checks in toJS() when the implementation returns a reference or Ref<>.

[Chris Dumez]

Created attachment 278694 [details]
Patch

[Chris Dumez]

Created attachment 278695 [details]
Patch

[Darin Adler]

Comment on attachment 278694 [details]
Patch

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

Should have renamed this to convert and changed all the ExecState* to ExecState&. OK, just kidding.

A few comments:

> Source/WebCore/bindings/js/JSDOMBinding.h:255
>  template<typename T> JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, PassRefPtr<T>);

Why not change this to const PassRefPtr<T>&, at least until we can delete it.

> Source/WebCore/bindings/js/JSDOMBinding.h:559
> +    return toJS(exec, globalObject, const_cast<Ref<T>&>(ptr).get());

Why not do the const cast on the result of get() instead?

> Source/WebCore/bindings/js/JSDOMBinding.h:574
> +    return toJSNewlyCreated(exec, globalObject, const_cast<Ref<T>&>(ptr).get());

Ditto.

> Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp:43
> +        return wrap<JSAllVideoCapabilities>(globalObject, static_cast<AllVideoCapabilities>(object));

Pre-existing: This seems like a peculiar cast. Shouldn’t it be AllVideoCapabilities&? Do we really want to copy the object?

> Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp:45
> +    return wrap<JSAllAudioCapabilities>(globalObject, static_cast<AllAudioCapabilities>(object));

Ditto.

[Build Bot]

Comment on attachment 278695 [details]
Patch

Attachment 278695 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1307533

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 278698 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Chris Dumez]

Created attachment 278699 [details]
Patch

[Alex Christensen]

Comment on attachment 278699 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:559
> +    return toJS(exec, globalObject, const_cast<T&>(ptr.get()));

Do we really need to add a const_cast?  Couldn't we make the parameter a Ref<T>& or make toJS take a const T&?

> Source/WebCore/bindings/js/JSDOMBinding.h:574
> +    return toJSNewlyCreated(exec, globalObject, const_cast<T&>(ptr.get()));

ditto.

[Darin Adler]

Comment on attachment 278699 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3414
> +    void* actualVTablePointer = *(reinterpret_cast<void**>(&impl));

Extra unneeded parentheses here around the reinterpret_cast.

[WebKit Commit Bot]

Comment on attachment 278699 [details]
Patch

Clearing flags on attachment: 278699

Committed r200775: <http://trac.webkit.org/changeset/200775>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 278699 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMBinding.h:559
>> +    return toJS(exec, globalObject, const_cast<T&>(ptr.get()));
> 
> Do we really need to add a const_cast?  Couldn't we make the parameter a Ref<T>& or make toJS take a const T&?

If the parameter would be a Ref<T>&, then it wouldn't be able to bind to a temporary. E.g.
return toJS(state, globalObject, impl.createObject());

would not work if impl.createObject() returns a Ref<>

[Chris Dumez]

Looks like a 2% progression on Speedometer on iOS.

[Ryosuke Niwa]

(In reply to comment #12)
> Looks like a 2% progression on Speedometer on iOS.

And 3% on JSBench on iOS.

[Darin Adler]

!!!

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #12)
> > Looks like a 2% progression on Speedometer on iOS.
> 
> And 3% on JSBench on iOS.

And it looks like 2% progression on JetStream / iOS as well !?

I did not know JSBench / JetStream used our JS bindings at all.

[youenn fablet]

(In reply to comment #11)
> Comment on attachment 278699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278699&action=review
> 
> >> Source/WebCore/bindings/js/JSDOMBinding.h:559
> >> +    return toJS(exec, globalObject, const_cast<T&>(ptr.get()));
> > 
> > Do we really need to add a const_cast?  Couldn't we make the parameter a Ref<T>& or make toJS take a const T&?
> 
> If the parameter would be a Ref<T>&, then it wouldn't be able to bind to a
> temporary. E.g.
> return toJS(state, globalObject, impl.createObject());
> 
> would not work if impl.createObject() returns a Ref<>

I updated JSDOMBinding.h in bug 157307 to cope with that.

It might be good also to make toJSNewlyCreated() take Ref<>/RefPtr<> or Ref<>&&/RefPtr<>&& and not raw pointers/references. That may help removing some count churnning.
I'll look at that, hopefully next week.

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #11)
> > Comment on attachment 278699 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=278699&action=review
> > 
> > >> Source/WebCore/bindings/js/JSDOMBinding.h:559
> > >> +    return toJS(exec, globalObject, const_cast<T&>(ptr.get()));
> > > 
> > > Do we really need to add a const_cast?  Couldn't we make the parameter a Ref<T>& or make toJS take a const T&?
> > 
> > If the parameter would be a Ref<T>&, then it wouldn't be able to bind to a
> > temporary. E.g.
> > return toJS(state, globalObject, impl.createObject());
> > 
> > would not work if impl.createObject() returns a Ref<>
> 
> I updated JSDOMBinding.h in bug 157307 to cope with that.
> 
> It might be good also to make toJSNewlyCreated() take Ref<>/RefPtr<> or
> Ref<>&&/RefPtr<>&& and not raw pointers/references. That may help removing
> some count churnning.
> I'll look at that, hopefully next week.

I believe this patch did this as well unless I misunderstood you.

[youenn fablet]

> > It might be good also to make toJSNewlyCreated() take Ref<>/RefPtr<> or
> > Ref<>&&/RefPtr<>&& and not raw pointers/references. That may help removing
> > some count churnning.
> > I'll look at that, hopefully next week.
> 
> I believe this patch did this as well unless I misunderstood you.

IIANM, toJSNewlyCreated is basically calling createWrapper.
Currently, createWrapper is taking a raw pointer.
It then creates a Ref<> when calling JSXX::create, thus incrementing the DOM object counter.

It might be good to have createWrapper take a Ref<> or Ref<>&&.
That way, when toJSNewlyCreated is passed a Ref<>&&, it would be moved to createWrapper and there would be no counter modification.

toJS may be in scope also but I am less sure of its usefulness.