Possible Performance Degradation of HQC128 through Update to 2023-04-30 Submission #2047
Comments
|
Thanks for the report! Good to know somebody has an interest in HQC. It looks like you are building liboqs with |
|
Thank you for your quick and nice response :) When adding the build variable Is the statement correct that this performance degradation should not be there? Because in the publication of the hqc team accompanying the submission is no indication of a performance decrease, rather the opposite. |
|
Sorry, I should have been more clear: I think the relevant test to rerun is 0.9.2, with OQS_DIST_BUILD=OFF and OQS_OPT_TARGET=generic. This will test the 0.9.2 generic code. That way we can see whether there was a performance regression in the generic code. |
|
Like requested i built liboqs |
|
Thinking about it, there was one major change between the 2021-06-06 version and 2023-04-30 version that would affect performance: HQC switched from AES to SHA3 for seed expansion. Based on the logs, it looks like your builds are using AES hardware acceleration. This could explain some of the difference. If you add the flag |
|
Building with |
Of course! That was meant to illustrate how much performance depends on the underlying AES or SHA3 implementation. We do have an open issue to integrate the HQC AVX2-optimized implementation: #1596. However, I'm not inclined to work on it until the upstream source publishes a fix for the significant correctness/security issue currently present in the reference implementation.
Sure—but be advised that the HQC spec has been updated a number of times since that release, including the change to SHA3 from AES, so your results won't be current. |
|
@SWilson4 Looking at the release history for 0.10.0, I assume responsibility for this: I do not recall having executed the section in the In the light of this issue, what should be improved to avoid this problem from re-occurring? Is the script OK? Worthwhile re-considering the "de-emphasizing" of the profiling sub project ? Has the "noregress" script been run for 0.11.0 and 0.12.0 releases? Are there similar problems with algorithms beyond HQC? Worthwhile creating a separate issue to investigate? With this many questions, I've got to take a step back and look at it from a more general level: OQS once wanted to indiscriminately report pros and cons of all PQC algorithms -- and that included performance. Could you agree that this seems like another area where project utility got reduced? Might it be worth while to discuss this at OQS TSC and/or PQCA TAC level? I have the nagging feeling that OQS / PQCA pursue too many, somewhat contradicting goals (and/or too few people contributing for the level of goals set), leading to fewer being done at an excellent level. As now a full one year passed since LinuxFoundation/PQCA took control of OQS, time to take stock/review/realign? |
I don't recall if we did execute the performance script for the 0.10.0 release, but if we did I don't think a drop in HQC performance was significant cause for concern:
I believe the noregress script was run for the latest release. Don't remember for 0.11.0.
I think it makes sense to bring up the profiling project at the TSC and/or PQCA level, perhaps to see if we could get an external contributor (like @geedo0 for OpenSSH or @ajbozarth for demos) to revive/redo it. |
In the light of #2054 allow me to ask this question again: Is this an HQC-only problem or one affecting more algs? Is there something really wrong with common algorithms, the configs and/or copy_from_upstream? |
|
I'm currently working to pinpoint the issue causing the slowdown and see if anything can be done about it. |
|
I have run some comparisons across different versions of liboqs and different specifications of HQC. Here are the numbers: Resultsliboqs main (patched HQC 2023-04-30)This is the current liboqs main branch, which includes a patched version (via PQClean) of the HQC 2023-04-30 implementation. liboqs 0.9.2 (patched HQC 2020-10-01)This is liboqs 0.9.2, which includes a patched version (via PQClean) of the HQC 2020-10-01 implementation. There is a noticeable increase in performance compared to the current main. liboqs main with unpatched HQC 2023-04-30This is liboqs main, replacing the HQC implementation with the 2023-04-30 reference code (with no patching). There is effectively no performance difference from the plain main. This leads me to believe that the PQClean patches have little to no impact on performance. liboqs 0.9.2 with unpatched HQC 2023-04-30This is liboqs 0.9.2, replacing the HQC implementation with the 2023-04-30 reference code (with no patching). There is effectively no performance difference from liboqs main with the unpatched 2023-04-30 reference code; however, there is a significant performance drop when compared to plain liboqs 0.9.2. This leads me to believe that changes in liboqs glue code after 0.9.2 have little to no impact on performance. ConclusionThese comparisons lead me to believe that the drop in performance in HQC since 0.9.2 is entirely due to changes in the HQC specification and reference code. I see no evidence that performance is impacted by either liboqs glue code or patches in PQClean. We would likely see performance improvements if we were to import the "optimized" implementation. I'm not inclined to work on this myself until the correctness and security error in decapsulation is patched upstream. Past imports of HQC's optimized implementation have also required extensive patching to eliminate possible memory errors. However, if somebody has a strong interest in HQC and would like to take a stab at pulling in the optimized code, with the necessary security patches, I would be very happy to support the effort and review any necessary PRs. Scripts and commandsIn case anybody wants to duplicate the results, this is the contents of the #!/bin/sh
set -e
if [ -z "$VERSION" ]
then
exit 1
fi
hqc128dir="src/kem/hqc/pqclean_hqc-128_clean"
namespace="PQCLEAN_HQC128_CLEAN_"
# check to see if we're on 0.9.2
if [ "62b58a34fbbcc1cb23f2c090c8a19b090ebf1aa2" = "$(git rev-parse HEAD)" ]
then
# use old naming conventions
hqc128dir="src/kem/hqc/pqclean_hqc-rmrs-128_clean"
namespace="PQCLEAN_HQCRMRS128_CLEAN_"
# use updated sizes
git restore -s main -- src/kem/hqc/kem_hqc.h
# build all source files
sed -i "s/pqclean_hqc-rmrs-128_clean\/vector.c/pqclean_hqc-rmrs-128_clean\/shake_ds.c pqclean_hqc-rmrs-128_clean\/shake_prng.c pqclean_hqc-rmrs-128_clean\/vector.c/g" src/kem/hqc/CMakeLists.txt
fi
# get the official HQC code
if [ ! -d hqc ]
then
mkdir hqc
wget https://pqc-hqc.org/doc/hqc-submission_${VERSION}.zip
unzip hqc-submission_${VERSION}.zip -d hqc
fi
# copy over the reference implementation source files
for file in hqc/Reference_Implementation/hqc-128/src/*.cpp
do
# skip the harness file
if [ main_hqc.cpp = $(basename $file) ]
then
continue
fi
# rename .cpp to .c
cp $file $hqc128dir/$(basename -s .cpp $file).c
done
for file in hqc/Reference_Implementation/hqc-128/src/*.h
do
cp $file $hqc128dir/$(basename $file)
done
# replace the gf2x.* files with the versions in the "additional implementation"
# the reference versions depend on the GF2X C++ library
cp hqc/Additional_Implementation/hqc-128/src/gf2x.* $hqc128dir
# rename the main API functions to match existing OQS boilerplate code
for suffix in keypair enc dec
do
sed -i "s/crypto_kem_$suffix/${namespace}crypto_kem_$suffix/g" $hqc128dir/api.h $hqc128dir/kem.c
done
# swap out the KAT PRNG for a CSPRNG
sed -i 's/shake_prng/randombytes/g' $hqc128dir/hqc.c $hqc128dir/vector.cAnd this is the sequence of commands I ran: git clone https://github.com/open-quantum-safe/liboqs
# current main
cd liboqs && mkdir build && cd build
cmake -GNinja -DOQS_MINIMAL_BUILD="KEM_hqc_128" -DOQS_DIST_BUILD=OFF -DOQS_OPT_TARGET=generic ..
ninja
cd -
./build/tests/speed_kem -d 10 HQC-128
# switch to 0.9.2
git checkout 0.9.2
rm -r build && mkdir build && cd build
cmake -GNinja -DOQS_MINIMAL_BUILD="KEM_hqc_128" -DOQS_DIST_BUILD=OFF -DOQS_OPT_TARGET=generic .. && ninja && cd -
./build/tests/speed_kem -d 10 HQC-128
# back to main, pull in unpatched HQC
git checkout main
VERSION=2023-04-30 ../import_hqc.sh
rm -r build && mkdir build && cd build
cmake -GNinja -DOQS_MINIMAL_BUILD="KEM_hqc_128" -DOQS_DIST_BUILD=OFF -DOQS_OPT_TARGET=generic .. && ninja && cd -
./build/tests/speed_kem -d 10 HQC-128
# switch to 0.9.2, pull in unpatched HQC
git reset --hard main
git checkout 0.9.2
VERSION=2023-04-30 ../import_hqc.sh
rm -r build && mkdir build && cd build
cmake -GNinja -DOQS_MINIMAL_BUILD="KEM_hqc_128" -DOQS_DIST_BUILD=OFF -DOQS_OPT_TARGET=generic .. && ninja && cd -
./build/tests/speed_kem -d 10 HQC-128I ran these comparisons on an x86_64 AWS EC2 instance. |
|
In light of the above, closing this issue. It seems like resolving #1596 is the way to go if we want to improve HQC's performance. |
Describe the bug
When running
./speed_kem HQC-128from the build/tests directory there is a big performance degradation, introduced by #1585 by @SWilson4.I wanna mention, that it just may be the effect of not having an avx2 implementation anymore, as mentioned here PQClean/PQClean#512. If this is the reason, then consider my bug report solved.
To Reproduce
Steps to reproduce the behavior:
tags/0.10.0./speed_kem HQC-128tags/0.9.2./speed_kem HQC-128Expected behavior
No performance degradation this big.
Logs
0.10.0
0.9.2
Environment (please complete the following information):
Additional context
I used git bisect to find the exact commit introducing this behaviour.
The text was updated successfully, but these errors were encountered: