Closed
Bug 1387399
Opened 7 years ago
Closed 6 years ago
Rectangles could use some SIMD optimizations
Categories
(Core :: Graphics, enhancement, P3)
Core
Graphics
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.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
Can you include performance numbers in the commit message?
Assignee | ||
Comment 3•7 years ago
|
||
(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.))
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jmuizelaar)
Comment 6•7 years ago
|
||
(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)
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
I will include these numbers on the first patch commit message.
Comment 9•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
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 11•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-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+
Updated•7 years ago
|
Assignee | ||
Comment 15•6 years ago
|
||
I should get to this.
Comment 16•6 years ago
|
||
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
Comment 17•6 years ago
|
||
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
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4beeacb75bf5 https://hg.mozilla.org/mozilla-central/rev/b78a225fa8c9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 19•6 years ago
|
||
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
Comment 20•6 years ago
|
||
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
status-firefox61:
fixed → ---
Flags: needinfo?(bas)
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8894951 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(bas)
Comment 22•6 years ago
|
||
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
Comment 23•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/16a7ab26c645
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
status-firefox57:
fix-optional → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•