Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2225393002:
Optimized implementation of quickReject() (Closed)
DescriptionOptimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Jank Tests Performance Impact:
Should gain us a ms or two on some tests.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
Committed: https://skia.googlesource.com/skia/+/d22a817ff57986407facd16af36320fc86ce02da
Committed: https://skia.googlesource.com/skia/+/fbfa25802709139c2f14e304319c9541da65ca27
Patch Set 1 #Patch Set 2 : Fix SkCanvas::init() #Patch Set 3 : Update the clip *after* changing the matrix/clip stack #Patch Set 4 : Add null check #Patch Set 5 : Include fwd transform in quickReject() #Patch Set 6 : use float 4s #Patch Set 7 : Cache fIsScaleTranslate and fDeviceClipBounds #Patch Set 8 : Cleaning things up #
Total comments: 20
Patch Set 9 : Rebase on deletion of quickRejectY() #Patch Set 10 : Response to comments #
Total comments: 2
Patch Set 11 : Adding test and Debug asserts #
Total comments: 2
Patch Set 12 : Fixed a bug and assert #Patch Set 13 : Fix NX_NOSIMD and Chrome assert #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 48 (32 generated)
Description was changed from ========== Fast cheating version of quickReject() BUG=skia: ========== to ========== Fast cheating version of quickReject() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002 ==========
msarett@google.com changed reviewers: + herb@google.com, mtklein@google.com, reed@google.com
Ahh this breaks everything... Stay tuned for bug fixes.
Description was changed from ========== Fast cheating version of quickReject() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002 ========== to ========== Optimized implementation of quickReject() Impl Overview (1) Keep the device clip bounds up to date. This requires minimal additional work in a few places throughout canvas. (2) Keep track of if the ctm isScaleTranslate. Yes, there's a function that does this, but it's slow to call. (3) Perform the src->device transform in quick reject, then check intersection/nan. Other Notes: (1) NaN and intersection checks are performed simultaneously. (2) We no longer quick reject infinity. (3) Affine and perspective are both handled in the slow case. (4) SkRasterClip::isEmpty() is handled by the intersection check. Performance on Nexus 6P: 93.2ms -> 59.8ms Overall Android Performance Impact: Might grab us a ms on sone tests. Hard to tell but is at least non-harmful. TODO: Correctness testing BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002 ==========
Please take a look. I think this has evolved into something we could actually land. The new commit message does a decent job of summarizing the strategy. Writing correctness tests is still a TODO.
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
I am a fan. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:269: static inline void set_qr_clip_bounds(SkRect* dst, const SkIRect& src) { Give that this is all inline, it'll probably make the same assembly and clearer C++ to just return dst? fDeviceClipBounds = qr_clip_bounds(bounds); Or going in a different direction, this->setDeviceClipBounds(bounds); https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:691: fIsScaleTranslate = true; Might want to name fConservativeIsScaleTranslate, or something like that to indicate that it's not catastrophic if we set it to false when it could be true? Does a scenario with tension between setting it properly and skipping math to be fast ever come up? https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1507: fIsScaleTranslate = fMCRec->fMatrix.isScaleTranslate(); I think we can conservatively approximate this as fIsScaleTranslate = fIsScaleTranslate && matrix.isScaleTranslate(); This might be quicker, as it's data-independent of the preConcat() math. There are of course ways that we can have non-scale-transalate x non-scale-translate == non-scale-translate (i.e. rotate 45°, then -45°), but it just seems unusual to get into that scenario. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1826: __m128 lltt = _mm_unpacklo_ps(devRect.fVec, devClip.fVec); Might help to make one rect ltrb and the other LTRB, so you can see the opposition and symmetry more clearly? https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1836: SkASSERT(false); ಠ_ಠ https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1843: static __attribute__((noinline)) bool quick_reject_slow_path(const SkRect& src, Let's add SK_NEVER_INLINE alongside SK_ALWAYS_INLINE? I think it's __declspec(noinline) for MSVC. Another option is to define this outside this translation unit, e.g. in SkRect.cpp or SkMatrix.cpp. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1853: return quick_reject_slow_path(src, fDeviceClipBounds, fMCRec->fMatrix); We might follow up to see how bad things are if we return false here. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1879: bool SkCanvas::quickRejectY(SkScalar top, SkScalar bottom) const { I think we should consider a pre-patch deleting quickRejectY(). I only see old speed-insensitive picture code calling it: ~/skia (sideways) $ git grep quickRejectY include/core/SkCanvas.h: bool quickRejectY(SkScalar top, SkScalar bottom) const { src/core/SkPicturePlayback.cpp: if (!canvas->quickRejectY(top, bottom) && paint) { src/core/SkPicturePlayback.cpp: if (!canvas->quickRejectY(top, bottom) && paint) { src/core/SkPicturePlayback.cpp: if (!canvas->quickRejectY(ptr[2], ptr[3]) && paint) { https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1885: top = top * fMCRec->fMatrix.getScaleY() + fMCRec->fMatrix.getTranslateY(); :,( "top " lines up so much more nicely with "bottom"
https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1507: fIsScaleTranslate = fMCRec->fMatrix.isScaleTranslate(); On 2016/08/11 01:24:41, mtklein wrote: > I think we can conservatively approximate this as > > fIsScaleTranslate = fIsScaleTranslate && matrix.isScaleTranslate(); > > This might be quicker, as it's data-independent of the preConcat() math. > > There are of course ways that we can have non-scale-transalate x > non-scale-translate == non-scale-translate (i.e. rotate 45°, then -45°), but it > just seems unusual to get into that scenario. err, meant to write not-st x not-st == st.
https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:269: static inline void set_qr_clip_bounds(SkRect* dst, const SkIRect& src) { On 2016/08/11 01:24:41, mtklein wrote: > Give that this is all inline, it'll probably make the same assembly and clearer > C++ to just return dst? > > fDeviceClipBounds = qr_clip_bounds(bounds); > > Or going in a different direction, > > this->setDeviceClipBounds(bounds); Agreed. Went with this->setCachedClipDeviceBounds(bounds). Thought it made sense since we already have a this->getClipDeviceBounds(). https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:691: fIsScaleTranslate = true; On 2016/08/11 01:24:41, mtklein wrote: > Might want to name fConservativeIsScaleTranslate, or something like that to > indicate that it's not catastrophic if we set it to false when it could be true? Done. > Does a scenario with tension between setting it properly and skipping math to > be fast ever come up? Potentially on concat(). I'm not really sure whether it's better to always be correct or to take a shortcut. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1507: fIsScaleTranslate = fMCRec->fMatrix.isScaleTranslate(); On 2016/08/11 01:25:43, mtklein wrote: > On 2016/08/11 01:24:41, mtklein wrote: > > I think we can conservatively approximate this as > > > > fIsScaleTranslate = fIsScaleTranslate && matrix.isScaleTranslate(); > > > > This might be quicker, as it's data-independent of the preConcat() math. > > > > There are of course ways that we can have non-scale-transalate x > > non-scale-translate == non-scale-translate (i.e. rotate 45°, then -45°), but > it > > just seems unusual to get into that scenario. > > err, meant to write not-st x not-st == st. Got it, I think this is a good guess. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1826: __m128 lltt = _mm_unpacklo_ps(devRect.fVec, devClip.fVec); On 2016/08/11 01:24:41, mtklein wrote: > Might help to make one rect ltrb and the other LTRB, so you can see the > opposition and symmetry more clearly? Agreed, done. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1836: SkASSERT(false); On 2016/08/11 01:24:41, mtklein wrote: > ಠ_ಠ Guessing this means I should implement the scalar code... Done. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1843: static __attribute__((noinline)) bool quick_reject_slow_path(const SkRect& src, On 2016/08/11 01:24:41, mtklein wrote: > Let's add SK_NEVER_INLINE alongside SK_ALWAYS_INLINE? I think it's > __declspec(noinline) for MSVC. > > Another option is to define this outside this translation unit, e.g. in > SkRect.cpp or SkMatrix.cpp. SK_NEVER_INLINE sgtm. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1853: return quick_reject_slow_path(src, fDeviceClipBounds, fMCRec->fMatrix); On 2016/08/11 01:24:41, mtklein wrote: > We might follow up to see how bad things are if we return false here. SGTM https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1879: bool SkCanvas::quickRejectY(SkScalar top, SkScalar bottom) const { On 2016/08/11 01:24:41, mtklein wrote: > I think we should consider a pre-patch deleting quickRejectY(). I only see old > speed-insensitive picture code calling it: > > ~/skia (sideways) $ git grep quickRejectY > include/core/SkCanvas.h: bool quickRejectY(SkScalar top, SkScalar bottom) > const { > src/core/SkPicturePlayback.cpp: if (!canvas->quickRejectY(top, > bottom) && paint) { > src/core/SkPicturePlayback.cpp: if (!canvas->quickRejectY(top, > bottom) && paint) { > src/core/SkPicturePlayback.cpp: if (!canvas->quickRejectY(ptr[2], > ptr[3]) && paint) { SGTM. I wanted to delete it... Glad to know the picture code is speed-insensitive. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1885: top = top * fMCRec->fMatrix.getScaleY() + fMCRec->fMatrix.getTranslateY(); On 2016/08/11 01:24:41, mtklein wrote: > :,( > > "top " lines up so much more nicely with "bottom" Acknowledged.
lgtm w/ request for assert and possibly find a way to more tightly associate changing raster-bounds and its mirror fDeviceClipBounds https://codereview.chromium.org/2225393002/diff/180001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2225393002/diff/180001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1848: bool SkCanvas::quickReject(const SkRect& src) const { SkASSERT(compute_float_from_rasterclip_bounds == fDeviceClipBounds);
Adding debug asserts to catch bugs if they exist (or pop up later). Also adding a test. Tried to better associate raster-bounds and fDeviceClipRect, but couldn't find a way to make things better. One option is to put fDeviceClipRect on MCRec, but then we pay for it in terms of stack space. https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2225393002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:269: static inline void set_qr_clip_bounds(SkRect* dst, const SkIRect& src) { On 2016/08/11 16:04:01, msarett wrote: > On 2016/08/11 01:24:41, mtklein wrote: > > Give that this is all inline, it'll probably make the same assembly and > clearer > > C++ to just return dst? > > > > fDeviceClipBounds = qr_clip_bounds(bounds); > > > > Or going in a different direction, > > > > this->setDeviceClipBounds(bounds); > > Agreed. Went with this->setCachedClipDeviceBounds(bounds). Thought it made > sense since we already have a this->getClipDeviceBounds(). Changed my mind. Going with qr_clip_bounds(). It's more clearly inlined and accessible to test/debug code in quickReject(). https://codereview.chromium.org/2225393002/diff/180001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2225393002/diff/180001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1848: bool SkCanvas::quickReject(const SkRect& src) const { On 2016/08/11 18:36:28, reed1 wrote: > SkASSERT(compute_float_from_rasterclip_bounds == fDeviceClipBounds); Done.
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2225393002/diff/200001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2225393002/diff/200001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1509: // independ of preConcat() may allow us to go faster. independ -> independent Did this actually help?
Description was changed from
==========
Optimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Performance Impact:
Might grab us a ms on sone tests. Hard to tell but is at
least non-harmful.
TODO: Correctness testing
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
==========
to
==========
Optimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Performance Impact:
Might grab us a ms on some tests. Hard to tell but is at
least non-harmful.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
==========
Description was changed from
==========
Optimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Performance Impact:
Might grab us a ms on some tests. Hard to tell but is at
least non-harmful.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
==========
to
==========
Optimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Jank Tests Performance Impact:
Should gain us a ms or two on some tests.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
Assert caught a bug...fixed. https://codereview.chromium.org/2225393002/diff/200001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2225393002/diff/200001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1509: // independ of preConcat() may allow us to go faster. On 2016/08/11 20:19:12, mtklein wrote: > independ -> independent > > Did this actually help? Added a bench for concat and dropped this optimization - seems to make no difference.
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #12 (id:220001) has been deleted
Patchset #12 (id:240001) has been deleted
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/2225393002/#ps260001 (title: "Fixed a bug and assert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Optimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Jank Tests Performance Impact:
Should gain us a ms or two on some tests.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
==========
to
==========
Optimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Jank Tests Performance Impact:
Should gain us a ms or two on some tests.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
Committed:
https://skia.googlesource.com/skia/+/d22a817ff57986407facd16af36320fc86ce02da
==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001) as https://skia.googlesource.com/skia/+/d22a817ff57986407facd16af36320fc86ce02da
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:260001) has been created in https://codereview.chromium.org/2231393003/ by mtklein@google.com. The reason for reverting is: New assert triggering in the Chrome roll, https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... and breaks the SKNX_NO_SIMD bot, https://codereview.chromium.org/2236363004.
Message was sent while issue was closed.
Description was changed from
==========
Optimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Jank Tests Performance Impact:
Should gain us a ms or two on some tests.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
Committed:
https://skia.googlesource.com/skia/+/d22a817ff57986407facd16af36320fc86ce02da
==========
to
==========
Optimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Jank Tests Performance Impact:
Should gain us a ms or two on some tests.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
Committed:
https://skia.googlesource.com/skia/+/d22a817ff57986407facd16af36320fc86ce02da
==========
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/2225393002/#ps280001 (title: "Fix NX_NOSIMD and Chrome assert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Optimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Jank Tests Performance Impact:
Should gain us a ms or two on some tests.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
Committed:
https://skia.googlesource.com/skia/+/d22a817ff57986407facd16af36320fc86ce02da
==========
to
==========
Optimized implementation of quickReject()
Impl Overview
(1) Keep the device clip bounds up to date. This
requires minimal additional work in a few places
throughout canvas.
(2) Keep track of if the ctm isScaleTranslate. Yes,
there's a function that does this, but it's slow
to call.
(3) Perform the src->device transform in quick reject,
then check intersection/nan.
Other Notes:
(1) NaN and intersection checks are performed
simultaneously.
(2) We no longer quick reject infinity.
(3) Affine and perspective are both handled in the slow
case.
(4) SkRasterClip::isEmpty() is handled by the intersection
check.
Performance on Nexus 6P:
93.2ms -> 59.8ms
Overall Android Jank Tests Performance Impact:
Should gain us a ms or two on some tests.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225393002
Committed:
https://skia.googlesource.com/skia/+/d22a817ff57986407facd16af36320fc86ce02da
Committed:
https://skia.googlesource.com/skia/+/fbfa25802709139c2f14e304319c9541da65ca27
==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001) as https://skia.googlesource.com/skia/+/fbfa25802709139c2f14e304319c9541da65ca27 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
