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

all: replace division with right shift if possible #29911

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gitglorythegreat
Copy link
Contributor

@gitglorythegreat gitglorythegreat commented Jun 3, 2024

Replace x.Div(y, 2^n) with x.Rsh(y, n). Using Rsh is cleaner and faster.

  • For big.Int, even if y is negative, Rsh is rounding down (towards negative infinity), the same behavior with Div.
  • For uint256, both Rsh and Div are for unsigned numbers, while SRsh and SDiv are for signed numbers.

crypto/crypto.go Outdated Show resolved Hide resolved
@MariusVanDerWijden
Copy link
Member

Hmm feels like a micro-optimization to me, that doesn't have much impact on the overall speed, but reduces readability.
I made the following benchmarks:

func BenchmarkRSH(b *testing.B) {
	integer := new(big.Int).Rsh(common.Big1, uint(b.N)+3)
	for i := 0; i < b.N; i++ {
		integer.Rsh(integer, 1)
	}
}

func BenchmarkDiv(b *testing.B) {
	integer := new(big.Int).Rsh(common.Big1, uint(b.N)+3)
	for i := 0; i < b.N; i++ {
		integer.Div(integer, common.Big2)
	}
}

func BenchmarkDiv2(b *testing.B) {
	integer := new(big.Int).Rsh(common.Big1, uint(b.N)+3)
	for i := 0; i < b.N; i++ {
		integer.Div(integer, big.NewInt(2))
	}
}
BenchmarkRSH-24    	311260804	         3.660 ns/op	       0 B/op	       0 allocs/op
BenchmarkDiv-24    	127801504	         9.504 ns/op	       0 B/op	       0 allocs/op
BenchmarkDiv2-24    	135632257	         8.908 ns/op	       0 B/op	       0 allocs/op

While the shift is much faster, the div is generally pretty fast. The only place where we might see a difference is in transaction unmarshaling, as we're doing a lot of those

@gitglorythegreat
Copy link
Contributor Author

gitglorythegreat commented Jun 17, 2024

@MariusVanDerWijden

Your benchmark is wrong. Rsh(common.Big1, uint(b.N)+3) is 0. You are computing 0 / 2 in all cases. You should use Lsh or big.NewInt here.

Apart from speed, this PR removes some global variables. I think this is good to have.

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.

None yet

4 participants