Bug 194073 – Hit testing functions optimizations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

<rdar://problem/47692312> Hit testing functions optimizations

[Benjamin Poulain]

Created attachment 360684 [details]
Patch

[Benjamin Poulain]

<rdar://problem/47692312>

[Simon Fraser (smfr)]

Comment on attachment 360684 [details]
Patch

Do any of these really make measurable perf improvements?

[Benjamin Poulain]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 360684 [details]
> Patch
> 
> Do any of these really make measurable perf improvements?

Oh yeah.

Maybe not on your machine but on older CPUs that hurts.

[Tim Horton]

Comment on attachment 360684 [details]
Patch

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

> Source/WTF/ChangeLog:4
> +        <rdar://problem/47692312> Hit testing functions optimizations
> +        https://bugs.webkit.org/show_bug.cgi?id=194073

This is usually:

Title
Bugzilla link
Radar link

[Simon Fraser (smfr)]

"webkit-patch upload" will format the changelog for you.

[zalan]

or prepare-ChangeLog -b 194073 if you prefer uploading patches manually.

[Benjamin Poulain]

(In reply to Tim Horton from comment #5)
> Comment on attachment 360684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360684&action=review
> 
> > Source/WTF/ChangeLog:4
> > +        <rdar://problem/47692312> Hit testing functions optimizations
> > +        https://bugs.webkit.org/show_bug.cgi?id=194073
> 
> This is usually:
> 
> Title
> Bugzilla link
> Radar link

I just use the radar title as bug title. What's the easiest way to create bugzilla bug from radars?

[Tim Horton]

(In reply to Benjamin Poulain from comment #8)
> (In reply to Tim Horton from comment #5)
> > Comment on attachment 360684 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=360684&action=review
> > 
> > > Source/WTF/ChangeLog:4
> > > +        <rdar://problem/47692312> Hit testing functions optimizations
> > > +        https://bugs.webkit.org/show_bug.cgi?id=194073
> > 
> > This is usually:
> > 
> > Title
> > Bugzilla link
> > Radar link
> 
> I just use the radar title as bug title. What's the easiest way to create
> bugzilla bug from radars?

webkit-patch will make a bugzilla bug for you when you're uploading your patch

[Benjamin Poulain]

(In reply to Tim Horton from comment #9)
> > I just use the radar title as bug title. What's the easiest way to create
> > bugzilla bug from radars?
> 
> webkit-patch will make a bugzilla bug for you when you're uploading your
> patch

What I always use.

[Benjamin Poulain]

Created attachment 360823 [details]
Patch

[Benjamin Poulain]

(In reply to Benjamin Poulain from comment #4)
> (In reply to Simon Fraser (smfr) from comment #3)
> > Comment on attachment 360684 [details]
> > Patch
> > 
> > Do any of these really make measurable perf improvements?
> 
> Oh yeah.
> 
> Maybe not on your machine but on older CPUs that hurts.

Most of it is getting rid of int<->float and int<->double conversion. That's where we bleed the most.

I would expect ARM would benefit even more but I guess we do a lot less hit testing there.

[Simon Fraser (smfr)]

Comment on attachment 360823 [details]
Patch

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

> Source/WebCore/page/FrameViewLayoutContext.h:96
> +    RenderLayoutState* layoutState() const PURE_FUNCTION;

What does PURE_FUNCTION do?

[Simon Fraser (smfr)]

(In reply to Benjamin Poulain from comment #12)
> Most of it is getting rid of int<->float and int<->double conversion. That's
> where we bleed the most.

I bet we have TONS of LayoutRect <-> FloatRect <-> IntRect conversions in various places. Is there any way to find the hotspots?

[Benjamin Poulain]

(In reply to Simon Fraser (smfr) from comment #13)
> Comment on attachment 360823 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360823&action=review
> 
> > Source/WebCore/page/FrameViewLayoutContext.h:96
> > +    RenderLayoutState* layoutState() const PURE_FUNCTION;
> 
> What does PURE_FUNCTION do?

We tell the compiler that the return value only depends on its parameters, and global memory. AND that the function does not change the state of the program.

Since we do not change the memory of FrameViewLayoutContext between the two calls, the compiler is allowed to reuse the value of the first call.

The doc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
A good Stackoverflow: https://stackoverflow.com/questions/29117836/attribute-const-vs-attribute-pure-in-gnu-c

> I bet we have TONS of LayoutRect <-> FloatRect <-> IntRect conversions in
> various places. Is there any way to find the hotspots?

I suspect you are right.

I don't have an idea to find the bad ones though.

[Benjamin Poulain]

Created attachment 360945 [details]
Patch

[Benjamin Poulain]

I added a fast int->unsigned since that was used in an API test.

Also added some tests for the new functions.

[Simon Fraser (smfr)]

It's probably better to fork the clamp changes into their own bug (Darin might have opinions).

[Darin Adler]

I agree that we should separate out the clampTo fixes; I’d think we’d want to land them first, and then go on to the optimizations that are built on them. Even if it’s just for regression testing and future bisection if no other reason.

I haven’t studied the behavior changes in clampTo yet -- I understand the principle but not the details of how the function needs to be used differently. I see many call sites changing and the regression tests alone don’t make it clear to me what changed about how it has to be used and what results you get.

The idea of making it more efficient without changing behavior is fantastic. I almost wish that any changes on how it has to be used or implements in behavior could be separate from a patch that was purely about performance and didn’t require changes at call sites. That could take the form of a patch that changes call sites without affecting behavior or performance. Then another that affects performance without changing behavior. Or whatever sequence we can make like that. Again, better for later bisection to see if any problems were caused.

[Benjamin Poulain]

Sounds good. I'll make a separate clampTo() patch with better explanations and testing.

[Benjamin Poulain]

Created attachment 361155 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 361155 [details]
Patch

Clearing flags on attachment: 361155

Committed r240968: <https://trac.webkit.org/changeset/240968>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.