-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-99108: Add HACL* Blake2 implementation to hashlib #119316
Conversation
@gpshead if you can trigger a round of CI that would be most helpful. Thanks! |
So I computed some benchmarks on a variety of machines, comparing HACL's performance with libb2. This is using the benchmarking infrastructure from HACL-packages: https://github.com/cryspen/hacl-packages/blob/main/benchmarks/blake.cc To read these benchmarks, bear in mind that:
M3 Max (ARM)
interpretation:
Intel AVX2 (8-Core Intel Core i9, Macbook Pro, 2019)
interpretation:
AVX2 desktop machine, Haswell
interpretation:
This leaves us with several options. I would love to have your opinion @gpshead
Please share thoughts. Thanks! CC @R1kM who provided considerable help with libb2 benchmarking |
I'd personally go for "2. CPython loses the ability to build against libb2, and packages HACL's portable versions and vectorized". But even if we only get to option 1. without the cpu detection and just using the portable version, it still seems reasonable. The differences are not huge. And moving from there to option 2 would still be a later followup possibility. Both of those reduce our dependencies. |
Happy to do option 2, however, could you help me out a little bit with the build? I've pushed the new files to my branch, but I need to encode in the build system that:
I don't know if Setup.stdlib.in supports this, and if so, how to encode it. Any pointers appreciated. Thanks! |
cpython/Modules/Setup.stdlib.in Lines 11 to 13 in 769aea3
This is a generalization of a simpler fact: Setup.stdlib.in is processed as an AC_CONFIG_FILES, which means you can do any Lines 156 to 165 in 769aea3
indicates you need to update a match for Lines 178 to 182 in 769aea3
so e.g. $(AVX_CFLAGS). There doesn't currently seem to be a way to build a module where some of its files are built with certain CFLAGS and some are built with other CFLAGS, which indicates you need to pass -mavx* to all files in the blake module, which may be a problem In commit d777790, @gpshead updated the sha* code from HACL* to build as a standalone static library instead, which then gets linked to in Setup.stdlib.in by specifying a filepath to a I think that last approach is what you want to do as that gives you the freedom to define HACL* to build however you need it to, and then Setup.stdlib.in just needs to link to HACL* rather than define the fiddly bits of how to compile it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At
Lines 216 to 219 in 769aea3
# Internal static libraries | |
LIBMPDEC_A= Modules/_decimal/libmpdec/libmpdec.a | |
LIBEXPAT_A= Modules/expat/libexpat.a | |
LIBHACL_SHA2_A= Modules/_hacl/libHacl_Hash_SHA2.a |
Add:
LIBHACL_BLAKE2_A= Modules/_hacl/libHacl_Hash_BLAKE2.a
After:
Lines 638 to 657 in 769aea3
########################################################################## | |
# hashlib's HACL* library | |
LIBHACL_SHA2_OBJS= \ | |
Modules/_hacl/Hacl_Hash_SHA2.o | |
LIBHACL_HEADERS= \ | |
Modules/_hacl/include/krml/FStar_UInt128_Verified.h \ | |
Modules/_hacl/include/krml/FStar_UInt_8_16_32_64.h \ | |
Modules/_hacl/include/krml/fstar_uint128_struct_endianness.h \ | |
Modules/_hacl/include/krml/internal/target.h \ | |
Modules/_hacl/include/krml/lowstar_endianness.h \ | |
Modules/_hacl/include/krml/types.h \ | |
Modules/_hacl/Hacl_Streaming_Types.h \ | |
Modules/_hacl/python_hacl_namespaces.h | |
LIBHACL_SHA2_HEADERS= \ | |
Modules/_hacl/Hacl_Hash_SHA2.h \ | |
Modules/_hacl/internal/Hacl_Hash_SHA2.h \ | |
$(LIBHACL_HEADERS) |
Add:
LIBHACL_BLAKE2_AVX_OBJ = Modules/_hacl/Hacl_Hash_Blake2s_Simd128.o
LIBHACL_BLAKE2_AVX2_OBJ = Modules/_hacl/Hacl_Hash_Blake2s_Simd256.o
LIBHACL_BLAKE2_OBJS= \
Modules/_hacl/Hacl_Hash_Blake2s.o \
Modules/_hacl/Hacl_Hash_Blake2b.o \
@LIBHACL_BLAKE2_AVX_OBJ@ \
@LIBHACL_BLAKE2_AVX2_OBJ@ \
Modules/_hacl/Lib_Memzero0.o
(and teach configure.ac to replace these @XXX@
with $(XXX)
if and only if they are supported by the compiler)
at:
Lines 1332 to 1340 in 769aea3
# Build HACL* static libraries for hashlib: libHacl_Hash_SHA2.a | |
LIBHACL_CFLAGS=-I$(srcdir)/Modules/_hacl/include -D_BSD_SOURCE -D_DEFAULT_SOURCE $(PY_STDMODULE_CFLAGS) $(CCSHARED) | |
Modules/_hacl/Hacl_Hash_SHA2.o: $(srcdir)/Modules/_hacl/Hacl_Hash_SHA2.c $(LIBHACL_SHA2_HEADERS) | |
$(CC) -c $(LIBHACL_CFLAGS) -o $@ $(srcdir)/Modules/_hacl/Hacl_Hash_SHA2.c | |
$(LIBHACL_SHA2_A): $(LIBHACL_SHA2_OBJS) | |
-rm -f $@ | |
$(AR) $(ARFLAGS) $@ $(LIBHACL_SHA2_OBJS) |
Add:
LIBHACL_BLAKE2_CFLAGS=-I$(srcdir)/Modules/_hacl/ -I$(srcdir)/Modules/_hacl/include -D_BSD_SOURCE -D_DEFAULT_SOURCE
Modules/_hacl/Hacl_Hash_Blake2s.o: $(srcdir)/Modules/_hacl/Hacl_Hash_Blake2s.c $(LIBHACL_BLAKE2_HEADERS)
$(CC) -c $(LIBHACL_CFLAGS) -o $@ $(srcdir)/Modules/_hacl/Hacl_Hash_Blake2s.c
Modules/_hacl/Hacl_Hash_Blake2b.o: $(srcdir)/Modules/_hacl/Hacl_Hash_Blake2b.c $(LIBHACL_BLAKE2_HEADERS)
$(CC) -c $(LIBHACL_CFLAGS) -o $@ $(srcdir)/Modules/_hacl/Hacl_Hash_Blake2b.c
Modules/_hacl/Hacl_Hash_Blake2s_Simd128.o: $(srcdir)/Modules/_hacl/Hacl_Hash_Blake2s_Simd128.c $(LIBHACL_BLAKE2_HEADERS)
$(CC) -c $(LIBHACL_CFLAGS) -mavx -o $@ $(srcdir)/Modules/_hacl/Hacl_Hash_Blake2s_Simd128.c
Modules/_hacl/Hacl_Hash_Blake2s_Simd256.o: $(srcdir)/Modules/_hacl/Hacl_Hash_Blake2s_Simd256.c $(LIBHACL_BLAKE2_HEADERS)
$(CC) -c $(LIBHACL_CFLAGS) -mavx2 -o $@ $(srcdir)/Modules/_hacl/Hacl_Hash_Blake2s_Simd256.c
Modules/_hacl/Lib_Memzero0.o: $(srcdir)/Modules/_hacl/Lib_Memzero0.c $(LIBHACL_BLAKE2_HEADERS)
$(CC) -c $(LIBHACL_CFLAGS) -o $@ $(srcdir)/Modules/_hacl/Lib_Memzero0.c
$(LIBHACL_BLAKE2_A): $(LIBHACL_BLAKE2_OBJS)
-rm -f $@
$(AR) $(ARFLAGS) $@ $(LIBHACL_BLAKE2_OBJS)
Thanks @eli-schwartz that's super helpful. In another issue, someone mentioned that the .a file was causing complications #116043 so I have a pending PR #119320 to remove that. But I have a hunch you might be able to shed some light on the matter :-). Thanks! |
I've never used freeze.py myself. I would guess that it's collecting compiled objects used to produce python and its extensions and thinks it needs the *.a files for that purpose, even though it's used to build extensions, and is simply a matter of it not being adapted as the Makefile evolves. ... You might think it would be simple to just add We really want to use the compile rule from Makefile.pre.in since that allows specifying -mavx per-file... |
silly question, why expand every single object file into its own recipe instead of relying on a pattern rule? |
…ASHLIB_HASHES appears twice as a #define in confdefs.h -- try to fix this
…erly by distinguishing simd128 from simd256
The traditional reason for this is the necessity of supporting Make implementations that don't support the GNU pattern rule extension. Opinions vary about whether to support other Make implementations but the fact of the matter is that you can't even use modern 2024 editions of BSD make in bleeding edge BSD platforms and have those work; you're forced to install the GNU make port and execute Making such a change requires a bit more discussion than fits this PR, I bet. |
No worries, that's what I suspected but wanted to double-check. Thanks for your prompt response. I'm putting the final touches for this, and then you'll have to double-check I haven't used any GNUisms -- all of the Makefiles I've written in recent years assume GNU make > 3.81. |
Thanks. FreeBSD is good now, and MacOS failures seem unrelated. Do you see anything else suspicious / worth investigating? (I don't.) |
I don't see any other issues. It won't totally surprise me if some more compiler flag tweaks pop up as necessary in the future on this under various people's configs given those freebsd bots (which should just be using a modern clang or gcc?) didn't like |
Thanks everyone! I'll follow up with the validation / cosmetic changes soon. |
|
|
|
Oh and yes please keep me posted on RHEL7 and whether it matters or not. If yes, I'll do some more autoconf wizardry, though I can't say this is something I look forward to :-D |
This build failure also happened on Android – #121595 (comment). It looks like it would be sufficient to define |
@msprotz Could you ping me when the PR is ready please? (or just bluntly ask for a review from me?) Thank you in advance. |
RHEL7 doesn't matter, but if you don't do the icky wizardry patch, you're effectively leaving it to the maintainer of Android (or if we didn't have Android: some niche BSD that'll struggle to update to 3.14 in a few years). |
This is partly my fault for not setting up the Android buildbot soon enough, so I'll submit a fix today – #99108 (comment). |
👋 it looks like the line looks like this:
I assume they should look similar to the hashing modules below them? |
What is Modules/Setup used for? The build of blake2 is more complicated because some files can only be compiled if the toolchain supported them (see this PR, and notably the changes in configure.ac and Makefile.pre.in). The logic should be pretty self-explanatory so hopefully you'll know how to fix this? Otherwise, happy to explain in more detail what's happening for the build of the new blake2 module. Thanks! |
@msprotz the file Modules/Setup.stdlib.in was updated in this PR. It actually has a comment that says it isn't used by default "yet". :/ I think that Modules/Setup maybe just has to be updated exactly the same way as the former file? |
Strange! My local testing picked up my changes in Setup.stdlib.in so I'm not sure in what scenario Setup.stdlib.in is ignored in favor of Setup. But it's a small change so happy to submit a followup PR. See #123146 |
…119316) This replaces the existing hashlib Blake2 module with a single implementation that uses HACL\*'s Blake2b/Blake2s implementations. We added support for all the modes exposed by the Python API, including tree hashing, leaf nodes, and so on. We ported and merged all of these changes upstream in HACL\*, added test vectors based on Python's existing implementation, and exposed everything needed for hashlib. This was joint work done with @R1kM. See the PR for much discussion and benchmarking details. TL;DR: On many systems, 8-50% faster (!) than `libb2`, on some systems it appeared 10-20% slower than `libb2`.
|
This replaces the existing hashlib module with a single implementation that uses HACL*'s Blake2b/Blake2s implementations. We added support for all the modes exposed by the Python API, including tree hashing, leaf nodes, and so on. We ported and merged all of these changes upstream in HACL*, added test vectors based on Python's existing implementation, and exposed everything needed for hashlib.
This is a preliminary PR but I wanted to gather early feedback from @gpshead.
What remains to be done on this PR:
This is joint work with @R1kM