Repository navigation
Switch to xxhash, drop vendored siphash, modernize CI - #8
Merged
Merged
Conversation
Curates the useful parts of PR #7 (closed) without its module rename to a different fork: - Replace github.com/dchest/siphash with github.com/cespare/xxhash/v2 and drop the vendored siphash copy. Unlike PR #7, offset and skip are still derived from two independently salted hashes rather than reusing one hash for both, preserving the permutation's hash independence. - Remove the unused .travis.yml (GitHub Actions already covers CI) and its dead badge in the README. - Bump go.mod to go 1.21 and update the CI workflow to actions/checkout@v4, actions/setup-go@v5, and run on all PRs rather than just ones targeting master. - Lowercase error strings per Go convention and add TestRequiresPrime for the non-prime lookup table size check.
There was a problem hiding this comment.
Pull request overview
This PR updates the Maglev hashing implementation to use github.com/cespare/xxhash/v2 instead of github.com/dchest/siphash, removes the previously vendored SipHash implementation, and modernizes CI/tooling around Go 1.21.
Changes:
- Replace SipHash with xxhash (including salted hashing for
offset/skip) and remove vendored SipHash code. - Modernize CI by removing Travis config and updating GitHub Actions to newer action versions / Go 1.21, and run CI for all PRs.
- Normalize error strings to lower-case and add a new test covering non-prime lookup table rejection.
Reviewed changes
Copilot reviewed 6 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vendor/modules.txt | Removes the vendored siphash module entry. |
| vendor/github.com/dchest/siphash/siphash.go | Deletes vendored SipHash implementation. |
| vendor/github.com/dchest/siphash/README.md | Deletes vendored SipHash README. |
| vendor/github.com/dchest/siphash/hash128.go | Deletes vendored SipHash 128-bit implementation. |
| vendor/github.com/dchest/siphash/hash128_amd64.s | Deletes vendored SipHash amd64 128-bit asm. |
| vendor/github.com/dchest/siphash/hash.go | Deletes vendored SipHash 64-bit implementation. |
| vendor/github.com/dchest/siphash/hash_asm.go | Deletes vendored SipHash asm declarations. |
| vendor/github.com/dchest/siphash/hash_arm.go | Deletes vendored SipHash ARM implementation shim. |
| vendor/github.com/dchest/siphash/hash_amd64.s | Deletes vendored SipHash amd64 asm. |
| vendor/github.com/dchest/siphash/blocks.go | Deletes vendored SipHash block/finalize code. |
| vendor/github.com/dchest/siphash/blocks_asm.go | Deletes vendored SipHash asm block declarations. |
| vendor/github.com/dchest/siphash/blocks_arm.s | Deletes vendored SipHash ARM asm blocks. |
| vendor/github.com/dchest/siphash/blocks_amd64.s | Deletes vendored SipHash amd64 asm blocks/finalize. |
| vendor/github.com/dchest/siphash/.travis.yml | Removes vendored Travis config. |
| README.md | Removes the Travis badge from the README. |
| maglev.go | Switches hashing to xxhash, introduces salted hashing for permutation parameters, and updates error messages/comments. |
| maglev_test.go | Adds TestRequiresPrime to validate lookup-table prime requirement. |
| go.sum | Replaces siphash sums with xxhash sums. |
| go.mod | Bumps Go version to 1.21 and switches dependency to xxhash/v2. |
| .travis.yml | Removes repo-level Travis CI config. |
| .github/workflows/go.yml | Updates actions versions and Go version; adjusts PR trigger behavior. |
Suppressed comments (2)
.github/workflows/go.yml:18
- The
Set up Gostep is not indented understeps:. Each step entry must be part of thestepssequence, otherwise the workflow YAML is invalid.
- name: Set up Go
uses: actions/setup-go@v5
with:
go-version: '1.21'
.github/workflows/go.yml:24
- The
Build/Teststeps are not indented understeps:. This prevents the job from having any executable steps and breaks CI.
- name: Build
run: go build -v ./...
- name: Test
run: go test -v ./...
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
12
to
+13
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - uses: actions/checkout@v4 |
Comment on lines
+123
to
128
| func saltedHash(salt byte, data []byte) uint64 { | ||
| d := xxhash.New() | ||
| d.Write([]byte{salt}) | ||
| d.Write(data) | ||
| return d.Sum64() | ||
| } |
kkdai
added a commit
that referenced
this pull request
Aug 26, 2026
Builds on the xxhash switch from #8; the key-to-backend mapping is unchanged. Verified by dumping the mapping for 2000 keys over 10 backends and a 65537-slot table before and after: byte-for-byte identical. Clarity: - rename the receiver to mg so mg.m (table size) no longer reads as m.m - drop the redundant n field; it mirrored len(nodeList) and Clear() forgot to reset it, which could make a later Add fail spuriously - rename generatePopulation/populate to buildPermutations/buildLookup and drive both from a single rebuild(), with comments explaining why sorting and a prime table size matter - build permutation rows by stepping instead of offset+j*skip, so the arithmetic stays bounded by m and cannot overflow on large tables - lookup becomes []int since it indexes nodeList; name the hash salts; turn the unused bigM const into the documented, exported BigM - hash the key with xxhash.Sum64String so Get needs no []byte conversion - add Backends() and a package doc comment Errors: - replace the ad-hoc error strings with wrapped sentinels (ErrTableSizeNotPrime, ErrTooManyBackends, ErrBackendExists, ErrBackendNotFound, ErrNoBackends) so callers can use errors.Is Tests: - drop the commented-out dead block, add helpers, use t.Fatal where continuing would nil-deref, and replace log tracing with real assertions - cover every sentinel error, Clear followed by refill, table invariants, and minimal disruption (27 of 10000 survivor keys move when one of ten backends is removed) - TestNonPrimeTableSize subsumes TestRequiresPrime, asserting the specific sentinel rather than any error - add BenchmarkGet: 11.8 ns/op, 0 allocs/op Tooling: - require Go 1.24 for the slices package, range-over-int and b.Loop() - run vet, -race and benchmarks in CI
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
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.
Summary
Cherry-picks the reasonable parts of #7 (closed) without also renaming the module to a different fork:
github.com/dchest/siphashwithgithub.com/cespare/xxhash/v2and remove the vendored siphash copy. Improves on Backporting binary search and condition on loop edge #7's approach: that PR derived bothoffsetandskipfrom the same unsaltedxxhash.Sum64(bData)call (just reduced with different moduli), which correlates the two values. This PR salts each hash independently sooffsetandskipstay as independent as they were under the original two-key SipHash scheme..travis.yml(GitHub Actions has covered CI for a while) and its now-dead badge in the README.go.modtogo 1.21and update the workflow toactions/checkout@v4/actions/setup-go@v5, and run CI on all PRs instead of only ones targetingmaster.errors.Newmessages shouldn't start capitalized).TestRequiresPrime, covering the non-prime lookup table size rejection.Left out from #7: the module path / README rename to
github.com/hardpointlabs/maglev— this repo should keep its own identity.Test plan
go build ./...go vet ./...go test -v ./...(all existing tests plus the newTestRequiresPrimepass)gofmt -l .reports no files