Bug 250523

Summary:	Use shifts to speedup floor() and round()
Product:	WebKit	Reporter:	Ahmad Saleem <ahmad.saleem792>
Component:	Layout and Rendering	Assignee:	Brent Fulgham <bfulgham>
Status:	RESOLVED FIXED
Severity:	Normal	CC:	ap, bfulgham, cdumez, ntim, simon.fraser, thorton, webkit-bug-importer, zalan
Priority:	P2	Keywords:	InRadar
Version:	WebKit Nightly Build
Hardware:	Unspecified
OS:	Unspecified

Description Ahmad Saleem 2023-01-12 12:57:34 PST

Hi Team,

While going through Blink's commit, came across another potential performance optimization:

Blink Commit - https://chromium.googlesource.com/chromium/blink/+/143faa0607149758526318a98757a3055197cb0f

WebKit Commit - https://searchfox.org/wubkat/source/Source/WebCore/platform/LayoutUnit.h#61 & https://searchfox.org/wubkat/source/Source/WebCore/platform/LayoutUnit.h#161 & https://searchfox.org/wubkat/source/Source/WebCore/platform/LayoutUnit.h#170

Just wanted to raise it for input. Thanks!

Comment 1 Radar WebKit Bug Importer 2023-01-19 12:58:17 PST

<rdar://problem/104441420>

Comment 2 Brent Fulgham 2023-01-19 14:08:38 PST

Interesting. I wonder if this is a measurable performance improvement.

Comment 3 Brent Fulgham 2023-01-19 14:23:39 PST

I started an A/B test to see if it's measurable.

Comment 4 Brent Fulgham 2023-01-19 14:47:08 PST

Pull request: https://github.com/Webkit/WebKit/pull/8853

Comment 5 Simon Fraser (smfr) 2023-01-19 15:36:55 PST

Was the speedup confirmed?

Comment 6 Brent Fulgham 2023-01-19 18:01:28 PST

(In reply to Simon Fraser (smfr) from comment #5)
> Was the speedup confirmed?

A/B test is running now. I wanted to put the PR up to check correctness.

Comment 7 Brent Fulgham 2023-01-20 09:55:11 PST

I messed up the perf test, and am running them again now. :-(

Comment 8 Brent Fulgham 2023-01-20 16:48:23 PST

MotionMark has a small (0.18%) improvement, but is not statistically significant.

Waiting for PLT results.

Comment 9 Brent Fulgham 2023-01-23 10:47:28 PST

Performance A/B Results:

iOS 16 (iPad Pro)
* MotionMark: 0.18% better (insignificant)
* PLT5: 0.52% better (insignificant)

macOS 13, M1 MacBook Air
* MotionMark: 1.79% better (insignificant)
* PLT5: 0.23% better (insignificant)

macOS 13, Intel iMac
* MotionMark: 0.28% worse (insignificant)
* PLT5: 0.17% better (insignificant)

Comment 10 Brent Fulgham 2023-02-16 20:09:14 PST

There doesn't seem to be a measurable win on this change, so let's not add the complexity (and less-readable code).

Comment 11 EWS 2023-02-16 21:37:22 PST

Committed 260422@main (3fdb8cf70b8a): <https://commits.webkit.org/260422@main>

Reviewed commits have been landed. Closing PR #8853 and removing active labels.

Comment 12 Alexey Proskuryakov 2023-02-17 08:18:25 PST

Looks like we ended up landing this anyway. Should it be reverted then?

Comment 13 Chris Dumez 2023-02-17 08:24:28 PST

(In reply to Alexey Proskuryakov from comment #12)
> Looks like we ended up landing this anyway. Should it be reverted then?

Sorry, that's my bad. I hadn't seen the discussion on the bugzilla side.

I'll post micro-benchmark numbers today. The new code is simpler (less branching).
We can decide whether to revert or not then.

Comment 14 zalan 2023-02-17 08:37:37 PST

Not surprised we haven't seen any perf improvements on non-micro benchmarks as I  think these functions are not used too much anymore after going full-subpixel layout/rendering (other than some in some DOM api) and their usage should be trending down.

Comment 15 Chris Dumez 2023-02-17 10:24:31 PST

With 100000000 random values (ran 3 times each):

* floor:
Before: 112.52 ms
After: 79.46ms (29% faster)

* round:
Before: 192.19ms
After: 97.04ms (50% faster)