Remove slice deque #30
Conversation
This probably performs worse, but it removes platform dependent code and it removes all uses of unsafe (which makes me feel better). This is for the moment a straight forward replacement with the minimal changes necessary to make it compile, circular::Buffer has some methods that could be used to tune when the buffer is shifted. We might want to take a look at optimizing the size of the buffer as well.
|
Another benefit is that there no longer is any unsafe code (there is some in circular 0.3, I have a patch removing unsafety in circular as well, that patch doesn't seem to affect performance) |
| @@ -637,44 +626,34 @@ impl<R: Read> ReadPgn for BufferedReader<R> { | |||
| type Err = io::Error; | |||
|
|
|||
| fn fill_buffer_and_peek(&mut self) -> io::Result<Option<u8>> { | |||
There was a problem hiding this comment.
Correctness requires that this method definitely fills the buffer with at least MIN_BUFFER_SIZE bytes, or all bytes until the end of the file. Is this true with the new implementation?
There was a problem hiding this comment.
It loops until available_data in the buffer is MIN_BUFFER_SIZE and it only breaks out of the loop when it reads no more data, which is the same logic as was used with slice-deque, so yes that behaviour is unchanged.
To be honest when changing that function I was unsure why it needed to read to fill the buffer that far. It can't be for correctness in parsing as far as I understand it there is no reason to see that far ahead. And it's unnecessary for safety reasons (the buffer gets fully initialized at the very beginning and constantly reused, so there are never any poisonous undefined bytes that can get read).
There was a problem hiding this comment.
It needs this only in a one or two places, parsing headers and comments, so that the entire header/comment will be present in the slice at once. All other places could use a more relaxed "peek", if there's a performance difference.
Is it not possible for space() to be empty, and then read() being called on a 0-sized buffer and breaking out of the loop?
There was a problem hiding this comment.
Unless the consume_noshift variant of consume is used the buffer will be shifted when the position pointer of the buffer is over the halfway point in it's internal storage.
That means that available_data() + available_space() should always be more than half of the buffer size which I kept at MIN_BUFFER_SIZE * 2.
Unlike what the orignal commit says this performs better than slice-deque on my machine running on Linux (I'd assume the performance benefit is even larger on Windows). This becomes even more pronounced with the sozu-proxy/circular#8 pull request (the same effect can be reached using lto = "thin", at least on the stats example), slice-deque on my machine takes about 4 minutes to run stats on lichess_db_standard_rated_2018-10.pgn, circular 0.3 without inlining 3 minutes 30, circular with the inlining annotations or with lto = thin takes 1 minute 40 seconds (everything tested in release mode).