Fix for png encoding performance regressions (part 2)

Fix for png encoding performance regressions (part 2)

The original fix was:

> 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.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was not applying the appropriate optimizations.

For the first fix to work, we need the compiler to inline
the particular function *and* optimize out the unnecessary
comparison with MAXINT.

I suggested to upstream that they just accept a more
portable fix, but that quickly became a philosophical
discussion of "code size vs. performance" and didn't
really go anywhere.  They suggested that altering the
compiler flags was an alternative, and they are right!

I've altered the build files to apply the necessary flags
on Android and Windows.  I think this is the best option,
though I'm not exactly sure if there's any problem with
overriding Chrome's default optimization flags.  Other
options (which I think are worse) include:
(1) Forking our copy of libpng to apply a non-compiler
    dependent fix.
(2) Accepting the regression.

(FWIW, I'm not 100% sure on Windows - it worked on my local
Windows box, but I can't get the Windows perf trybots
to work).

BUG=619850
BUG=599917

[msarett, 2016-07-01 12:43:48]

Description was changed from

==========
Fix for png encoding performance regressions (part 2)


BUG=
==========

to

==========
Fix for png encoding performance regressions (part 2)

The original fix was:

> 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.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was applying the appropriate optimizations.

For this to work, we need the compiler to inline the
particular function *and* optimize out the unnecessary
comparison with MAXINT.

I suggested to upstream that they just accept a more
portable fix, but that quickly became philosophical
discussion of "code size vs. performance" and didn't
really go anywhere.  They suggested that this could be
fixed by altering the compiler flags, and they are right!
(I'm not 100% sure on Windows - it worked on my local
Windows box, but I can't get the Windows perf trybots
to work).

I've altered the build files to apply the necessary flags
on Android and Windows.  I think this is the best option,
though I'm not exactly sure if there's any issue with
overriding Chrome's default optimization flags.  Other
options (which I think are worse) include:
(1) Forking our copy of libpng to apply a non-compiler
dependent fix.
(2) Accepting the regression.

BUG=619850
BUG=599917
==========

[msarett, 2016-07-01 12:44:09]

Description was changed from

==========
Fix for png encoding performance regressions (part 2)

The original fix was:

> 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.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was applying the appropriate optimizations.

For this to work, we need the compiler to inline the
particular function *and* optimize out the unnecessary
comparison with MAXINT.

I suggested to upstream that they just accept a more
portable fix, but that quickly became philosophical
discussion of "code size vs. performance" and didn't
really go anywhere.  They suggested that this could be
fixed by altering the compiler flags, and they are right!
(I'm not 100% sure on Windows - it worked on my local
Windows box, but I can't get the Windows perf trybots
to work).

I've altered the build files to apply the necessary flags
on Android and Windows.  I think this is the best option,
though I'm not exactly sure if there's any issue with
overriding Chrome's default optimization flags.  Other
options (which I think are worse) include:
(1) Forking our copy of libpng to apply a non-compiler
dependent fix.
(2) Accepting the regression.

BUG=619850
BUG=599917
==========

to

==========
Fix for png encoding performance regressions (part 2)

The original fix was:

> 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.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was not applying the appropriate optimizations.

For this to work, we need the compiler to inline the
particular function *and* optimize out the unnecessary
comparison with MAXINT.

I suggested to upstream that they just accept a more
portable fix, but that quickly became philosophical
discussion of "code size vs. performance" and didn't
really go anywhere.  They suggested that this could be
fixed by altering the compiler flags, and they are right!
(I'm not 100% sure on Windows - it worked on my local
Windows box, but I can't get the Windows perf trybots
to work).

I've altered the build files to apply the necessary flags
on Android and Windows.  I think this is the best option,
though I'm not exactly sure if there's any issue with
overriding Chrome's default optimization flags.  Other
options (which I think are worse) include:
(1) Forking our copy of libpng to apply a non-compiler
dependent fix.
(2) Accepting the regression.

BUG=619850
BUG=599917
==========

[msarett, 2016-07-01 12:44:39]

Description was changed from

==========
Fix for png encoding performance regressions (part 2)

The original fix was:

> 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.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was not applying the appropriate optimizations.

For this to work, we need the compiler to inline the
particular function *and* optimize out the unnecessary
comparison with MAXINT.

I suggested to upstream that they just accept a more
portable fix, but that quickly became philosophical
discussion of "code size vs. performance" and didn't
really go anywhere.  They suggested that this could be
fixed by altering the compiler flags, and they are right!
(I'm not 100% sure on Windows - it worked on my local
Windows box, but I can't get the Windows perf trybots
to work).

I've altered the build files to apply the necessary flags
on Android and Windows.  I think this is the best option,
though I'm not exactly sure if there's any issue with
overriding Chrome's default optimization flags.  Other
options (which I think are worse) include:
(1) Forking our copy of libpng to apply a non-compiler
dependent fix.
(2) Accepting the regression.

BUG=619850
BUG=599917
==========

to

==========
Fix for png encoding performance regressions (part 2)

The original fix was:

> 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.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was not applying the appropriate optimizations.

For the first fix to work, we need the compiler to inline
the particular function *and* optimize out the unnecessary
comparison with MAXINT.

I suggested to upstream that they just accept a more
portable fix, but that quickly became philosophical
discussion of "code size vs. performance" and didn't
really go anywhere.  They suggested that this could be
fixed by altering the compiler flags, and they are right!
(I'm not 100% sure on Windows - it worked on my local
Windows box, but I can't get the Windows perf trybots
to work).

I've altered the build files to apply the necessary flags
on Android and Windows.  I think this is the best option,
though I'm not exactly sure if there's any issue with
overriding Chrome's default optimization flags.  Other
options (which I think are worse) include:
(1) Forking our copy of libpng to apply a non-compiler
dependent fix.
(2) Accepting the regression.

BUG=619850
BUG=599917
==========

[msarett, 2016-07-01 12:45:32]

Description was changed from

==========
Fix for png encoding performance regressions (part 2)

The original fix was:

> 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.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was not applying the appropriate optimizations.

For the first fix to work, we need the compiler to inline
the particular function *and* optimize out the unnecessary
comparison with MAXINT.

I suggested to upstream that they just accept a more
portable fix, but that quickly became philosophical
discussion of "code size vs. performance" and didn't
really go anywhere.  They suggested that this could be
fixed by altering the compiler flags, and they are right!
(I'm not 100% sure on Windows - it worked on my local
Windows box, but I can't get the Windows perf trybots
to work).

I've altered the build files to apply the necessary flags
on Android and Windows.  I think this is the best option,
though I'm not exactly sure if there's any issue with
overriding Chrome's default optimization flags.  Other
options (which I think are worse) include:
(1) Forking our copy of libpng to apply a non-compiler
dependent fix.
(2) Accepting the regression.

BUG=619850
BUG=599917
==========

to

==========
Fix for png encoding performance regressions (part 2)

The original fix was:

> 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.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was not applying the appropriate optimizations.

For the first fix to work, we need the compiler to inline
the particular function *and* optimize out the unnecessary
comparison with MAXINT.

I suggested to upstream that they just accept a more
portable fix, but that quickly became philosophical
discussion of "code size vs. performance" and didn't
really go anywhere.  They suggested that this could be
fixed by altering the compiler flags, and they are right!
(I'm not 100% sure on Windows - it worked on my local
Windows box, but I can't get the Windows perf trybots
to work).

I've altered the build files to apply the necessary flags
on Android and Windows.  I think this is the best option,
though I'm not exactly sure if there's any issue with
overriding Chrome's default optimization flags.  Other
options (which I think are worse) include:
(1) Forking our copy of libpng to apply a non-compiler
    dependent fix.
(2) Accepting the regression.

BUG=619850
BUG=599917
==========

[msarett, 2016-07-01 12:47:33]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 12:48:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msarett, 2016-07-01 12:49:50]

Description was changed from

==========
Fix for png encoding performance regressions (part 2)

The original fix was:

> 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.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was not applying the appropriate optimizations.

For the first fix to work, we need the compiler to inline
the particular function *and* optimize out the unnecessary
comparison with MAXINT.

I suggested to upstream that they just accept a more
portable fix, but that quickly became philosophical
discussion of "code size vs. performance" and didn't
really go anywhere.  They suggested that this could be
fixed by altering the compiler flags, and they are right!
(I'm not 100% sure on Windows - it worked on my local
Windows box, but I can't get the Windows perf trybots
to work).

I've altered the build files to apply the necessary flags
on Android and Windows.  I think this is the best option,
though I'm not exactly sure if there's any issue with
overriding Chrome's default optimization flags.  Other
options (which I think are worse) include:
(1) Forking our copy of libpng to apply a non-compiler
    dependent fix.
(2) Accepting the regression.

BUG=619850
BUG=599917
==========

to

==========
Fix for png encoding performance regressions (part 2)

The original fix was:

> 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.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was not applying the appropriate optimizations.

For the first fix to work, we need the compiler to inline
the particular function *and* optimize out the unnecessary
comparison with MAXINT.

I suggested to upstream that they just accept a more
portable fix, but that quickly became a philosophical
discussion of "code size vs. performance" and didn't
really go anywhere.  They suggested that altering the
compiler flags was an alternative, and they are right!

I've altered the build files to apply the necessary flags
on Android and Windows.  I think this is the best option,
though I'm not exactly sure if there's any problem with
overriding Chrome's default optimization flags.  Other
options (which I think are worse) include:
(1) Forking our copy of libpng to apply a non-compiler
    dependent fix.
(2) Accepting the regression.

(FWIW, I'm not 100% sure on Windows - it worked on my local
Windows box, but I can't get the Windows perf trybots
to work).

BUG=619850
BUG=599917
==========

[msarett, 2016-07-01 13:12:29]

msarett@google.com changed reviewers:
+ scroggo@chromium.org, thakis@chromium.org

[msarett, 2016-07-01 13:12:30]

Finally was able to get up and running on Windows to test this.  Please take a
look!

[commit-bot: I haz the power, 2016-07-01 13:16:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-01 13:16:54]

Dry run: Try jobs failed on following builders:
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[msarett, 2016-07-01 13:31:39]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 13:31:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2016-07-01 14:12:33]

It seems extremely fragile to require such specific compiler flags.

Can we carry this as downstream diff instead?

https://codereview.chromium.org/2119813003/diff/20001/third_party/libpng/BUIL...
File third_party/libpng/BUILD.gn (right):

https://codereview.chromium.org/2119813003/diff/20001/third_party/libpng/BUIL...
third_party/libpng/BUILD.gn:52: if (is_win && !is_clang) {
why `!is_clang`?

[msarett, 2016-07-01 14:44:33]

> It seems extremely fragile to require such specific compiler flags.
> Can we carry this as downstream diff instead?

I agree that this is a bit fragile.  I'm not thrilled with this solution.  Maybe
we can view it is as temporary (until we use clang everywhere)?

I'm pretty strongly against maintaining downstream diffs if we can avoid it at
all.  It seems like we always end up in a vicious circle where:
(1) Nobody wants to update the library because it's hard/complicated.
(2) Nobody updates the library and it gets really old.
(3) Updating the library is now really, really hard...

https://codereview.chromium.org/2119813003/diff/20001/third_party/libpng/BUIL...
File third_party/libpng/BUILD.gn (right):

https://codereview.chromium.org/2119813003/diff/20001/third_party/libpng/BUIL...
third_party/libpng/BUILD.gn:52: if (is_win && !is_clang) {
On 2016/07/01 14:12:33, Nico wrote:
> why `!is_clang`?

Possibly this is unnecessary...  I'll remove if you think it's best.

Clang builds on Mac/Linux were fixed without changing any compiler flags, so I'm
assuming that there is no need for this flag on Windows clang.  But I'm actually
not sure.  And I think that the perf bots are MSVC (not that I can get them to
work anyway), so I don't know how to test Win clang.

But this feels right because I'd expect that if/when Windows is switched to
clang, we'll catch regressions.

[Nico, 2016-07-01 14:53:59]

On 2016/07/01 14:44:33, msarett wrote:
> > It seems extremely fragile to require such specific compiler flags.
> > Can we carry this as downstream diff instead?
> 
> I agree that this is a bit fragile.  I'm not thrilled with this solution. 
Maybe
> we can view it is as temporary (until we use clang everywhere)?

"clang everywhere" is also relying on compiler flags :-)

> I'm pretty strongly against maintaining downstream diffs if we can avoid it at
> all.

I completely, violently agree with this.

But here, upstream seems to make a, hm, surprising decision. It sounds like
they're fine with regressing 20% performance for all their clients unless all
clients tweak their flags (and then carefully watch their benchmarks every time
they tweak libpng).

Ideally upstream would do the pragmatic thing and we could merge that.

Failing that, we have the choice of this patch and a downstream diff, which both
aren't great. Since we play with global compiler flags quite a bit (LTO, LTCG,
CFI, what have you), I think in the limit it seems better if each of our
third-party project had uniform compiler flags and a small downstream diff than
the other way round. Another way to think about this is that we can either have
to code say something inefficient and then hope that the compiler undoes that,
or we could have the code be efficient in the first place.

>  It seems like we always end up in a vicious circle where:
> (1) Nobody wants to update the library because it's hard/complicated.
> (2) Nobody updates the library and it gets really old.
> (3) Updating the library is now really, really hard...
> 
>
https://codereview.chromium.org/2119813003/diff/20001/third_party/libpng/BUIL...
> File third_party/libpng/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2119813003/diff/20001/third_party/libpng/BUIL...
> third_party/libpng/BUILD.gn:52: if (is_win && !is_clang) {
> On 2016/07/01 14:12:33, Nico wrote:
> > why `!is_clang`?
> 
> Possibly this is unnecessary...  I'll remove if you think it's best.

In general, I think it's good to special-case clang on Windows as little as
possible.

> Clang builds on Mac/Linux were fixed without changing any compiler flags, so
I'm
> assuming that there is no need for this flag on Windows clang.  But I'm
actually
> not sure.  And I think that the perf bots are MSVC (not that I can get them to
> work anyway), so I don't know how to test Win clang.
> 
> But this feels right because I'd expect that if/when Windows is switched to
> clang, we'll catch regressions.

[commit-bot: I haz the power, 2016-07-01 15:10:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-01 15:10:44]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-07-01 16:12:14]

Thanks for your thoughts.  Definitely understand where you're coming from.  I'm
going to try a few experiments to see if there's any way to avoid forking...

Otherwise, I'll upload that option next.