refactor: merge 4 C types into single XXHASHObject type#155
Open
ifduyue wants to merge 10 commits into
Open
Conversation
Following CPython _hashlib's pattern (one HASH type for all algorithms), merge the four per-algorithm C types (PYXXH32Object, PYXXH64Object, PYXXH3_64Object, PYXXH3_128Object) into a single XXHASHObject type. Key changes: - Single struct with XXH_Algo enum + union of state pointers - Single set of tp_* methods with algo-based dispatch (switch) - Single methods/getseters/slots/spec (was 4 copies of each) - 4 module-level constructor functions (xxh32, xxh64, xxh3_64, xxh3_128) sharing a single _xxhash_new helper - One-shot functions consolidated via DEFINE_ONESHOT macro - PyCFunction constructors replace 4 separate type objects - Updated .pyi stubs: constructors are functions, not @Final classes Results: - C source reduced by 37% (2271 -> 1421 lines, -845 lines) - Zero warnings on all supported Python versions (3.9-3.15, 3.14t) - All 124 tests pass across all versions
![gemini-code-assist[bot]](https://avatars.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the type stubs in xxhash/__init__.pyi to correctly define the xxh* constructors as functions returning a _Hasher instance rather than distinct classes, and updates the subinterpreter isolation tests accordingly. A review comment correctly identifies an incorrect alias assignment where xxh64 is assigned to xxh3_64 despite already being defined as a distinct function.
Comment on lines
+73
to
+74
| # Aliases | ||
| xxh64 = xxh3_64 # type: ignore[assignment] |
There was a problem hiding this comment.
The assignment xxh64 = xxh3_64 is incorrect. xxh64 is a distinct algorithm and has already been correctly defined as a function on line 69. It is not an alias of xxh3_64 (unlike xxh128 which is an alias of xxh3_128). This assignment should be removed to prevent type-checking errors and confusion.
Suggested change
| # Aliases | |
| xxh64 = xxh3_64 # type: ignore[assignment] | |
| # Aliases |
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
Replace the generic _xxhash_oneshot() dispatcher (switch(algo) x switch(return_type)) with fully specialized per-algorithm/per-return-type functions generated by DEFINE_ONESHOT_INT and DEFINE_ONESHOT_DH macros. Each function directly calls the right xxhash C function and formats the result, avoiding the double-dispatch overhead. Also extract _xxhash_hexdigest_from_bytes() and _xxhash_xxh128_intdigest_result() helpers, and remove the unused XXH_OneshotDispatch struct. Benchmark result (xxh32_hexdigest, 1KB data): Before: 421.2 ns/call After: 353.3 ns/call (~16% faster)
…l methods Define XXH_AlgoDispatch (create_state, free_state, reset, update, digest, intdigest, copy_state) and a const XXH_DISPATCH[] array indexed by XXH_Algo, eliminating all 10 switch(self->algo) statements in the stateful path (_xxhash_init_state, _xxhash_free_state, _xxhash_reset_state, _xxhash_do_update, _xxhash_new, XXHASH_init, XXHASH_digest, XXHASH_hexdigest, XXHASH_intdigest, XXHASH_copy). Also remove the unused _free_module callback (PyType_FromModuleAndSpec ties heap type lifetime to the module automatically). Benchmarks show performance at parity or slightly improved (constructor+hexdigest ~6-20% faster).
The comment explains why the extra state copy is needed: xxHash v0.7.3 and earlier have a bug where states reset with a seed will have a wild pointer to the original state when copied, causing a use-after-free if the original is freed.
The update() method is now on a unified XXHASHObject type supporting all four algorithms (XXH32, XXH64, XXH3_64, XXH3_128), not just xxh32.
… repr - XXHASH_new (tp_new) now raises TypeError instead of creating instances, matching _hashlib.HASH behavior. Only factory functions can create objects. - Add XXHASH_repr (tp_repr) showing algorithm name: <XXH32 _xxhash.HASH object> matching CPython's <md5 _hashlib.HASH object> pattern. - Rename type from 'xxhash.xxhash' to '_xxhash.HASH' to align with hashlib.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Following CPython _hashlib's pattern (one HASH type for all algorithms), merge the four per-algorithm C types (PYXXH32Object, PYXXH64Object, PYXXH3_64Object, PYXXH3_128Object) into a single XXHASHObject type.
Key changes:
Results: