Closed Bug 1387399 Opened 2 years ago Closed 2 years ago

Rectangles could use some SIMD optimizations

Categories

(Core :: Graphics, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten, NeedInfo)

References

(Depends on 1 open bug)

Details

(Keywords: feature, Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

Rectangles are sort of 2 vectors of 2 numbers. SIMD could help them.
Can you include performance numbers in the commit message?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Can you include performance numbers in the commit message?

What type of numbers do you want? Portions of profiles? A rough estimate? (It's about 2x-4x faster, depending on the 'circumstances' (Cache locality of the actual rectangle, etc.))
Flags: needinfo?(jmuizelaar)
(In reply to Bas Schouten (:bas.schouten) from comment #3)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> > Can you include performance numbers in the commit message?
> 
> What type of numbers do you want? Portions of profiles? A rough estimate?
> (It's about 2x-4x faster, depending on the 'circumstances' (Cache locality
> of the actual rectangle, etc.))

The best numbers would be a run of a benchmark or some task and how much it changes with this patch.
Flags: needinfo?(jmuizelaar)
So the two patches combined provide the following performance difference on the dl-test testcase:

DL-building without: 34.7ms
FLB without: 27.5ms

DL-building with: 33.4ms (~4% improvement)
FLB with: 23.5ms (~15% improvement)
Flags: needinfo?(jmuizelaar)
I will include these numbers on the first patch commit message.
Comment on attachment 8893776 [details]
Bug 1387399: Add SIMD optimizations for methods used frequently in nsRect.

https://reviewboard.mozilla.org/r/164878/#review173584

The integer pieces seem fine. You can land them now. I'll need more convincing that the floating point stuff is correct.

::: gfx/src/nsRect.h:289
(Diff revision 2)
>    mozilla::gfx::IntRect rect;
> -  rect.x = NSToIntRoundUp(NSAppUnitsToDoublePixels(x, aAppUnitsPerPixel) * aXScale);
> -  rect.y = NSToIntRoundUp(NSAppUnitsToDoublePixels(y, aAppUnitsPerPixel) * aYScale);
> -  // Avoid negative widths and heights due to overflow
> -  rect.width  = std::max(0, NSToIntRoundUp(NSAppUnitsToDoublePixels(XMost(),
> -                               aAppUnitsPerPixel) * aXScale) - rect.x);
> +
> +  __m128 c1 = _mm_set_ps(aAppUnitsPerPixel, aAppUnitsPerPixel, aAppUnitsPerPixel, aAppUnitsPerPixel);
> +  __m128 c2 = _mm_set_ps(aYScale, aXScale, aYScale, aXScale);
> +  __m128 c3 = _mm_set_ps(0.5f, 0.5f, 0.5f, 0.5f);
> +

It would be good to have some tests showing that these two pieces of code produce the same values. If not tests, some better documentation about why this is equivalent to the C++ code.

::: gfx/src/nsRect.h:301
(Diff revision 2)
> +  recti = _mm_add_epi32(recti, widthheight); // X, Y, XMost(), YMost()
> +
> +  __m128 rectf = _mm_cvtepi32_ps(recti);
> +
> +  // Scale
> +  rectf = _mm_mul_ps(_mm_div_ps(rectf, c1), c2);

Won't this mul be executed with a different rounding mode than in the C++ version?
Attachment #8893776 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8893776 [details]
Bug 1387399: Add SIMD optimizations for methods used frequently in nsRect.

https://reviewboard.mozilla.org/r/164878/#review173584

> It would be good to have some tests showing that these two pieces of code produce the same values. If not tests, some better documentation about why this is equivalent to the C++ code.

I will add more documentation.

> Won't this mul be executed with a different rounding mode than in the C++ version?

I'm unconvinced that it will matter. More importantly this is always an issue with SSE code as x87 and the SIMD instructions use different control registers for their rounding mode and although they have the same default (at least on Windows), we don't put a lot of effort into making sure they're in their default modes either way. More importantly though, SSE and x87 code has implicitly different results since x87 will use much larger precision. Differences here can occur even when the compiler inlines this functions as it can decide on x87 in one case and SSE in the other (although I could expect it to usually use SSE when compiled for that platform). I feel I would need a lot of convincing to believe this is relevant, considering the amount of machine dependency we have in Gecko, which I admit isn't great), it's hard to believe -this- will ever lead to an issue.
Comment on attachment 8894951 [details]
Bug 1387399 - Part 2: SIMD optimize ScaleToOuterPixels.

https://reviewboard.mozilla.org/r/166070/#review174904
Attachment #8894951 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8893776 [details]
Bug 1387399: Add SIMD optimizations for methods used frequently in nsRect.

https://reviewboard.mozilla.org/r/164878/#review178788
Attachment #8893776 - Flags: review?(jmuizelaar) → review+
Keywords: feature
Priority: -- → P3
Whiteboard: [gfx-noted]
I should get to this.
Assignee: nobody → bas
Blocks: 1385437
Status: NEW → ASSIGNED
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4beeacb75bf5
Add SIMD optimizations for methods used frequently in nsRect. r=jrmuizel
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b78a225fa8c9
Followup: Fix Windows static analysis bustage. r+a=bustage on a CLOSED TREE
https://hg.mozilla.org/mozilla-central/rev/4beeacb75bf5
https://hg.mozilla.org/mozilla-central/rev/b78a225fa8c9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Backout by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/ec3dd3ee2ae4
Backed out 2 changesets for OSX reftests permafailures on attachment-local-positioning-2.html. a=backout
Backed out for OSX reftests permafailures on attachment-local-positioning-2,3,4.html

Push with failures: https://is.gd/73dFF8

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174695096&repo=mozilla-inbound&lineNumber=2989

Backout link: https://hg.mozilla.org/mozilla-central/rev/ec3dd3ee2ae4b3a63529a912816a110e925eb2d0
Status: RESOLVED → REOPENED
Flags: needinfo?(bas)
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
Attachment #8894951 - Attachment is obsolete: true
Flags: needinfo?(bas)
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16a7ab26c645
Add SIMD optimizations for methods used frequently in nsRect. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/16a7ab26c645
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1521782
You need to log in before you can comment on or make changes to this bug.