Bug 164096 - [DOM] Add JSEventType
Summary: [DOM] Add JSEventType
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 164137
  Show dependency treegraph
 
Reported: 2016-10-27 16:57 PDT by Yusuke Suzuki
Modified: 2016-10-28 11:31 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.20 KB, patch)
2016-10-27 17:29 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.22 KB, patch)
2016-10-27 17:37 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.41 KB, patch)
2016-10-27 17:44 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (14.09 KB, patch)
2016-10-27 18:01 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-10-27 16:57:20 PDT
I've found that Event is frequently touched in Speedometer. And I think it is frequently touched in the real Web.
But, if you touch the accessors of Event interface, you always need to perform jsDynamicCast<Event*>.
It is costly because the typical event instance is not direct instance of the Event class. For example,

MouseEvent -> UIEvent -> Event

Event resides in higher position in the hierarchy. But it has many accessors and they are used (currentTarget, type, target etc.)
So it's worth introducing JSEventType (similar to JSDocumentWrapperType) and jsEventCast.

Current my policy is, "If the jsDynamicCast<XXX>() is so frequently called and it always walks many classes, we should consider introducing some JSType.".
For example, CanvasWebGLContext thing does not require it because jsDynamicCast<....Context>() typically succeeds with the first ClassInfo.
But it is not for Event.
Comment 1 Yusuke Suzuki 2016-10-27 17:29:45 PDT
Created attachment 293086 [details]
Patch
Comment 2 Yusuke Suzuki 2016-10-27 17:37:27 PDT
Created attachment 293089 [details]
Patch
Comment 3 Yusuke Suzuki 2016-10-27 17:44:23 PDT
Created attachment 293091 [details]
Patch
Comment 4 Yusuke Suzuki 2016-10-27 18:01:48 PDT
Created attachment 293092 [details]
Patch
Comment 5 Yusuke Suzuki 2016-10-27 20:59:46 PDT
Committed r208028: <http://trac.webkit.org/changeset/208028>
Comment 6 Darin Adler 2016-10-28 11:00:11 PDT
Comment on attachment 293092 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2531
>      return "jsElementCast" if $interfaceName eq "Element";
>      return "jsDocumentCast" if $interfaceName eq "Document";
>      return "jsEventTargetCast" if $interfaceName eq "EventTarget";
> +    return "jsEventCast" if $interfaceName eq "Event";
>      return "jsDynamicCast<JS$interfaceName*>";

I think we should consider putting all these optimizations into specializations of the jsDynamicCast function. Then we can remove all these special cases from the code generator, and programmers can’t ever make the mistake of using the slow version by accident.

Any reason not to do that?
Comment 7 Yusuke Suzuki 2016-10-28 11:23:15 PDT
(In reply to comment #6)
> Comment on attachment 293092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293092&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2531
> >      return "jsElementCast" if $interfaceName eq "Element";
> >      return "jsDocumentCast" if $interfaceName eq "Document";
> >      return "jsEventTargetCast" if $interfaceName eq "EventTarget";
> > +    return "jsEventCast" if $interfaceName eq "Event";
> >      return "jsDynamicCast<JS$interfaceName*>";
> 
> I think we should consider putting all these optimizations into
> specializations of the jsDynamicCast function. Then we can remove all these
> special cases from the code generator, and programmers can’t ever make the
> mistake of using the slow version by accident.
> 
> Any reason not to do that?

Definitely we should do that.
One concern is that we have a bad time if we forget to include the header that specializes jsDynamicCast.
It is serious because it will not cause any compile errors but will cause performance regression.

I think we should add a helper function in WebCore, like the following.

template<typename To, typename From>
ALWAYS_INLINE To jsDowncast(From from)
{
    return jsDynamicCast<To>(from);
}

template<>
template<typename From>
ALWAYS_INLINE jsDowncast<Node*>(From from)
{
    return jsNodeCast(from);
}
Comment 8 Yusuke Suzuki 2016-10-28 11:31:39 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Comment on attachment 293092 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=293092&action=review
> > 
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2531
> > >      return "jsElementCast" if $interfaceName eq "Element";
> > >      return "jsDocumentCast" if $interfaceName eq "Document";
> > >      return "jsEventTargetCast" if $interfaceName eq "EventTarget";
> > > +    return "jsEventCast" if $interfaceName eq "Event";
> > >      return "jsDynamicCast<JS$interfaceName*>";
> > 
> > I think we should consider putting all these optimizations into
> > specializations of the jsDynamicCast function. Then we can remove all these
> > special cases from the code generator, and programmers can’t ever make the
> > mistake of using the slow version by accident.
> > 
> > Any reason not to do that?
> 
> Definitely we should do that.
> One concern is that we have a bad time if we forget to include the header
> that specializes jsDynamicCast.
> It is serious because it will not cause any compile errors but will cause
> performance regression.
> 
> I think we should add a helper function in WebCore, like the following.
> 
> template<typename To, typename From>
> ALWAYS_INLINE To jsDowncast(From from)
> {
>     return jsDynamicCast<To>(from);
> }
> 
> template<>
> template<typename From>
> ALWAYS_INLINE jsDowncast<Node*>(From from)
> {
>     return jsNodeCast(from);
> }

I've opened the issue. https://bugs.webkit.org/show_bug.cgi?id=164137
And soon I'll upload the patch.