Skip to content
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

base64::decode_config excessive memory allocation #195

Closed
dkomanov opened this issue Oct 15, 2022 · 5 comments
Closed

base64::decode_config excessive memory allocation #195

dkomanov opened this issue Oct 15, 2022 · 5 comments
Assignees

Comments

@dkomanov
Copy link

dkomanov commented Oct 15, 2022

According to the source code:

pub fn decode_config<T: AsRef<[u8]>>(input: T, config: Config) -> Result<Vec<u8>, DecodeError> {
    let mut buffer = Vec::<u8>::with_capacity(input.as_ref().len() * 4 / 3);

    decode_config_buf(input, config, &mut buffer).map(|_| buffer)
}

decode_config method allocates excessive amount of memory: len * 4 / 3 instead of (len + 3) * 3 / 4.

@dkomanov
Copy link
Author

Oops. I guess I missed #179.

The fix that is done is not the most performant... I did some benchmarking with different input sizes:

method                                input  output  avg time
base64::decode_config (excessive)        16      12        86
base64::decode_config (excessive)        68      51       133
base64::decode_config (excessive)       136     102       183
base64::decode_config (excessive)       668     501       579
base64::decode_config (excessive)      1336    1002      1134
base64::decode_config_buf (no alloc)     16      12        93
base64::decode_config_buf (no alloc)     68      51       136
base64::decode_config_buf (no alloc)    136     102       191
base64::decode_config_buf (no alloc)    668     501       588
base64::decode_config_buf (no alloc)   1336    1002      1125
base64::decode_config_slice (unsafe)     16      12        79
base64::decode_config_slice (unsafe)     68      51       119
base64::decode_config_slice (unsafe)    136     102       170
base64::decode_config_slice (unsafe)    668     501       560
base64::decode_config_slice (unsafe)   1336    1002      1061
base64::decode_config_slice (safe)       16      12        93
base64::decode_config_slice (safe)       68      51       133
base64::decode_config_slice (safe)      136     102       183
base64::decode_config_slice (safe)      668     501       588
base64::decode_config_slice (safe)     1336    1002      1101

And the code:

// Current solution
pub fn base64_decode_config_buf_no_prealloc(s: &String) -> Vec<u8> {
    let mut buffer = Vec::<u8>::new();
    base64::decode_config_buf(s, base64::STANDARD_NO_PAD, &mut buffer).map(|_| buffer).unwrap()
}

// Previous solution
pub fn base64_decode_config_buf_excessive_alloc(s: &String) -> Vec<u8> {
    let mut buffer = Vec::<u8>::with_capacity(s.len() * 4 / 3);
    base64::decode_config_buf(s, base64::STANDARD_NO_PAD, &mut buffer).map(|_| buffer).unwrap()
}

// The most performant solution with unsafe
pub fn base64_decode_config_slice(s: &String) -> Vec<u8> {
    let mut buffer = Vec::<u8>::with_capacity((s.len() + 3) * 3 / 4);
    unsafe {
        let mut sl = std::slice::from_raw_parts_mut(buffer.as_mut_ptr(), buffer.capacity());
        let size = base64::decode_config_slice(s, base64::STANDARD_NO_PAD, &mut sl).unwrap();
        buffer.set_len(size);
    }
    buffer
}

// Safe performant solution.
pub fn base64_decode_config_slice_memset(s: &String) -> Vec<u8> {
    let mut buffer = vec![0; (s.len() + 3) * 3 / 4];
    let size = base64::decode_config_slice(s, base64::STANDARD_NO_PAD, &mut buffer).unwrap();
    buffer.truncate(size);
    buffer
}

@marshallpierce
Copy link
Owner

Thanks for the report. I had originally held off from doing the pre-sized vec because if the input is invalid then it's wasted, but that's probably a poor trade-off.

I have a bunch of 1.0 work that's been waiting until I have enough time to give it the focus it deserves, but I'll try to get this improvement out promptly in a patch release without waiting for 1.0.

@marshallpierce marshallpierce self-assigned this Oct 16, 2022
@dkomanov
Copy link
Author

If you interested, there are few interesting comments on reddit.

@marshallpierce
Copy link
Owner

Yep, there are faster crates out there now -- the major feature of the 1.0 work is allowing pluggable "engines" so users can use CPU-specific implementations under the hood while still enjoying all the rest of the functionality of this crate so hopefully we can all stop duplicating work.

@marshallpierce
Copy link
Owner

Addressed via master...mp/decode-vec-size-backport in 0.13.1. I'll merge it by hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants