1276127 - Enable SSE optimization in the in-tree libpng

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Glenn Randers-Pehrson]

Libpng-1.6.22 offers optimized support for Intel-SSE platforms.  It's not enabled by default.

Comment 1

[Glenn Randers-Pehrson]

In bug # 1275901 (update embedded libpng to version 1.6.23) comment #14 Seth commented:

I did want to bring to your attention that for now, at least, we are still supporting machines that have SSE but not SSE2. I just wanted to make sure you've thought about this, as we've seen some nasty issues related to running SSE2 code on SSE-only machines in the past.

Comment 2

[Glenn Randers-Pehrson]

Attachment: v00-1276127-part01-add-sse-directory-to-intree-libpng

Comment 3

[Glenn Randers-Pehrson]

Attachment: v00-1276127-part02-sse-update-pngpriv

Comment 4

[Glenn Randers-Pehrson]

Attachment: v00-1276127-part03-sse-mozbuild

Comment 5

[Glenn Randers-Pehrson]

Attachment: v00-1276127-part04-sse-configure

Please submit all 4 parts together to try.

Comment 6

[Glenn Randers-Pehrson]

Seth wrote in comment #1
> I did want to bring to your attention that for now, at least, we are still
> supporting machines that have SSE but not SSE2. I just wanted to make sure
> you've thought about this, as we've seen some nasty issues related to
> running SSE2 code on SSE-only machines in the past.

There is code in pngpriv.h that tries to sort out the various levels of SSE support.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

applying v00-1276127-part01-add-sse-directory-to-intree-libpng.diff
unable to find 'media/libpng/sse2/intel_init.c' for patching

Comment 8

[Glenn Randers-Pehrson]

Attachment: v01-1276127-part01-add-sse-directory-to-intree-libpng

diff against /dev/null instead of existing media/libpng/sse2 directory
Please retry all parts together

Comment 9

[Glenn Randers-Pehrson]

Attachment: v00-1276127-part05-sse-license

Comment 10

[Glenn Randers-Pehrson]

Attachment: v01-1276127-part04-sse-configure

Simplified SSE and ARM selection in old-configure.in

Comment 11

[Glenn Randers-Pehrson]

Attachment: v02-1276127-part04-sse-configure

Further simplify SSE/ARM selection in old-configure.in

Comment 12

[Glenn Randers-Pehrson]

Attachment: v00-1276127-part06-sse-changes

Comment 13

[Glenn Randers-Pehrson]

Attachment: v01-1276127-part02-sse-update-pngpriv

Conditionally compile sse2 headers

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efa537a529fb

Sorry for the extra commits in that patch queue. They're not going to hurt anything.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8776423 [details] [diff] [review]
v00-1276127-part06-sse-changes

The commit message calls this part 5

Comment 16

[Glenn Randers-Pehrson]

Attachment: v01-1276127-part06-sse-changes

Fixed typo in part06 checkin message

Comment 17

[Glenn Randers-Pehrson]

Comment on attachment 8775998 [details] [diff] [review]
v01-1276127-part01-add-sse-directory-to-intree-libpng

Try is all green and logs show SSE2 support was built.

Comment 18

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8775786 [details] [diff] [review]
v00-1276127-part03-sse-mozbuild

Redirecting to Jeff.

Comment 19

[Glenn Randers-Pehrson]

Attachment: v03-1276127-part04-sse-old.configure

Rebased.

Comment 20

[Glenn Randers-Pehrson]

Attachment: v00-1276127-part07-sse-moz.configure

Split from part04

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8775786 [details] [diff] [review]
v00-1276127-part03-sse-mozbuild

Review of attachment 8775786 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libpng/moz.build
@@ +37,5 @@
>      SOURCES += [
>          'arm/filter_neon.S'
>      ]
>  
> +if CONFIG['MOZ_PNG_INTEL_SSE']:

I think this can be:
if CONFIG['INTEL_ARCHITECTURE']:

I wasn't able to find out what's supposed to set MOZ_PNG_INTEL_SSE

Comment 22

[Glenn Randers-Pehrson]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> Comment on attachment 8775786 [details] [diff] [review]
> v00-1276127-part03-sse-mozbuild
> 
> Review of attachment 8775786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libpng/moz.build
> @@ +37,5 @@
> >      SOURCES += [
> >          'arm/filter_neon.S'
> >      ]
> >  
> > +if CONFIG['MOZ_PNG_INTEL_SSE']:
> 
> I think this can be:
> if CONFIG['INTEL_ARCHITECTURE']:
> 
> I wasn't able to find out what's supposed to set MOZ_PNG_INTEL_SSE

It's set in old-configure.in (patch part 04) and can be disabled
with  --enable-png-intel-sse-support=(yes/no)

Comment 23

[Glenn Randers-Pehrson]

Attachment: v03-1276127-part04-sse-old.configure

Move --enable-png-sse-support (and --enable-png-arm-neon-support) test outside the "case CPU_ARCH" test so user can override the result.  So:
PNG_INTEL_SSE_SUPPORT is 0 by default, then 1 on x86 or x86-64, and finally 0 or 1 if --enable/--disable is present.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

(In reply to Glenn Randers-Pehrson from comment #23)
> Created attachment 8778023 [details] [diff] [review]
> v03-1276127-part04-sse-old.configure
> 
> Move --enable-png-sse-support (and --enable-png-arm-neon-support) test
> outside the "case CPU_ARCH" test so user can override the result.  So:
> PNG_INTEL_SSE_SUPPORT is 0 by default, then 1 on x86 or x86-64, and finally
> 0 or 1 if --enable/--disable is present.

Is there any reason we would want to disable SSE support?

Comment 25

[Glenn Randers-Pehrson]

We might want to allow users to disable SSE support for debugging or performance testing purposes.

Comment 26

[Jeff Muizelaar [:jrmuizel]]

(In reply to Glenn Randers-Pehrson from comment #25)
> We might want to allow users to disable SSE support for debugging or
> performance testing purposes.

There's no precedent for this in other libraries that we support. I think being able to to disable it from the moz.build files is probably sufficient.

Also, do you mind posting the performance results from these changes?

Comment 27

[Glenn Randers-Pehrson]

There's precedent in the way ARM-NEON support has been handled by the PNG decoder, but I can easily get rid of the enable/disable feature for both ARM-NEON and INTEL-SSE2.  I don't have performance results for Firefox but I've tested with an instrumented version of pngcrush, but I've got to rerun those with libpng-1.6.24 now.

Comment 28

[Glenn Randers-Pehrson]

Attachment: v04-1276127-part04-sse-old-configure

Rebased old-configure.in

Comment 29

[Glenn Randers-Pehrson]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #26)
> Also, do you mind posting the performance results from these changes?

Here are some.  There's a bigger effect with PAETH-filtered images, and
not much effect on typical icons:

pngcrush_183_uxxx Not SSE2 Optimized
pngcrush_183_usxx SSE2 Optimized
HRT means "High resolution timing", i.e., clock_gettime().

Linux 4.4.0-31-generic #50-Ubuntu
gcc (Ubuntu 5.4.0-6ubuntu1~16.04.1) 5.4.0 20160609

Timing tests for firefox linux themes (10 times *.png)
pngcrush_183_uxxx: HRT time decode 0.2474, other 0.0739, total 0.3300 sec
pngcrush_183_usxx: HRT time decode 0.2444, other 0.0715, total 0.3245 sec

Timing tests for firefox branding (20 times *.png)
pngcrush_183_uxxx: HRT time decode 0.2300, other 0.0707, total 0.3096 sec
pngcrush_183_usxx: HRT time decode 0.2098, other 0.0545, total 0.2731 sec

Timing tests for W-rgb-M147.png (100000x159, PAETH filtered)
pngcrush_183_uxxx: HRT time decode 0.5648, other 0.1914, total 0.7563 sec
pngcrush_183_usxx: HRT time decode 0.4401, other 0.1927, total 0.6328 sec

Timing tests for W-rgba-M147.png (100000x159, PAETH filtered)
pngcrush_183_uxxx: HRT time decode 0.7682, other 0.2513, total 1.0195 sec
pngcrush_183_usxx: HRT time decode 0.5496, other 0.2524, total 0.8020 sec

SunOS 5.10 Generic_147441-13 i86pc i386 i86pc
gcc (GCC) 4.7.1

Timing tests for firefox linux themes (10 times *.png)
pngcrush_183_uxxx: HRT time decode 0.2910, other 0.0434, total 0.3451 sec
pngcrush_183_usxx: HRT time decode 0.2848, other 0.0437, total 0.3391 sec

Timing tests for firefox branding (20 times *.png)
pngcrush_183_uxxx: HRT time decode 0.2499, other 0.0367, total 0.2985 sec
pngcrush_183_usxx: HRT time decode 0.2467, other 0.0364, total 0.2948 sec

Timing tests for W-rgb-M147.png (100000x159, PAETH filtered)
pngcrush_183_uxxx: HRT time decode 0.7805, other 0.1473, total 0.9279 sec
pngcrush_183_usxx: HRT time decode 0.7699, other 0.1456, total 0.9156 sec

Timing tests for W-rgba-M147.png (100000x159, PAETH filtered)
pngcrush_183_uxxx: HRT time decode 1.0327, other 0.1962, total 1.2290 sec
pngcrush_183_usxx: HRT time decode 0.8255, other 0.1940, total 1.0196 sec

Comment 30

[Jeff Muizelaar [:jrmuizel]]

(In reply to Glenn Randers-Pehrson from comment #27)
> There's precedent in the way ARM-NEON support has been handled by the PNG
> decoder, but I can easily get rid of the enable/disable feature for both
> ARM-NEON and INTEL-SSE2.

Yeah, let's not add it for INTEL-SSE2. You can drop the ARM-NEON stuff in another bug. The patches mostly look good. In the next version of patch please only separate out the parts that are actually independent. e.g there's no point in splitting out the licensing changes from the other ones. I expect the whole thing probably makes the most sense as a single patch.

Comment 31

[Glenn Randers-Pehrson]

Attachment: v02-1276127-part01-add-sse-directory-to-intree-libpng

Update to libpng-1.6.24

Comment 32

[Glenn Randers-Pehrson]

Attachment: v00-1276127-part02-sse-intree-libpng

Combined patches, update to libpng-1.6.24, remove configuration and use INTEL_ARCHITECTURE instead.

Need another try run since so much is changed.

Comment 33

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8780164 [details] [diff] [review]
v00-1276127-part02-sse-intree-libpng

Review of attachment 8780164 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8780163 [details] [diff] [review]
v02-1276127-part01-add-sse-directory-to-intree-libpng

Review of attachment 8780163 [details] [diff] [review]:
-----------------------------------------------------------------

It would be fine to have joined both of these patches as one. Please use a single commit.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Folded and pushed to Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c663ca95bb7

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdd3e16aebd4
Add SSE support to in-tree libpng. r=jrmuizel

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cdd3e16aebd4

Comment 38

[Glenn Randers-Pehrson]

This seems to have had no discernible effect on arewefastyet.com in mid-August (I don't know if AWFY tests png decoding or not, but it doesn't look that way).

Comment 39

[Ryan VanderMeulen [:RyanVM]]

arewefastyet only measures JS performance