This repository was archived by the owner on Apr 23, 2024. It is now read-only.
examples/benchmark: one-way benchmark #318
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 14, 2021
leighmcculloch
added a commit
that referenced
this pull request
Sep 15, 2021
Remove use of the google/go-cmp package for checking the equality of complex types. Benchmarking in #318 has shown that we spend time in the go-cmp package, and there's some interesting things also in the profiles showing a go routine running for detecting data races.
leighmcculloch
added a commit
to stellar/go
that referenced
this pull request
Sep 20, 2021
…3914) Store the generated ed25519 public and private keys within the `FromAddress` and `Full` types. Also, simplified our implementation of crc16. Benchmarking with stellar-deprecated/starlight#318 identified that the app was spending as much time constructing keys as it was verifying and signing. It's expected that verifying and signing will be somewhat expensive, but it was a surprise that repeated use of an in memory keypair would result in repeated regeneration of the keys. Regenerating keys doesn't only consume CPU time, it allocates memory. The `FromAddress` and `Full` types store the public and private key in strkey [SEP-23] form in memory. Every `Hint`, `Sign`, and `Verify` call decodes the strkey then generates the ed25519 keys used by the Go stdlib. That key setup is pretty expensive, especially for verifying with a secret key, and for the hint comparisons. There are some competing attributes of keypair that make introducing this change non-trivial. 1. The keypair types are safe for concurrent use if you treat them as immutable after construction. Introducing a cache and caching on first hint/sign/verify, would break that attribute because the types would be mutable and calling those functions concurrently would no longer be safe without a synchronization primitive such as `sync.Once` or `sync.Mutex`. 2. The keypair types are copyable. Introducing a simple `sync.Once` on the keypairs to address the first point would break the copyable attribute because synchronizing types like `sync.Once` and `sync.Mutex` are not safe to copy in Go. These two points leave us with one reasonable solution: generating the ed25519 keys when the keypair FromAddress and Full values are created. This makes some assumptions about how the keypair types are used. For example, if you use the `Full` type to purely hold an `S...` strkey but use none of the signing, verifying, or hint functions, the `Full`s construction will now be slower. Also, if you call `keypair.Random` frequently that call will also slower. However, these slower operations are shifting existing time spent in the other functions, so this is not new time applications will spend, unless those applications are generating random keys without actually using the keys. Generally applications use types for functionality and these types are primarily present for signing and verifying. The crc16 implementation is also simplified, removing use of the `binary.Write` and `bytes.Buffer` which are both unnecessary. Removing the use of those reduce the allocations from 24 to 18 in calls to `FromRawSeed` (used by `Random`) and helps to offset a little the impact of adding new weight to that function. ### Benchmarks The following benchmarks were run with the `benchmarks_test.go` against the `master` branch and this branch, then the results were pushed through [benchstat] to get comparisons. ``` goos: darwin goarch: amd64 pkg: github.com/stellar/go/keypair cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz ``` ``` name old time/op new time/op delta FromAddress_ParseAddress-8 464ns ± 0% 375ns ± 0% -19.26% FromAddress_Hint-8 468ns ± 0% 3ns ± 0% -99.43% FromAddress_Verify-8 54.0µs ± 0% 53.1µs ± 0% -1.53% Full_ParseFull-8 239ns ± 0% 250ns ± 0% +4.56% Full_FromRawSeed-8 439ns ± 0% 19445ns ± 0% +4330.39% Full_Hint-8 19.4µs ± 0% 0.0µs ± 0% -99.99% Full_Verify-8 73.0µs ± 0% 57.9µs ± 0% -20.80% Full_Sign-8 42.0µs ± 0% 22.2µs ± 0% -47.03% name old alloc/op new alloc/op delta FromAddress_ParseAddress-8 244B ± 0% 178B ± 0% -27.05% FromAddress_Hint-8 244B ± 0% 0B -100.00% FromAddress_Verify-8 244B ± 0% 1B ± 0% -99.59% Full_ParseFull-8 128B ± 0% 128B ± 0% 0.00% Full_FromRawSeed-8 374B ± 0% 776B ± 0% +107.49% Full_Hint-8 420B ± 0% 0B -100.00% Full_Verify-8 420B ± 0% 0B -100.00% Full_Sign-8 484B ± 0% 0B -100.00% name old allocs/op new allocs/op delta FromAddress_ParseAddress-8 6.00 ± 0% 4.00 ± 0% -33.33% FromAddress_Hint-8 6.00 ± 0% 0.00 -100.00% FromAddress_Verify-8 6.00 ± 0% 0.00 -100.00% Full_ParseFull-8 2.00 ± 0% 2.00 ± 0% 0.00% Full_FromRawSeed-8 10.0 ± 0% 18.0 ± 0% +80.00% Full_Hint-8 10.0 ± 0% 0.0 -100.00% Full_Verify-8 10.0 ± 0% 0.0 -100.00% Full_Sign-8 11.0 ± 0% 0.0 -100.00% ``` I used a Makefile to automate running tests on master and the branch and comparing them. The Makefile is visible in PR #3914 at 0f17280 if anyone is of interest, but it was removed from the PR before merge. ### Testing Existing unit tests have been updated and cover the api surface area pretty well. There was a small area of the package without test coverage and I have added tests for that area in #3922 which will be merged ahead of this PR. In addition to unit tests I wrote a regression test that is in this PR but won't be merged with the final merge. It uses random input and executes all of the functions of the FromAddress and Full types on both the master branch and on this PR and compares the outputs of all of them. The tests have shown no observable difference. The regression test also runs all its assertions in parallel multiple times for the same input, and when the regression test was run with the race detector enabled there was no races detected. This provides some confidence over what we know thereotically, that this PR does not change the keypair package's safety for us in concurrent applications when the keypair values are used in an immutable fashion. The regression tests were removed from the PR before merge, but they are visible in PR #3914 before commit 7372f9d. [SEP-23]: https://stellar.org/protocol/sep-23 [benchstat]: https://pkg.go.dev/golang.org/x/perf/cmd/benchstat
acharb
reviewed
Sep 28, 2021
There was a problem hiding this comment.
lgtm, just had 1 question, and 1 thought below
💡 eventually it may be a good idea to add a readme in this folder (or add to TLD readme) to describe how someone can test for themselves the numbers we come up with. But that can probably be added later after more of the benchmarking tests are done so we have a better idea what to write
Comment on lines
+146
to
+153
| tick := time.Tick(1 * time.Second) | ||
| for i := 5; i >= 0; { | ||
| fmt.Fprintf(os.Stderr, "%d\n", i) | ||
| i-- | ||
| if i >= 0 { | ||
| <-tick | ||
| } | ||
| } |
There was a problem hiding this comment.
❓ a little confused by this part. Is this just counting down from 5?
There was a problem hiding this comment.
It's just a countdown at the beginning to make sure both ends are setup. There's an issue where one side may not be ready and have ingested the open and updated their internal state. Rather than add code to handle that more gracefully I added this delay.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Add a client that benchmarks a bidirectional payment channel that has payments going in one direction at a time.
Why
To learn where the bottlenecks are in both the implementation and the design.
Close #40