1562548 - Improve GCM perfomance on aarch32

Status: RESOLVED FIXED

(NSS :: Libraries, task, P2)

Description

[Makoto Kato [:m_kato]]

bug 1559012 is for aarch64, but we can improve GCM even if aarch32.

Comment 1

[J.C. Jones [:jcj] (he/they)]

Makoto - Can I volunteer you for this one? :) (Feel free to un-assign if it's overload).

Comment 2

[Chris Peterson [:cpeterson]]

esr68=affected because we might want to uplift this optimization to Fennec ESR 68.1.

Comment 3

[Makoto Kato [:m_kato]]

Test on m-c's try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab9576bd0e59185190a7b8d7fd5cb3c2f1989f9f

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Bug 1562548 - Improve GCM perfomance on aarch32 using NEON.

Optimize GCM perfomance using https://conradoplg.cryptoland.net/files/2010/12/gcm14.pdf via ARM's NEON.

Comment 5

[Andrew Creskey [:acreskey]]

I'm curious about the attached patch - can I give this a try and see if it helps? Is it waiting on anything?
I've re-profiled speedof.me with webrender and the GCM_DecryptUpdate is very prominent:
https://perfht.ml/2pBJsVr

Comment 6

[Makoto Kato [:m_kato]]

I need to rebase old patch for review

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Andrew Creskey from comment #5)

I'm curious about the attached patch - can I give this a try and see if it helps? Is it waiting on anything?
I've re-profiled speedof.me with webrender and the GCM_DecryptUpdate is very prominent:
https://perfht.ml/2pBJsVr

Well, this fix will improve a lot of performance of gcmHash_Update that is 22% of this profiling data. About rijndael_encryptBlock128, we have already landed improvement for ARMv8 CPU (MotoG5, Pixel 2 and etc) although it isn't merged into Fenix Nightly.

Comment 8

[Andrew Creskey [:acreskey]]

Thank you Makoto.
I did apply this patch to an arm32 PGO build of geckoview_example.
Unfortunately I couldn't pick up any performance improvements but all I have to test with are the speedof.me tests.
I think Ideally we would more isolated GCM performance tests.

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Andrew Creskey from comment #8)

Thank you Makoto.
I did apply this patch to an arm32 PGO build of geckoview_example.
Unfortunately I couldn't pick up any performance improvements but all I have to test with are the speedof.me tests.
I think Ideally we would more isolated GCM performance tests.

This fix is that GCM improves 1.5-2.0 times faster than original even if non-ARMv8 hardware. Since we don't turn on WebRender for 32-bit device, it spends a lot of times on filter processing (bug 961759).

#     mode          in symmkey  opreps  cxreps     context          op   time(sec)     thrgput
 aes_gcm_e        69Mb     256      1M       0       0.000   10000.000      10.000         6Mb <-- with this fix
 aes_gcm_e        37Mb     256    654T       0       0.000   10000.000      10.000         3Mb

Comment 10

[Andrew Creskey [:acreskey]]

Thank you for collecting those throughput results, m_kato, they look great.
I did also turn on WebRender for that test, but, like I said, it's not a well defined work flow and there's a lot of noise.
I have a bit of a backlog, but let me needinfo myself to run this change on a real set of pages.
Our current automated pageload tests don't necessarily use the actual cipher suite that a client/server would agree on.

Comment 11

[Andrew Creskey [:acreskey]]

Makato, I'm curious -- how are you building your measured throughput tests?
Is this in-tree in gecko?
In Bug 1591725 we're looking at a more aggressive build optimization than the current -Oz. It occurred to me that if your tests are built in a standalone app with a different build configuration then we would get different results. AFAIK -Oz may disable some vectorization.

Comment 12

[Makoto Kato [:m_kato]]

(In reply to Andrew Creskey from comment #11)

Makato, I'm curious -- how are you building your measured throughput tests?
Is this in-tree in gecko?
In Bug 1591725 we're looking at a more aggressive build optimization than the current -Oz. It occurred to me that if your tests are built in a standalone app with a different build configuration then we would get different results. AFAIK -Oz may disable some vectorization.

This result is by bltest in NSS tree (https://searchfox.org/mozilla-central/source/security/nss/cmd/bltest). And this algorithm is for armv7 using the paper (comment #4) and this doesn't depends on compiler's automatic vectorization.

Comment 13

[Makoto Kato [:m_kato]]

Since I don't have commit permission for nss, could you land this? (I guess that sheriffs don't have permission for nss).

Comment 14

[J.C. Jones [:jcj] (he/they)]

Will take in NSS 3.49. Leaving my needinfo until after I branch.

Comment 15

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/a9ba652046e634ccb4ca21a43dd6d76858e20d62

Comment 16

[tt_1]

I do get this build error in new nss-3.49 release on arm, and after a bit of tinkering I found the issue to be the patch discussed in this thread. Here's the error:

/usr/lib/gcc/armv7a-unknown-linux-gnueabihf/9.2.0/../../../../armv7a-unknown-linux-gnueabihf/bin/ld: warning: wildcard match appears in both version 'NSSprivate_3.11' and 'NSSprivate_3.16' in script
/usr/lib/gcc/armv7a-unknown-linux-gnueabihf/9.2.0/../../../../armv7a-unknown-linux-gnueabihf/bin/ld: error: Linux2.6_arm_armv7a-unknown-linux-gnueabihf-gcc_glibc_PTH_OPT.OBJ/Linux_SINGLE_SHLIB/gcm-arm32-neon.o: multiple definition of 'gcm_HashMult_hw'
/usr/lib/gcc/armv7a-unknown-linux-gnueabihf/9.2.0/../../../../armv7a-unknown-linux-gnueabihf/bin/ld: Linux2.6_arm_armv7a-unknown-linux-gnueabihf-gcc_glibc_PTH_OPT.OBJ/Linux_SINGLE_SHLIB/gcm.o: previous definition here
/usr/lib/gcc/armv7a-unknown-linux-gnueabihf/9.2.0/../../../../armv7a-unknown-linux-gnueabihf/bin/ld: error: Linux2.6_arm_armv7a-unknown-linux-gnueabihf-gcc_glibc_PTH_OPT.OBJ/Linux_SINGLE_SHLIB/gcm-arm32-neon.o: multiple definition of 'gcm_HashWrite_hw'
/usr/lib/gcc/armv7a-unknown-linux-gnueabihf/9.2.0/../../../../armv7a-unknown-linux-gnueabihf/bin/ld: Linux2.6_arm_armv7a-unknown-linux-gnueabihf-gcc_glibc_PTH_OPT.OBJ/Linux_SINGLE_SHLIB/gcm.o: previous definition here
/usr/lib/gcc/armv7a-unknown-linux-gnueabihf/9.2.0/../../../../armv7a-unknown-linux-gnueabihf/bin/ld: error: Linux2.6_arm_armv7a-unknown-linux-gnueabihf-gcc_glibc_PTH_OPT.OBJ/Linux_SINGLE_SHLIB/gcm-arm32-neon.o: multiple definition of 'gcm_HashInit_hw'
/usr/lib/gcc/armv7a-unknown-linux-gnueabihf/9.2.0/../../../../armv7a-unknown-linux-gnueabihf/bin/ld: Linux2.6_arm_armv7a-unknown-linux-gnueabihf-gcc_glibc_PTH_OPT.OBJ/Linux_SINGLE_SHLIB/gcm.o: previous definition here
/usr/lib/gcc/armv7a-unknown-linux-gnueabihf/9.2.0/../../../../armv7a-unknown-linux-gnueabihf/bin/ld: error: Linux2.6_arm_armv7a-unknown-linux-gnueabihf-gcc_glibc_PTH_OPT.OBJ/Linux_SINGLE_SHLIB/gcm-arm32-neon.o: multiple definition of 'gcm_HashZeroX_hw'
/usr/lib/gcc/armv7a-unknown-linux-gnueabihf/9.2.0/../../../../armv7a-unknown-linux-gnueabihf/bin/ld: Linux2.6_arm_armv7a-unknown-linux-gnueabihf-gcc_glibc_PTH_OPT.OBJ/Linux_SINGLE_SHLIB/gcm.o: previous definition here
collect2: error: ld returned 1 exit status

I'm going to attach the full build log in a moment.
Should I open a new bug for this?

Comment 17

[tt_1]

Attachment: nss-3.49-neon-fails.log.gz

Comment 18

[J.C. Jones [:jcj] (he/they)]

Thanks for opening 1608327, Mike.