Skip to content

CommPat / CommPv2 hasher#30

Closed
magik6k wants to merge 3 commits intomasterfrom
commpv2
Closed

CommPat / CommPv2 hasher#30
magik6k wants to merge 3 commits intomasterfrom
commpv2

Conversation

@magik6k
Copy link
Copy Markdown

@magik6k magik6k commented Jan 7, 2026

Related to filecoin-project/FIPs#1216

It does appear to work correctly with easily >1GB/s throughput. Possibly a bit more verbose than it needs to be and possibly quite a few optimizations remain (e.g. better cache-line utilisation for left-right pairs, less allocations) but it's pretty fast.

Do not merge until PoDSIv2 FRC is in; feel free to review.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 7, 2026

Codecov Report

❌ Patch coverage is 87.54448% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.45%. Comparing base (fe57ba8) to head (d6952ff).

Files with missing lines Patch % Lines
commp2/commp2.go 87.54% 25 Missing and 10 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #30       +/-   ##
===========================================
+ Coverage   41.58%   63.45%   +21.87%     
===========================================
  Files           3        4        +1     
  Lines         392      561      +169     
===========================================
+ Hits          163      356      +193     
+ Misses        225      191       -34     
- Partials        4       14       +10     
Files with missing lines Coverage Δ
commp2/commp2.go 87.54% <87.54%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@magik6k
Copy link
Copy Markdown
Author

magik6k commented Jan 7, 2026

For reviewers: https://ipfs.io/ipfs/bafkreiacuhuhzbfvplw7d3lzei5vg4uyczoebpq3ihhffplij4he3opply is very nice to have opened on the side to look at tree layouts

Comment thread commp2/commp2.go

// nextPow2 returns the smallest power of 2 >= n
func nextPow2(n uint64) uint64 {
if n == 0 {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Micro-op:

func nextPow2(n uint64) uint64 {
	if n == 0 {
		return 1
	}
	return 1 << bits.Len(uint(n-1))
}

Comment thread commp2/commp2.go
}

// isZero checks if a byte slice is all zeros
func isZero(b []byte) bool {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func isZero is unused (U1000)

Comment thread commp2/commp2.go
// Shift bits to account for the 2-bit shim
// In: {{ C[7] C[6] }} X[7] X[6] X[5] X[4] X[3] X[2] X[1] X[0] ...
// Out: X[5] X[4] X[3] X[2] X[1] X[0] C[7] C[6] ...
for i := 31; i < 63; i++ {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance: (if this is hotpath)
Keep a carry var instead of 2 reads per loop. Also, process 8 bytes at a time:

    var carry uint64
    for ........ {
        v := binary.BigEndian.Uint64(b[i:])
        out := (v << 2) | carry
        binary.BigEndian.PutUint64(b[i:], out)
        carry = v >> 62
    }

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably worse in out-of-order CPUs because it creates dependencies. Good compiler probably can resolve both to exactly the same bit-shuffling instructions tho.

Comment thread commp2/commp2.go
Comment on lines +403 to +407
func payloadPosToLeaf(pos int) int {
switch {
case pos < 31:
return 0
case pos < 62:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func payloadPosToLeaf(pos int) int { return (pos+1) >> 5 }

Comment thread commp2/commp2.go
for quadIdx := startQuad; quadIdx < endQuad; quadIdx++ {
startLeafInQuad, endLeafInQuad := quadFromBuf(paddedQuad[:], buf, start, quadIdx)
if startLeafInQuad < 0 {
continue // no data in this quad
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no test touches this line. Is this an interesting case?

Comment thread commp2/commp2_test.go
for _, offset := range quadBoundaries {
t.Run(fmt.Sprintf("offset_%d", offset), func(t *testing.T) {
dataSize := int64(127 * 4) // 4 quads of data
data := generateRandomData(dataSize, 12345)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you run some of these generated data through a known-good implementation and have those results compared here in some tests?

@magik6k
Copy link
Copy Markdown
Author

magik6k commented Apr 28, 2026

Not needed as of filecoin-project/FIPs#1216 (comment)

@magik6k magik6k closed this Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants