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.

Sign up for GitHub

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

Remove slice deque #30

Merged
merged 2 commits into from
Oct 3, 2022
Merged

Remove slice deque #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
236cc1c
Use circular instead of slice-deque
Sep 26, 2022
6b6ab1a
Forgot removing an unnecessary unsafe block
Sep 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ edition = "2021"
memchr = "2.2"
btoi = "0.4"
shakmaty = "0.22"
slice-deque = "0.3"
circular = "0.3"

[dev-dependencies]
crossbeam = "0.8"
Expand Down
51 changes: 15 additions & 36 deletions src/reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use shakmaty::{
san::{San, SanPlus, Suffix},
CastlingSide, Color, Outcome,
};
use slice_deque::SliceDeque;
// use slice_deque::SliceDeque;

use crate::{
types::{Nag, RawComment, RawHeader, Skip},
Expand Down Expand Up @@ -496,20 +496,20 @@ trait ReadPgn {
/// Internal read ahead buffer.
#[derive(Debug, Clone)]
pub struct Buffer {
inner: SliceDeque<u8>,
inner: circular::Buffer,
}

impl Buffer {
fn new() -> Buffer {
Buffer {
inner: SliceDeque::with_capacity(MIN_BUFFER_SIZE * 2),
inner: circular::Buffer::with_capacity(MIN_BUFFER_SIZE * 2),
}
}
}

impl AsRef<[u8]> for Buffer {
fn as_ref(&self) -> &[u8] {
self.inner.as_ref()
self.inner.data()
}
}

Expand Down Expand Up @@ -556,17 +556,6 @@ impl<R: Read> BufferedReader<R> {
buffer: Buffer::new(),
};

unsafe {
// Initialize the entire ring buffer, so that reading into the
// tail-head slice is always safe.
//
// Use https://doc.rust-lang.org/std/io/struct.Initializer.html
// once stabilized.
let uninitialized = reader.buffer.inner.tail_head_slice();
assert!(uninitialized.len() >= 2 * MIN_BUFFER_SIZE);
ptr::write_bytes(uninitialized.as_mut_ptr(), 0, uninitialized.len());
}

reader
}

Expand Down Expand Up @@ -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>> {
Copy link
Owner

@niklasf niklasf Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, cool.

while self.buffer.inner.len() < MIN_BUFFER_SIZE {
unsafe {
let size = {
// This is safe because we have initialized the entire
// buffer in the constructor.
let remainder = self.buffer.inner.tail_head_slice();
self.inner.read(remainder)?
};

if size == 0 {
break;
}
while self.buffer.inner.available_data() < MIN_BUFFER_SIZE {
let remainder = self.buffer.inner.space();
let size = self.inner.read(remainder)?;

self.buffer.inner.move_tail(size as isize);
if size == 0 {
break;
}

self.buffer.inner.fill(size);
}

Ok(self.buffer.inner.front().cloned())
Ok(self.buffer.inner.data().get(0).cloned())
}

fn invalid_data() -> io::Error {
io::Error::from(io::ErrorKind::InvalidData)
}

fn buffer(&self) -> &[u8] {
self.buffer.inner.as_slice()
self.buffer.inner.data()
}

fn consume(&mut self, bytes: usize) {
// This is unconditionally safe with a fully initialized buffer.
debug_assert!(bytes <= MIN_BUFFER_SIZE * 2);
unsafe {
self.buffer.inner.move_head(bytes as isize);
}
self.buffer.inner.consume(bytes);
}

fn peek(&self) -> Option<u8> {
self.buffer.inner.front().cloned()
self.buffer.inner.data().get(0).cloned()
}
}

Expand Down