Skip to content
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

Move division by 0 check out of the bloom loops #2622

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Jan 17, 2024

Why this should be merged

This PR provides hints to the compiler that numBits can be assumed to be non-zero when iterating over the bloom filter hashes.

The compiler requires these hints because there is a subtle change in behavior of the function if len(hashSeeds) == 0.

image

How this works

By including _ = 1 % numBits // hint to the compiler that numBits is not 0 outside of the loop we move the numBits == 0 division by 0 panic check outside of the loop.

We can see the difference in the compiled code of contains here (from go tool compile -S):

...
	PCDATA	$3, $1
	LSL	$3, R4, R2
+	CBZ	R2, $PANICDIVIDE
	MOVD	ZR, R5
	MOVD	$1, R7
	JMP	$LOOPCMP
UPDATEHASH:
	AND	$7, R8, R8
	MOVBU	(R3)(R9), R9
	LSR	R8, R9, R8
	ADD	$1, R5, R5
	AND	R8, R7, R7
LOOPCMP:
	CMP	R5, R1
	BLE	$EXIT
	MOVBU	R7, R8
	CBZW	R8, $EXIT
	MOVD	(R0)(R5<<3), R8
	EOR	R6@>47, R8, R6
-	CBZ	R2, $PANICDIVIDE
	UDIV	R2, R6, R8
	MSUB	R2, R6, R8, R8
	LSR	$3, R8, R9
	CMP	R8>>3, R4
	BHI	$UPDATEHASH
	JMP$PANICOOB
EXIT:
	MOVBU	R7, R1
	CMPW	$0, R1
	CSET	NE, R0
	LDP	-8(RSP), (R29, R30)
	ADD	$32, RSP
	RET	(R30)
PANICOOB:
	MOVD	R9, R0
	MOVD	R4, R1
	PCDATA	$1, $1
	CALL	runtime.panicIndexU(SB)
PANICDIVIDE:
	CALL	runtime.panicdivide(SB)
...

I wasn't able to figure out how to get the compiler to move the PANICOOB call out of the loop, which would further optimize the check.

How this was tested

Before:

BenchmarkAdd-12    		43385254	        27.47 ns/op	       0 B/op	       0 allocs/op
BenchmarkContains-12    	52014932	        22.85 ns/op	       0 B/op	       0 allocs/op

After:

BenchmarkAdd-12    		43245295	        27.08 ns/op	       0 B/op	       0 allocs/op
BenchmarkContains-12    	53046825	        22.50 ns/op	       0 B/op	       0 allocs/op

🚀

@StephenButtolph StephenButtolph self-assigned this Jan 17, 2024
@StephenButtolph StephenButtolph added this to the v1.10.18 milestone Jan 17, 2024
@aaronbuchwald
Copy link
Collaborator

Great meme

@StephenButtolph StephenButtolph added this pull request to the merge queue Jan 17, 2024
Merged via the queue into dev with commit fee76e8 Jan 17, 2024
17 checks passed
@StephenButtolph StephenButtolph deleted the optimize-bloom-filter-panic-check branch January 17, 2024 19:55
chand1012 pushed a commit to multisig-labs/avalanchego that referenced this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants