-
-
Notifications
You must be signed in to change notification settings - Fork 67
Improve block codecs #492
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
Improve block codecs #492
Conversation
Codecov ReportBase: 92.84% // Head: 92.82% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #492 +/- ##
==========================================
- Coverage 92.84% 92.82% -0.03%
==========================================
Files 92 91 -1
Lines 4332 4291 -41
==========================================
- Hits 4022 3983 -39
+ Misses 310 308 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
I'm happy for this to be approved given the related discussion in #407 but do we want to extend to all codecs in this PR, or address later?
I was thinking of extending it; I should have some time next week to do it, and run some tests just to make sure all is good. I'll ask you to have another look when I push more changes. |
ea55a81
to
2df6daa
Compare
I ran test for all modified codecs, looks like it's at least not worse, but mostly better performance now: Raw data: results.tar.gz So I'm pretty comfortable merging that. |
2df6daa
to
f08e35f
Compare
thread_local std::vector<uint32_t> inbuf(block_size); | ||
thread_local std::vector<uint32_t> outbuf; | ||
thread_local std::array<std::uint32_t, block_size> inbuf{}; | ||
thread_local std::vector<uint32_t> outbuf; // TODO: Can we use array? How long does it need |
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.
Shall we convert this TODO to an issue?
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.
Will do.
f08e35f
to
9ecfc72
Compare
Fixes #407