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
Implement iter::size_hint and ExactSizeIterator for Witness Iter #1053
Conversation
895b5cd
to
df71ecd
Compare
src/blockdata/witness.rs
Outdated
@@ -43,7 +43,7 @@ pub struct Witness { | |||
} | |||
|
|||
/// Support structure to allow efficient and convenient iteration over the Witness elements | |||
pub struct Iter<'a>(::core::slice::Iter<'a, u8>); | |||
pub struct Iter<'a>(::core::slice::Iter<'a, u8>, usize); |
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.
If it is already two fields, I would name them.
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.
sure, I added a small test also
1149c35
to
52014d7
Compare
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.
ACK 52014d7
src/blockdata/witness.rs
Outdated
@@ -43,7 +43,10 @@ pub struct Witness { | |||
} | |||
|
|||
/// Support structure to allow efficient and convenient iteration over the Witness elements | |||
pub struct Iter<'a>(::core::slice::Iter<'a, u8>); | |||
pub struct Iter<'a> { | |||
inner: ::core::slice::Iter<'a, u8>, |
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.
nit: We don't need these leading colons anymore, since the MSRV bump.
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 hold off on merging since I'd like this to have a chance to be addressed.
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.
fixed
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.
ACK 52014d7 nice!
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.
LOL at first I thought small ones got worse, I'm actually surprised they didn't. (Would be happy to see an explanation if anyone knows.)
ACK 52014d7
// with size_hint // test blockdata::witness::benches::bench_big_witness_to_vec ... bench: 313 ns/iter (+/- 13) // test blockdata::witness::benches::bench_witness_to_vec ... bench: 204 ns/iter (+/- 11) // without // test blockdata::witness::benches::bench_big_witness_to_vec ... bench: 489 ns/iter (+/- 28) // test blockdata::witness::benches::bench_witness_to_vec ... bench: 221 ns/iter (+/- 102)
52014d7
to
ec8dada
Compare
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.
ACK ec8dada
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.
ACK ec8dada
…xactSizeIterator for Witness Iter ec8dada Implement iter::size_hint and ExactSizeIterator for Witness Iter (Riccardo Casatta) Pull request description: close rust-bitcoin/rust-bitcoin#1050 I don't think we need to change the `collect` since it use the `size_hint()` lower bound to initially allocate // with size_hint // test blockdata::witness::benches::bench_big_witness_to_vec ... bench: 313 ns/iter (+/- 13) // test blockdata::witness::benches::bench_witness_to_vec ... bench: 204 ns/iter (+/- 11) // without // test blockdata::witness::benches::bench_big_witness_to_vec ... bench: 489 ns/iter (+/- 28) // test blockdata::witness::benches::bench_witness_to_vec ... bench: 221 ns/iter (+/- 102) The reason why the small witness doesn't get big perf boost is because by default vec allocates 4 slots ACKs for top commit: Kixunil: ACK ec8dada apoelstra: ACK ec8dada Tree-SHA512: dbe09ba6ebd4014fe0639412894beedab6cc7e844a5ec1697af8f0694b62ae5d423a801df1b48ac7029444c01877975e2d5168728f038fbd4f5808bda90e0f2f
close #1050
I don't think we need to change the
collect
since it use thesize_hint()
lower bound to initially allocate// with size_hint
// test blockdata::witness::benches::bench_big_witness_to_vec ... bench: 313 ns/iter (+/- 13)
// test blockdata::witness::benches::bench_witness_to_vec ... bench: 204 ns/iter (+/- 11)
// without
// test blockdata::witness::benches::bench_big_witness_to_vec ... bench: 489 ns/iter (+/- 28)
// test blockdata::witness::benches::bench_witness_to_vec ... bench: 221 ns/iter (+/- 102)
The reason why the small witness doesn't get big perf boost is because by default vec allocates 4 slots