Various changes. #91
Closed
Various changes. #91
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
Prior to this commit a variety of property based tests existed and used a manual approach, consisting of filling vectors with random data and choosing random configs using the rand crate. This moves to using quickcheck to handle a lot of the grunt work that was previously done manually. In addition to the simpler code quickcheck also provides "shrinking" of input data to reduce test cases when possible. This can make troubleshooting test failures much simpler. In the process of refactoring the tests it became clear that there exists a lot of redundancy. I took the liberty of removing a variety of test cases that entirely overlapped with an already existing case. I believe the end result is net gain in terms of readability and maintenance of the test cases without any loss of code coverage.
decode_config invokes decode_config_buf which resizes the vector to the correct size. There is no reason for decode_config to attempt to pre-allocate the buffer. In the best case it moves the allocation from within decode_config_buf to within decode_config, but unfortunately there is a bug in the calculation used by decode_config that means it underallocates for tiny buffers (requiring decode_config_buf to allocate again), and allocates way more memory than requires for larger buffers.
The configuration consists of 3 traits. Encoding: Determine how to encode a 6-bit value into an 8-bit value. Decoding: Determine how to decode an 8-bit value into a 6-bit value. Padding: Whether to use padding and if so which character to use. These traits are currently sealed so they cannot be implemented outside this crate. This could be changed in the future, but until the trait definitions get a little more flushed out it seemed wise to not have users relying on the current definition. Each of the 3 supported alphabets defines it's own zero sized type and implements the traits. Public API methods are now converted to be generically bounded by the traits which will monomorphize each function specifically for the desired configuration. This results in a small performance improvement. This method also provides a path to supporting custom alphabets in a reasonable way. That functionality will be done in a future commit.
There is now a CustomConfig type. It supports custom alphabets and
padding and can be built with a builder pattern.
// A custom config that matches the builtin Crypt alphabet.
let custom_config = CustomConfigBuilder::with_alphabet(
b"./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
)
.with_padding(b'=')
.build()?;
The alphabet and padding provided must be ascii characters. This
limitation is in place to ensure that encoded values are valid UTF-8 and
can therefore be represented as rust String values.
An immutable pointer to CustomConfig implements Encoding + Decoding +
Padding. This allows it to be used in contexts where the standard
builtin configs are used.
Creating a CustomConfig is somewhat heavyweight and the resulting type
is somewhat large (~2.5KB). It is expected that building the config
would be early in program execution and that an immutable reference is
what will be efficiently passed around. i.e. The configuration would be a
good candidate to be used with lazy_static.
This is in anticipation for further submodules within each module.
This submodule will contain the logic and methods for encoding data in larger block sizes.
This submodule will contain the logic and methods for decoding data in larger block sizes. This change also refactors the decode_helper to no longer have baked-in assumptions about the block size used when decoding.
This implements SSE and AVX2 encoding, and AVX2 decoding on x86 and x86_64 for the standard configurations. SSE decoding was slower than the scalar implementation on the machines I have so I did not implement it. Custom configurations always use the scalar configuration.
Previously EncoderWriter stored a mutable reference to the generic writer, which seems intuitive but I believe is an anti-pattern. Instead, it's more flexible to take ownership of the underlying writer. The standard library has a blanket impl<W> Write for &mut W. This allows users that want to retain ownership of the underlying writer to pass a mutable reference into the EncoderWriter, but users that want to relinquish ownership can pass the underlying writer by value and it will be dropped when EncoderWriter is dropped.
|
Thanks, I'll dig into this and your other changes once #90 is dealt with (hopefully this weekend). |
|
I'm going to close this pull request as it's probably suffered bitrot over the months. If you're still interested in implementing this there is an improved version in my base64 library Feel free to take any bits you like. |
|
Thanks for your contributions; I will do that! |
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a large pull request and it makes a number of individual changes. Feel free to pick and choose the pieces that you like and disregard anything that you don't. Along with some small cleanups, this moves to the trait based configuration I outlined in #89, where each of the standard configs are a zero sized type, and adds a CustomConfig, as requested in #87, to work with arbitrary alphabets and padding (well almost arbitrary, they still need to be ascii characters). It also implements SIMD encoding and decoding for x86/x86_64. The SIMD operations slow down encoding/decoding 3 byte buffers by <5%, but show a considerable speedup for any buffer larger than that in the benchmarks.
On my avx2 machine, I see a 3x improvement in throughput when encoding a 3k buffer, and a 5x improvement when decoding a 3k buffer.
This was just a learning exercise for me to get some hands on experience with rust, I'm not invested in any of these changes.