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

Faster and cached square root #12191

Merged
merged 6 commits into from
Mar 29, 2023
Merged

Faster and cached square root #12191

merged 6 commits into from
Mar 29, 2023

Conversation

potuz
Copy link
Contributor

@potuz potuz commented Mar 27, 2023

This PR adds a different method to compute the integer square root of a number. It first uses a simple mutex and a cache for the last computation done. If there is a cache miss, then it uses simple Newton approximation to find the new square root.

Rationale:

Square roots of total balances are computed when computing

  • AttestationsDeltas in the epoch precompute.
  • When computing the base reward per increment (this is used when computing several rewards)

In all cases we pass the total effective balance of validators that were active in the current epoch. This number stays constant throughout the epoch and changes slowly, by multiples of 10**9 Gwei, on epoch transitions.

This makes us believe that a single value cache is already good enough to avoid this computation on block processing, as the effective balance will not change intra-epoch. And In case of a cache miss, we can start a Newton approximation with the latest cached value that is guaranteed to be close to the actual value.

Possible improvemnents:

  • We could use a map with 2 or three cached values given that in some places we use only balances that have participated in the current or previous epoch (for attestation rewards for example) and in others we use the full active balance.
  • We could improve the Newton approximation by using the fact that balances only change in multiples of 1ETH instead of arbitrary integer numbers. We felt the complication of this would give us very little gain

Some Benchmarks

Below there are some benchmarks on a AMD Ryzen 5 3600. On computing raw square roots for over 52 bits our method is about x2.3 faster, that is comparable with Golang's square root computation for numbers under 52 bits.

-----------------------------------------------------------------------------
goos: linux
goarch: amd64
cpu: AMD Ryzen 5 3600 6-Core Processor
BenchmarkIntegerSquareRootBelow52Bits-12       	34503249	        59.55 ns/op
BenchmarkIntegerSquareRootAbove52Bits-12       	11731746	        97.91 ns/op
BenchmarkSquareRootEffectiveBalance-12         	35858377	        46.14 ns/op
BenchmarkSquareRootBabylonian-12               	41247252	        27.98 ns/op
BenchmarkSquareRootOldWay-12                   	21897693	        55.02 ns/op
BenchmarkIntegerSquareRoot_WithDatatable-12    	85549964	        14.38 ns/op
PASS

@potuz potuz requested a review from a team as a code owner March 27, 2023 14:47
@potuz potuz added the Ready For Review A pull request ready for code review label Mar 27, 2023
@@ -43,6 +50,25 @@ var squareRootTable = map[uint64]uint64{
4194304: 2048,
}

// SquareRootEffectiveBalance implements Newton's algorithm to compute the square root of
// the given uint64 starting from the last cached value
func SquareRootEffectiveBalance(balance uint64) uint64 {
Copy link
Member

Choose a reason for hiding this comment

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

Qustion on general usages for this function. Is this only intended for EffectiveBalance, if yes then we should move the function to a better location not general math helper

If it's not only intended for EffectiveBalance such that it could be applied to other usages, we can rename this function to SquareRootCached or something else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on the size of the cache it can be used for everything, I could move the cache to a map.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this function again, given it's just a single value, it might be best to move (hide) it to core pkg because we don't want to share it among others. Imagine Alice joins the team tomorrow and start using SquareRootEffectiveBalance for something less relevant

@potuz
Copy link
Contributor Author

potuz commented Mar 29, 2023

Tested that the function returns the same value as our previous method for balances between 700K validators and 5 million validators.

@potuz potuz merged commit 6ebe5ea into develop Mar 29, 2023
@delete-merged-branch delete-merged-branch bot deleted the better_square_root branch March 29, 2023 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants