-
Notifications
You must be signed in to change notification settings - Fork 27
perf: improve checksum calculation #688
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
Conversation
Optimize the checksum calculation that is used for read/write transactions. New implementation: ``` goos: darwin goarch: arm64 pkg: github.com/googleapis/go-sql-spanner cpu: Apple M3 BenchmarkChecksumRowIteratorRandom BenchmarkChecksumRowIteratorRandom/num-rows-1 BenchmarkChecksumRowIteratorRandom/num-rows-1-8 134152 8184 ns/op 13528 B/op 123 allocs/op BenchmarkChecksumRowIteratorRandom/num-rows-10 BenchmarkChecksumRowIteratorRandom/num-rows-10-8 16498 72459 ns/op 120690 B/op 1335 allocs/op BenchmarkChecksumRowIteratorRandom/num-rows-100 BenchmarkChecksumRowIteratorRandom/num-rows-100-8 1725 692326 ns/op 1153975 B/op 12781 allocs/op BenchmarkChecksumRowIteratorRandom/num-rows-1000 BenchmarkChecksumRowIteratorRandom/num-rows-1000-8 157 7566917 ns/op 11375170 B/op 130466 allocs/op BenchmarkChecksumRowIteratorRandom/num-rows-10000 BenchmarkChecksumRowIteratorRandom/num-rows-10000-8 14 83402393 ns/op 112045544 B/op 1294480 allocs/op ``` Original implementation: ``` goos: darwin goarch: arm64 pkg: github.com/googleapis/go-sql-spanner cpu: Apple M3 BenchmarkChecksumRowIteratorRandom BenchmarkChecksumRowIteratorRandom/num-rows-1 BenchmarkChecksumRowIteratorRandom/num-rows-1-8 33792 34931 ns/op 908 B/op 54 allocs/op BenchmarkChecksumRowIteratorRandom/num-rows-10 BenchmarkChecksumRowIteratorRandom/num-rows-10-8 4807 249917 ns/op 8768 B/op 531 allocs/op BenchmarkChecksumRowIteratorRandom/num-rows-100 BenchmarkChecksumRowIteratorRandom/num-rows-100-8 494 2408323 ns/op 87580 B/op 5301 allocs/op BenchmarkChecksumRowIteratorRandom/num-rows-1000 BenchmarkChecksumRowIteratorRandom/num-rows-1000-8 44 25243051 ns/op 870935 B/op 53003 allocs/op BenchmarkChecksumRowIteratorRandom/num-rows-10000 BenchmarkChecksumRowIteratorRandom/num-rows-10000-8 4 259823406 ns/op 8691096 B/op 530018 allocs/op ```
Summary of ChangesHello @olavloite, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant performance optimization to the checksum calculation logic used in read/write transactions. By replacing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request significantly improves the performance of checksum calculations by replacing the encoding/gob-based implementation with a more direct and efficient hashing mechanism. The benchmark results clearly demonstrate the benefits of this change. The new implementation is well-structured and the tests have been updated accordingly. I've found one critical issue that could lead to a panic, and a couple of minor improvement opportunities in the test utility code. Overall, this is a great performance enhancement.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…-sql-spanner into optimize-checksum-iterator
bhatt4982
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM...
The int and float buffers were defined as global variables, which meant that they could have been used concurrently if multiple checksumIterators were used at the same time.
bhatt4982
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM...
Optimize the checksum calculation that is used for read/write transactions.
New implementation:
Original implementation: