Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2062423002:
Fix performance regression in png encoding caused by libpng update (Closed)
DescriptionFix performance regression in png encoding caused by libpng update
libpng calculates "filter heuristics" to choose which png
filter to apply. Chrome need not calculate these "filter
heuristics" because we always choose to use the SUB filter,
for speed.
libpng 1.6 refactors the SUB filter to share code, also
forcing us to calculate the filter heuristics, even though
we don't need to. This caused a performance regression.
This CL cherry picks a fix from upstream that:
(1) Passes PNG_SIZE_MAX to the filter code. This allows
the compiler to optimize out the heuristic calculation
code, fixing the performance regression.
(2) Fixes overflow handling in the calculation of filter
heuristics. This won't affect Chrome.
BUG=619850
BUG=599917
Committed: https://crrev.com/b43ad19eed3d9b660f2fc3e445e250a1b42b6b93
Cr-Commit-Position: refs/heads/master@{#401298}
Patch Set 1 #Patch Set 2 : Add fixes to libpng #
Total comments: 9
Patch Set 3 : Clarify comments in PNGImageEncoder #Patch Set 4 : Fix win #
Messages
Total messages: 43 (21 generated)
msarett@google.com changed reviewers: + dglazkov@chromium.org, jochen@chromium.org, thakis@chromium.org
third_party/WebKit/ dglazkov@ third_party/libpng/ jochen@ Is there anything I need to do to make sure that this commit ships with the original libpng update (https://codereview.chromium.org/2021403002/)?
jochen@chromium.org changed reviewers: + mdempsky@chromium.org, noel@chromium.org
+noel/mdempsky who might be better reviewers for this
Can you make the CL description a bit longer? It looks like this cherry-picks an upstream patch, but to get to the "what does this patch do and why?" I had to click a bug, from there to a github issue linking to a pull request, where the authors say "this isn't the right fix, look at this other thing instead" and then I had to read a somewhat lengthy patch. maybe you could summarize what's happening directly in the cl description.
Description was changed from ========== Fix performance regression in png encoding caused by libpng update BUG=619850 BUG=599917 ========== to ========== Fix performance regression in png encoding caused by libpng update libpng calculates "filter heuristics" to choose which png filter to apply. Chrome need not calculate these "filter heuristics" because we always choose to use the SUB filter, for speed. libpng 1.6 refactors the SUB filter to share code, also forcing us to calculate the filter heuristics, even though we don't need to. This caused a performance regression. This CL cherry picks a fix from upstream that: (1) Passes PNG_SIZE_MAX to the filter code. This allows the compiler to optimize out the heuristic calculation code, fixing the performance regression. (2) Fixes overflow handling in the calculation of filter heuristics. This won't affect Chrome. BUG=619850 BUG=599917 ==========
On 2016/06/21 12:39:13, Nico wrote: > Can you make the CL description a bit longer? It looks like this cherry-picks an > upstream patch, but to get to the "what does this patch do and why?" I had to > click a bug, from there to a github issue linking to a pull request, where the > authors say "this isn't the right fix, look at this other thing instead" and > then I had to read a somewhat lengthy patch. maybe you could summarize what's > happening directly in the cl description. Yes I've updated the description, sorry this became a bit unclear.
msarett@google.com changed reviewers: + scroggo@chromium.org
+scroggo Please take a look!
https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp (left): https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp:61: // The parameters are the same as what libpng uses by default for RGB and RGBA images, except: I think we've lost some information here. Why did you delete this line? https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp (right): https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp:63: // The zlib compression level is set to 3 instead of the default, to avoid the lazy Ziv-Lempel match searching. Why not say "instead of 6", like we did before? With the line you deleted, it makes it clear that 6 is the default. https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp:66: // The zlib memory level is set to 8, which matches the default. Why do we need to set if it is the default? Do we have to set everything if we set some? https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp:75: // The delta filter is 1 ("sub") instead of 5 ("all"), to reduce the filter computations. I think this comment was out of date before you got here. We no longer pass numbers, and the names don't quite match the macro names. I think this is clearer as // The delta filter is set to PNG_FILTER_SUB instead of PNG_ALL_FILTERS to // reduce the filter computations.
https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp (left): https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp:61: // The parameters are the same as what libpng uses by default for RGB and RGBA images, except: On 2016/06/21 17:39:43, scroggo_chromium wrote: > I think we've lost some information here. Why did you delete this line? The first time I read it, I thought it was referring to the options below. Now that I look again it can be interpreted as referring to any other png encoding options that aren't set here. I've added it back in. https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp (right): https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp:63: // The zlib compression level is set to 3 instead of the default, to avoid the lazy Ziv-Lempel match searching. On 2016/06/21 17:39:44, scroggo_chromium wrote: > Why not say "instead of 6", like we did before? With the line you deleted, it > makes it clear that 6 is the default. Done. https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp:66: // The zlib memory level is set to 8, which matches the default. On 2016/06/21 17:39:44, scroggo_chromium wrote: > Why do we need to set if it is the default? Do we have to set everything if we > set some? We now set everything that the past code comments deemed to be important. An old comment says: "// The zlib memory level (8) and strategy (Z_FILTERED) will be set inside libpng." Z_FILTERED is no longer the default in libpng 1.6 (so the update caused a behavior change). I think it makes sense to set everything we care about, so future updates don't change behavior we were specifically expecting. https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp:75: // The delta filter is 1 ("sub") instead of 5 ("all"), to reduce the filter computations. On 2016/06/21 17:39:44, scroggo_chromium wrote: > I think this comment was out of date before you got here. We no longer pass > numbers, and the names don't quite match the macro names. > > I think this is clearer as > > // The delta filter is set to PNG_FILTER_SUB instead of PNG_ALL_FILTERS to > // reduce the filter computations. You're right. Done.
lgtm https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp (right): https://codereview.chromium.org/2062423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp:66: // The zlib memory level is set to 8, which matches the default. On 2016/06/21 19:06:33, msarett wrote: > On 2016/06/21 17:39:44, scroggo_chromium wrote: > > Why do we need to set if it is the default? Do we have to set everything if we > > set some? > > We now set everything that the past code comments deemed to be important. +1. Comments can get out of date more easily than code, so this sounds good. > > An old comment says: > > "// The zlib memory level (8) and strategy (Z_FILTERED) will be set inside > libpng." > > Z_FILTERED is no longer the default in libpng 1.6 (so the update caused a > behavior change). I think it makes sense to set everything we care about, so > future updates don't change behavior we were specifically expecting.
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/patch-status/2062423002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/patch-status/2062423002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #4 (id:60001) 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/patch-status/2062423002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #4 (id:80001) 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/patch-status/2062423002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Patchset #4 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062423002/120001
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 thakis@chromium.org, scroggo@chromium.org Link to the patchset: https://codereview.chromium.org/2062423002/#ps120001 (title: "Fix win")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062423002/120001
Message was sent while issue was closed.
Description was changed from ========== Fix performance regression in png encoding caused by libpng update libpng calculates "filter heuristics" to choose which png filter to apply. Chrome need not calculate these "filter heuristics" because we always choose to use the SUB filter, for speed. libpng 1.6 refactors the SUB filter to share code, also forcing us to calculate the filter heuristics, even though we don't need to. This caused a performance regression. This CL cherry picks a fix from upstream that: (1) Passes PNG_SIZE_MAX to the filter code. This allows the compiler to optimize out the heuristic calculation code, fixing the performance regression. (2) Fixes overflow handling in the calculation of filter heuristics. This won't affect Chrome. BUG=619850 BUG=599917 ========== to ========== Fix performance regression in png encoding caused by libpng update libpng calculates "filter heuristics" to choose which png filter to apply. Chrome need not calculate these "filter heuristics" because we always choose to use the SUB filter, for speed. libpng 1.6 refactors the SUB filter to share code, also forcing us to calculate the filter heuristics, even though we don't need to. This caused a performance regression. This CL cherry picks a fix from upstream that: (1) Passes PNG_SIZE_MAX to the filter code. This allows the compiler to optimize out the heuristic calculation code, fixing the performance regression. (2) Fixes overflow handling in the calculation of filter heuristics. This won't affect Chrome. BUG=619850 BUG=599917 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fix performance regression in png encoding caused by libpng update libpng calculates "filter heuristics" to choose which png filter to apply. Chrome need not calculate these "filter heuristics" because we always choose to use the SUB filter, for speed. libpng 1.6 refactors the SUB filter to share code, also forcing us to calculate the filter heuristics, even though we don't need to. This caused a performance regression. This CL cherry picks a fix from upstream that: (1) Passes PNG_SIZE_MAX to the filter code. This allows the compiler to optimize out the heuristic calculation code, fixing the performance regression. (2) Fixes overflow handling in the calculation of filter heuristics. This won't affect Chrome. BUG=619850 BUG=599917 ========== to ========== Fix performance regression in png encoding caused by libpng update libpng calculates "filter heuristics" to choose which png filter to apply. Chrome need not calculate these "filter heuristics" because we always choose to use the SUB filter, for speed. libpng 1.6 refactors the SUB filter to share code, also forcing us to calculate the filter heuristics, even though we don't need to. This caused a performance regression. This CL cherry picks a fix from upstream that: (1) Passes PNG_SIZE_MAX to the filter code. This allows the compiler to optimize out the heuristic calculation code, fixing the performance regression. (2) Fixes overflow handling in the calculation of filter heuristics. This won't affect Chrome. BUG=619850 BUG=599917 Committed: https://crrev.com/b43ad19eed3d9b660f2fc3e445e250a1b42b6b93 Cr-Commit-Position: refs/heads/master@{#401298} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b43ad19eed3d9b660f2fc3e445e250a1b42b6b93 Cr-Commit-Position: refs/heads/master@{#401298} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
