<rdar://problem/47692312> Hit testing functions optimizations
Created attachment 360684 [details] Patch
<rdar://problem/47692312>
Comment on attachment 360684 [details] Patch Do any of these really make measurable perf improvements?
(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.
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
"webkit-patch upload" will format the changelog for you.
or prepare-ChangeLog -b 194073 if you prefer uploading patches manually.
(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?
(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
(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.
Created attachment 360823 [details] Patch
(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.
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?
(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?
(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.
Created attachment 360945 [details] Patch
I added a fast int->unsigned since that was used in an API test. Also added some tests for the new functions.
It's probably better to fork the clamp changes into their own bug (Darin might have opinions).
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.
Sounds good. I'll make a separate clampTo() patch with better explanations and testing.
Created attachment 361155 [details] Patch
Comment on attachment 361155 [details] Patch Clearing flags on attachment: 361155 Committed r240968: <https://trac.webkit.org/changeset/240968>
All reviewed patches have been landed. Closing bug.