Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335723002:
Reduce overhead for linear color xforms (Closed)
DescriptionReduce overhead for linear color xforms
We used to build src and dst transfer fn tables
every time a new xform was created with linear
src and dst. Now we don't compute them because
we don't need them.
This will make SkColorSpaceXform a far better
option for any xforms with float or half-float
inputs or outputs, particularly on a small number
of pixels.
This CL also moves SkColorSpaceXform closer to
what I anticipate will be the eventual 'API design'.
I think apply() will want to take a SrcColorType enum
(not created yet because it's not necessary yet) and
a DstColorType enum (still using SkColorType because
there's not yet a reason not to).
Performance changes:
toSRGB 341us -> 366us
to2Dot2 404us -> 403us
toF16 318us -> 304us
There's no reason for toSRGB or to2Dot2 to change.
The refactor seems to have caused the compiler to
order the instructions a little differently...
This is something to come back to if we need to
squeeze more performance out of sRGB. For now,
let's not be held up by something we don't control.
F16 likely improves because we are no longer
(unnecessarily) building the linear tables.
Code size gets a little bigger. Measuring
SkColorSpaceXform size as a percentage of src/ size,
we go from 0.8% to 1.4%.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002
Committed: https://skia.googlesource.com/skia/+/8bbcd5aab81dc0742c3367479c0c9d97363b1203
Patch Set 1 #Patch Set 2 : Fix bench #
Total comments: 11
Patch Set 3 : Response to comments #Patch Set 4 : More refactoring #
Depends on Patchset: Messages
Total messages: 22 (15 generated)
Description was changed from ========== Avoid building tables for linear color xforms BUG=skia: ========== to ========== Avoid building tables for linear color xforms BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Avoid building tables for linear color xforms BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 ========== to ========== Reduce overhead for linear color xforms We used to build src and dst transfer fn tables every time a new xform was created with linear src and dst. Now we don't compute them because we don't need them. This will make SkColorSpaceXform a far better option for any xforms with float or half-float inputs or outputs, particularly on a small number of pixels. This CL also moves SkColorSpaceXform closer to what I anticipate will be the eventual 'API design'. I think apply() will want to take a SrcColorType enum (not created yet because it's not necessary yet) and a DstColorType enum (still using SkColorType because there's not yet a reason not to). Performance is unchanged - except for an improvement in xforms to linear F16. This is actually bigger than anticipated - still trying to understand why. Code size is not great. Measuring SkColorSpaceXform size as a percentage of src/ size, we go from 0.8% to 1.8%. Looking into how we might improve this. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 ==========
msarett@google.com changed reviewers: + brianosman@google.com, mtklein@google.com
Description was changed from ========== Reduce overhead for linear color xforms We used to build src and dst transfer fn tables every time a new xform was created with linear src and dst. Now we don't compute them because we don't need them. This will make SkColorSpaceXform a far better option for any xforms with float or half-float inputs or outputs, particularly on a small number of pixels. This CL also moves SkColorSpaceXform closer to what I anticipate will be the eventual 'API design'. I think apply() will want to take a SrcColorType enum (not created yet because it's not necessary yet) and a DstColorType enum (still using SkColorType because there's not yet a reason not to). Performance is unchanged - except for an improvement in xforms to linear F16. This is actually bigger than anticipated - still trying to understand why. Code size is not great. Measuring SkColorSpaceXform size as a percentage of src/ size, we go from 0.8% to 1.8%. Looking into how we might improve this. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 ========== to ========== Reduce overhead for linear color xforms We used to build src and dst transfer fn tables every time a new xform was created with linear src and dst. Now we don't compute them because we don't need them. This will make SkColorSpaceXform a far better option for any xforms with float or half-float inputs or outputs, particularly on a small number of pixels. This CL also moves SkColorSpaceXform closer to what I anticipate will be the eventual 'API design'. I think apply() will want to take a SrcColorType enum (not created yet because it's not necessary yet) and a DstColorType enum (still using SkColorType because there's not yet a reason not to). Performance is unchanged - except for a small improvement in xforms to linear F16. Code size is not great. Measuring SkColorSpaceXform size as a percentage of src/ size, we go from 0.8% to 1.8%. Looking into how we might improve this. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 ==========
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:454: if (srcSpace->gammaIsLinear()) { Seems impossible? https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:464: if (srcSpace->gammaIsLinear()) { Ditto. https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:478: } else { Ditto. https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:484: if (srcSpace->gammaIsLinear()) { Ditto. https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:1076: Seems like you could really reduce the code size impact by splitting this function right here. Move everything below into a helper that's only templated on kAlphaType, kCSM, and kSwapRB. Pass in the function pointers from the above initialization. Profit.
https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:454: if (srcSpace->gammaIsLinear()) { On 2016/09/13 13:39:30, Brian Osman wrote: > Seems impossible? Yes of course. Thanks, I missed this. https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:464: if (srcSpace->gammaIsLinear()) { On 2016/09/13 13:39:30, Brian Osman wrote: > Ditto. Done. https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:478: } else { On 2016/09/13 13:39:30, Brian Osman wrote: > Ditto. Done. https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:484: if (srcSpace->gammaIsLinear()) { On 2016/09/13 13:39:30, Brian Osman wrote: > Ditto. Done. https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:1076: On 2016/09/13 13:39:30, Brian Osman wrote: > Seems like you could really reduce the code size impact by splitting this > function right here. Move everything below into a helper that's only templated > on kAlphaType, kCSM, and kSwapRB. Pass in the function pointers from the above > initialization. Profit. I like this change in terms of design and it makes code size smaller (1.4% now). I've gone ahead and made it. It also causes one of my performance tests to get slower. Not necessarily a crisis, but I need to understand why - it's a bit strange given that everything gets inlined anyway...
Description was changed from ========== Reduce overhead for linear color xforms We used to build src and dst transfer fn tables every time a new xform was created with linear src and dst. Now we don't compute them because we don't need them. This will make SkColorSpaceXform a far better option for any xforms with float or half-float inputs or outputs, particularly on a small number of pixels. This CL also moves SkColorSpaceXform closer to what I anticipate will be the eventual 'API design'. I think apply() will want to take a SrcColorType enum (not created yet because it's not necessary yet) and a DstColorType enum (still using SkColorType because there's not yet a reason not to). Performance is unchanged - except for a small improvement in xforms to linear F16. Code size is not great. Measuring SkColorSpaceXform size as a percentage of src/ size, we go from 0.8% to 1.8%. Looking into how we might improve this. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 ========== to ========== Reduce overhead for linear color xforms We used to build src and dst transfer fn tables every time a new xform was created with linear src and dst. Now we don't compute them because we don't need them. This will make SkColorSpaceXform a far better option for any xforms with float or half-float inputs or outputs, particularly on a small number of pixels. This CL also moves SkColorSpaceXform closer to what I anticipate will be the eventual 'API design'. I think apply() will want to take a SrcColorType enum (not created yet because it's not necessary yet) and a DstColorType enum (still using SkColorType because there's not yet a reason not to). Performance changes: toSRGB 341us -> 366us to2Dot2 404us -> 403us toF16 318us -> 304us There's no reason for toSRGB or to2Dot2 to change. The refactor seems to have caused the compiler to order the instructions a little differently... This is something to come back to if we need to squeeze more performance out of sRGB. For now, let's not be held up by something we don't control. F16 likely improves because we are no longer (unnecessarily) building the linear tables. Code size gets a little bigger. Measuring SkColorSpaceXform size as a percentage of src/ size, we go from 0.8% to 1.4%. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 ==========
Description was changed from ========== Reduce overhead for linear color xforms We used to build src and dst transfer fn tables every time a new xform was created with linear src and dst. Now we don't compute them because we don't need them. This will make SkColorSpaceXform a far better option for any xforms with float or half-float inputs or outputs, particularly on a small number of pixels. This CL also moves SkColorSpaceXform closer to what I anticipate will be the eventual 'API design'. I think apply() will want to take a SrcColorType enum (not created yet because it's not necessary yet) and a DstColorType enum (still using SkColorType because there's not yet a reason not to). Performance changes: toSRGB 341us -> 366us to2Dot2 404us -> 403us toF16 318us -> 304us There's no reason for toSRGB or to2Dot2 to change. The refactor seems to have caused the compiler to order the instructions a little differently... This is something to come back to if we need to squeeze more performance out of sRGB. For now, let's not be held up by something we don't control. F16 likely improves because we are no longer (unnecessarily) building the linear tables. Code size gets a little bigger. Measuring SkColorSpaceXform size as a percentage of src/ size, we go from 0.8% to 1.4%. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 ========== to ========== Reduce overhead for linear color xforms We used to build src and dst transfer fn tables every time a new xform was created with linear src and dst. Now we don't compute them because we don't need them. This will make SkColorSpaceXform a far better option for any xforms with float or half-float inputs or outputs, particularly on a small number of pixels. This CL also moves SkColorSpaceXform closer to what I anticipate will be the eventual 'API design'. I think apply() will want to take a SrcColorType enum (not created yet because it's not necessary yet) and a DstColorType enum (still using SkColorType because there's not yet a reason not to). Performance changes: toSRGB 341us -> 366us to2Dot2 404us -> 403us toF16 318us -> 304us There's no reason for toSRGB or to2Dot2 to change. The refactor seems to have caused the compiler to order the instructions a little differently... This is something to come back to if we need to squeeze more performance out of sRGB. For now, let's not be held up by something we don't control. F16 likely improves because we are no longer (unnecessarily) building the linear tables. Code size gets a little bigger. Measuring SkColorSpaceXform size as a percentage of src/ size, we go from 0.8% to 1.4%. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 ==========
Alright I'm happy with this again... https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2335723002/diff/60001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:1076: On 2016/09/13 15:55:57, msarett wrote: > On 2016/09/13 13:39:30, Brian Osman wrote: > > Seems like you could really reduce the code size impact by splitting this > > function right here. Move everything below into a helper that's only templated > > on kAlphaType, kCSM, and kSwapRB. Pass in the function pointers from the above > > initialization. Profit. > > I like this change in terms of design and it makes code size smaller (1.4% now). > I've gone ahead and made it. > > It also causes one of my performance tests to get slower. Not necessarily a > crisis, but I need to understand why - it's a bit strange given that everything > gets inlined anyway... After looking into this futher, I've decided not to care (for now). From the new commit message: """ Performance changes: toSRGB 341us -> 366us to2Dot2 404us -> 403us toF16 318us -> 304us There's no reason for toSRGB or to2Dot2 to change. The refactor seems to have caused the compiler to order the instructions a little differently... This is something to come back to if we need to squeeze more performance out of sRGB. For now, let's not be held up by something we don't control. F16 likely improves because we are no longer (unnecessarily) building the linear tables. """
lgtm
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
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 ========== Reduce overhead for linear color xforms We used to build src and dst transfer fn tables every time a new xform was created with linear src and dst. Now we don't compute them because we don't need them. This will make SkColorSpaceXform a far better option for any xforms with float or half-float inputs or outputs, particularly on a small number of pixels. This CL also moves SkColorSpaceXform closer to what I anticipate will be the eventual 'API design'. I think apply() will want to take a SrcColorType enum (not created yet because it's not necessary yet) and a DstColorType enum (still using SkColorType because there's not yet a reason not to). Performance changes: toSRGB 341us -> 366us to2Dot2 404us -> 403us toF16 318us -> 304us There's no reason for toSRGB or to2Dot2 to change. The refactor seems to have caused the compiler to order the instructions a little differently... This is something to come back to if we need to squeeze more performance out of sRGB. For now, let's not be held up by something we don't control. F16 likely improves because we are no longer (unnecessarily) building the linear tables. Code size gets a little bigger. Measuring SkColorSpaceXform size as a percentage of src/ size, we go from 0.8% to 1.4%. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 ========== to ========== Reduce overhead for linear color xforms We used to build src and dst transfer fn tables every time a new xform was created with linear src and dst. Now we don't compute them because we don't need them. This will make SkColorSpaceXform a far better option for any xforms with float or half-float inputs or outputs, particularly on a small number of pixels. This CL also moves SkColorSpaceXform closer to what I anticipate will be the eventual 'API design'. I think apply() will want to take a SrcColorType enum (not created yet because it's not necessary yet) and a DstColorType enum (still using SkColorType because there's not yet a reason not to). Performance changes: toSRGB 341us -> 366us to2Dot2 404us -> 403us toF16 318us -> 304us There's no reason for toSRGB or to2Dot2 to change. The refactor seems to have caused the compiler to order the instructions a little differently... This is something to come back to if we need to squeeze more performance out of sRGB. For now, let's not be held up by something we don't control. F16 likely improves because we are no longer (unnecessarily) building the linear tables. Code size gets a little bigger. Measuring SkColorSpaceXform size as a percentage of src/ size, we go from 0.8% to 1.4%. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2335723002 Committed: https://skia.googlesource.com/skia/+/8bbcd5aab81dc0742c3367479c0c9d97363b1203 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://skia.googlesource.com/skia/+/8bbcd5aab81dc0742c3367479c0c9d97363b1203 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
