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

Don't always seek if a .shx was given #36

Merged
merged 1 commit into from
May 12, 2024
Merged

Conversation

tmontaigu
Copy link
Owner

If a .shx file was given or found we would always use it to seek to the position it contained.

However calling seek on a BufRead discards the buffer even if we seek to the position is at leading to a performance loss due to I/O.

So we now only seek if we need to

If a .shx file was given or found we would always use it to seek
to the position it contained.

However calling seek on a BufRead discards the buffer even if we seek
to the position is at leading to a performance loss due to I/O.

So we now only seek if we need to
@tmontaigu tmontaigu merged commit a27a93e into master May 12, 2024
8 checks passed
@tmontaigu tmontaigu deleted the dont-always-seek branch May 13, 2024 09:02
@@ -136,10 +136,12 @@ impl<'a, T: Read + Seek, S: ReadableShape> Iterator for ShapeIterator<'a, T, S>
// as some shapes may not be stored sequentially and may contain 'garbage'
// bytes between them
let start_pos = shapes_indices.next()?.offset * 2;
if let Err(err) = self.source.seek(SeekFrom::Start(start_pos as u64)) {
return Some(Err(err.into()));
if start_pos != self.current_pos as i32 {
Copy link

@michaelkirk michaelkirk May 14, 2024

Choose a reason for hiding this comment

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

Maybe you already saw this @tmontaigu but from the impl Seek for BufReader docs:

/// Seeking always discards the internal buffer, even if the seek position
/// would otherwise fall within it. This guarantees that calling
/// [`BufReader::into_inner()`] immediately after a seek yields the underlying reader
/// at the same position.
///
/// To seek without discarding the internal buffer, use [`BufReader::seek_relative`].

So with the new approach, you'll still discard the buffer whenever there is any non-zero seek.

Something like this might be simpler (and faster in the case of a seek within the existing buffer):

self.source.seek_relative(start_pos - self.current_pos as isize)?
self.current_pos = start_pos;

Not sure if it'd matter much in practice though.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Its true that using seek_relative would be better, but adding that small if was a faster fix

Since this the type of self.source is T: Seek + Read rather than always BufRead, we can't call seek_relative.

Well we actually can but the seek_relative method of the Seek trait is behind nightly as its not stable, se we would either need to wait for it to be stable or make our own SeekRelative trait or use cfg! to call seek_relative when detecting nightly as I dont want to force the use of nightly

Choose a reason for hiding this comment

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

Thanks for the explanation. Sorry for the distraction.

Copy link
Owner Author

Choose a reason for hiding this comment

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

You don't need to apologize 😅 , the comment is valid, and we should probably one of the thing above to handle it

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

Successfully merging this pull request may close these issues.

2 participants