Add buffering during encode #138
Conversation
|
Fuzzing for the win, uncovered #140 |
|
Needs redoing on top of #142 |
tcharding
force-pushed
the
10-17-buffered
branch
from
c2316bd
to
0d3216b
Compare
tcharding
force-pushed
the
10-17-buffered
branch
3 times, most recently
from
da82e4e
to
a957f7f
Compare
|
I'm not thrilled with the casts from
Alternately I can just ACK this as-is. What do you think? |
|
ACK a957f7f other than that. |
Why? |
|
@tcharding because every time I read the code I need to mentally re-check those facts. I agree it's safe, but it's a code smell. |
|
Ah yes, I guess I did it in some place without code comments. I agree its smelly, I'll see if I can come up with something better. |
Add an iterator adapter that is identical to `encode::CharIter` but yields `u8` byte values instead. This makes encoding cleaner because users do not need to cast `char`s to `u8`.
Currently we call `fmt::Write::write_char` and `io::Write::write_all` in a loop while iterating over the encoded characters of a bech32 string. This is inefficient. Add a buffer and copy ASCII bytes into it while looping the write the whole buffer out in a single call. The buffer has a size of 90 bytes because we know the bech32 string is guaranteed to be 90 chars or less.
As we did when writing in the `segwit` module; add a buffer to cache
characters and do writes a buffer at a time.
Although we do know the maximum length of the string we are encoding,
and we enforce this limit using `encoded_length::<Ck>()` we use an
unrelated fixed buffer size and loop over it as many times as needed.
This is because, surprisingly the following is not valid Rust
`let mut buf = [0_u8, Ck::CODE_LENGTH];`
We use a buffer of length 1024 which is larger than the 1023 code length
of the two `Checksum` implementations we provide with this
crate (`Bech32` and `Bech32m`).
tcharding
force-pushed
the
10-17-buffered
branch
from
a957f7f
to
c215c3d
Compare
|
Added |
|
Perfect, thanks! I'm a tiny bit disappointed that |
|
I wonder if we could stick a |
|
Flagging this, is the comment above actionable before #168 merges? |
|
@tcharding no. The more I think about this the less clear it is to me what we actually want to do here, buffering-wise. Let's just do 0.10.0 and then I think we'll be "done" our part (except for the error correction stuff which I continue to have on my todo list) and can ask Kix to come take a look. |
Currently we write char at a time to both writers (
fmt::Writeandstd::io::Write). We can improve performance by first collecting the ASCII bytes of the encoded bech32 string into a buffer on the stack then writing the buffer as a single call.This is purely an optimization.
The second patch is surprising, at least to me, would love to learn what I'm doing wrong. I thought this would be valid
But the compiler does not like the usage of an associated const.