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

Refactor bytesutil, add support for go1.20 slice to array conversions #11838

Merged
merged 7 commits into from
Jan 10, 2023

Conversation

prestonvanloon
Copy link
Member

@prestonvanloon prestonvanloon commented Jan 3, 2023

What type of PR is this?

Other

What does this PR do? Why is it needed?

This PR adds early support for go1.20 slice to array conversions. Also see go1.20 notes

Other changes:

  • Refactored this package so that everything wasn't in one bytes.go and bytes_test.go file.
  • Fixed PadTo where it would unnecessary pad a slice that was exactly the desired size.
  • Added benchmark to show improvement for bytes32 conversions.

Which issues(s) does this PR fix?

Other notes for review

Benchmark results indicate a significant reduction in conversions.

// Before
BenchmarkToBytes32-8    1000000000               0.4843 ns/op                                            

// After
BenchmarkToBytes32-8    1000000000               0.2438 ns/op

Tested with gotip @ go version devel go1.20-db36eca Thu Dec 29 23:36:05 2022 +0000 linux/amd64

This is a very high traffic method in Prysm and performance improvements could come in during epoch processing and other high impact workloads.

@prestonvanloon prestonvanloon requested a review from a team as a code owner January 3, 2023 17:08
Copy link
Contributor

@saolyn saolyn left a comment

Choose a reason for hiding this comment

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

Had a look through everything and it looks like a huge improvement.
I did notice there are a couple of functions in bytes.go ( FromBytes48Array ) and a couple others in integers.go that don't seem to have any unit tests. Might be worth adding them as well.

@prestonvanloon
Copy link
Member Author

Thanks @saolyn. I don't have capacity to add those missing tests at this time. They only notable additions in this PR are in the file bytes_go120.go. Everything else is a refactoring of existing stuff.

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