rust: make CRC checking optional #4573
Merged
Conversation
This file contains hidden or 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
Summary:
Instead of computing checksums for every record, we can now compute
checksums only for records that fail proto parsing. This means that bit
flips in payloads like string fields may not be caught. But it comes
with a significant performance improvement:
- 2.42× (±0.24×) faster for `mnist` (125 ms → 50 ms)
- 1.19× (±0.02×) faster for `edge_cgan` (10 s → 8.5 s)
- 1.23× (±0.01×) faster for `nndiv` (182 s → 148 s)
Both the main RustBoard binary and the `bench` binary now take
`--checksum`/`--no-checksum` flags, defaulting to `--no-checksum`.
Test Plan:
Unit tests extended. Benchmarked with:
```
hyperfine -w3 \
'./target/release/bench --logdir ~/testdata/edge_cgan/ {cksum}' \
-L cksum --checksum,--no-checksum
```
wchargin-branch: rust-optional-crc
wchargin-source: 767e7133f9f14808acff0f8f1f64f746b5b853bd
wchargin
added
theme:performance
wchargin-branch: rust-optional-crc wchargin-source: 11c8a1780124e7ab9023f5e78f04bca48f8028d1
wchargin-branch: rust-optional-crc wchargin-source: 11c8a1780124e7ab9023f5e78f04bca48f8028d1
stephanwlee
approved these changes
Jan 20, 2021
stephanwlee
reviewed
Jan 20, 2021
| } else { | ||
| match Event::decode(&record.data[..]) { | ||
| Ok(proto) => proto, | ||
| Err(e) => { |
Contributor
There was a problem hiding this comment.
Nice. Checking checksum on error!
Contributor
Author
There was a problem hiding this comment.
Yep, I figured that it could come in handy, and it’s easy enough and
costs nothing in the happy path.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary:
Instead of computing checksums for every record, we can now compute
checksums only for records that fail proto parsing. This means that bit
flips in payloads like string fields may not be caught. But it comes
with a significant performance improvement:
mnist(125 ms → 50 ms)edge_cgan(10.4 s → 8.7 s)nndiv(182 s → 148 s)Both the main RustBoard binary and the
benchbinary now take--checksum/--no-checksumflags, defaulting to--no-checksum.Test Plan:
Unit tests extended. Benchmarked with:
wchargin-branch: rust-optional-crc