rust-lang / ferris-says

A Rust flavored implementation of `cowsay`
https://crates.io/crates/ferris-says
Apache License 2.0
194 stars 34 forks source link

Reconsider buffering in `say` #57

Open dtolnay opened 7 months ago

dtolnay commented 7 months ago

ferris_says::say currently works like this:

pub fn say<W>(..., writer: W) -> Result<()> 
where 
    W: Write, 
{ 
    let mut write_buffer = SmallVec::<[u8; BUFSIZE]>::new(); 

    write_buffer.extend_from_slice(...);
    write_buffer.extend_from_slice(...);
    ...

    writer.write_all(&write_buffer)
}

This is not a pattern we would want users to mirror in their own projects. It is unusual that it would be appropriate for a callee to apply buffering like this to an arbitrary W: Write that it receives, because there is no way to inspect whether W is already buffered, and we just waste time shuffling data across multiple buffers if it is. Typically a W: Write should just be written to directly by a callee, and the caller is in charge of buffering if they want it.

If we do keep buffering inside say, then SmallVec is an odd choice for it. For large output, this ends up spilling to a heap allocation, when a better approach would be to flush the buffer by writing the contents to writer when it fills up, and skip the heap allocation.

The point of this issue isn't to optimize say, it is only to take best advantage of the opportunity to provide novices with a good example that will serve them well in their own future work, which the current approach in this crate doesn't do.

dtolnay commented 7 months ago

Buffering options:

buffer type destination for small input action when filled
none directly to writer N/A
SmallVec<[u8; N]> stack spill to heap
Vec<u8> heap continue growing on heap
BufWriter<W> heap flush to writer
ArrayBuf<W, N> stack flush to writer
lcnr commented 6 months ago

I agree that buffering inside of say is not something we should recommend to new users. If we were to keep buffering I would at least like to switch to BufWriter (ArrayBuf is also fine for me).

I would personally like to remove buffering from say but add an additional example using BufWriter in the method docs as a showcase for users.