5 ms·

I'm surprised serde_json takes a std::io::Read rather than a std::io::BufRead. I wonder why they made this choice. Not only does using plain Read make this mis

I'm surprised serde_json takes a std::io::Read rather than a std::io::BufRead. I wonder why they made this choice. Not only does using plain Read make this mistake possible, but it also means they have to be doing byte-by-byte processing. [1] So not really possible to do SIMD-oriented parsing acceleration like simdjson uses. (I gather simdjson's approach is quite sophisticated, but a simpler version is to scan for the next delimiter via a memchr2 on " and \, then have a routine that validates the characters until then are valid via SIMD methods.) [1] or do larger reads into their own buffer. But they're clearly not doing that given this strace output. And it'd be redundant when folks do use BufRead.

orlp 5y ago
Taking in a generic T: Read is the recommended way to accept streaming inputs. It is up to the user whether they want to buffer or not. You can pass in a BufReader into a T: Read no problem, and it will buffer any reads. That the reads internally are buffered is an implementation detail serde_json has no business knowing about. The converse is potentially disastrous - if you read too much from a TCP stream you could hang your program waiting for more input that never comes.
- scottlamb 5y ago
  > Taking in a generic T: Read is the recommended way to accept streaming inputs. Recommended by whom? When you want to process stuff in bulk, it's better to use BufRead::fill_buf (reusing the BufRead's buffer) rather than copy into your own. For example, I wrote some code which basically skips some escape bytes. [1] It wraps a BufRead and is itself a BufRead. Its caller actually can process bytes straight from the buffer of the BufRead you supply, skipping two layers of copying. > The converse is potentially disastrous - if you read too much from a TCP stream you could hang your program waiting for more input that never comes. You can implement any idea badly, but that doesn't mean the idea itself is bad. There's a difference between taking advantage of buffering and expecting to operate only on full, fixed-sized buffers. [1] https://github.com/dholroyd/h264-reader/blob/60ed66dc4dbfe745ceff80692902273b260f2ba9/src/rbsp.rs#L46 https://github.com/dholroyd/h264-reader/blob/60ed66dc4dbfe74...
sitta 5y ago
For what it's worth, if you want the best performance out of Serde you have to use from_str or from_slice, regardless of if you pass a BufRead to it. https://github.com/serde-rs/json/issues/160 https://github.com/serde-rs/json/issues/160
- scottlamb 5y ago
  In that issue, they're talking about specialization. With that language feature, they'd be able to make this interface optimal for either mode: * if the caller supplies a BufRead, they can use fill_buf. * if the caller supplies a Read that isn't BufRead, they can wrap it in a BufReader or the like, and then use fill_buf on that. Stable Rust doesn't have specialization today, so at most one of those can be handled optimally. (It looks like neither of them is handled well at all right now.) Maybe they're designing the simplest possible API for post-specialization. They might have been a lot more optimistic when they chose it than I am now about when Rust will get specialization. Or they might be heavily prioritizing API stability for years. Right now this interface is what I would call an attractive nuisance, leading to the problem in this blog post. What I would do for today is to have a method that explicitly takes a BufRead and uses it appropriately. And then perhaps a convenience method that explicitly wraps a Read in a BufRead. And the caller chooses. I'd deprecate the attractive nuisance interface for now (maybe reversing that later).

orlp 5y ago

Taking in a generic T: Read is the recommended way to accept streaming inputs. It is up to the user whether they want to buffer or not. You can pass in a BufReader into a T: Read no problem, and it will buffer any reads. That the reads internally are buffered is an implementation detail serde_json has no business knowing about. The converse is potentially disastrous - if you read too much from a TCP stream you could hang your program waiting for more input that never comes.

scottlamb 5y ago
> Taking in a generic T: Read is the recommended way to accept streaming inputs. Recommended by whom? When you want to process stuff in bulk, it's better to use BufRead::fill_buf (reusing the BufRead's buffer) rather than copy into your own. For example, I wrote some code which basically skips some escape bytes. [1] It wraps a BufRead and is itself a BufRead. Its caller actually can process bytes straight from the buffer of the BufRead you supply, skipping two layers of copying. > The converse is potentially disastrous - if you read too much from a TCP stream you could hang your program waiting for more input that never comes. You can implement any idea badly, but that doesn't mean the idea itself is bad. There's a difference between taking advantage of buffering and expecting to operate only on full, fixed-sized buffers. [1] https://github.com/dholroyd/h264-reader/blob/60ed66dc4dbfe745ceff80692902273b260f2ba9/src/rbsp.rs#L46 https://github.com/dholroyd/h264-reader/blob/60ed66dc4dbfe74...

sitta 5y ago

For what it's worth, if you want the best performance out of Serde you have to use from_str or from_slice, regardless of if you pass a BufRead to it. https://github.com/serde-rs/json/issues/160 https://github.com/serde-rs/json/issues/160

scottlamb 5y ago
In that issue, they're talking about specialization. With that language feature, they'd be able to make this interface optimal for either mode: * if the caller supplies a BufRead, they can use fill_buf. * if the caller supplies a Read that isn't BufRead, they can wrap it in a BufReader or the like, and then use fill_buf on that. Stable Rust doesn't have specialization today, so at most one of those can be handled optimally. (It looks like neither of them is handled well at all right now.) Maybe they're designing the simplest possible API for post-specialization. They might have been a lot more optimistic when they chose it than I am now about when Rust will get specialization. Or they might be heavily prioritizing API stability for years. Right now this interface is what I would call an attractive nuisance, leading to the problem in this blog post. What I would do for today is to have a method that explicitly takes a BufRead and uses it appropriately. And then perhaps a convenience method that explicitly wraps a Read in a BufRead. And the caller chooses. I'd deprecate the attractive nuisance interface for now (maybe reversing that later).