Improve performance of bdk_file_store::EntryIter
#1270
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Wrap file reader with `BufReader`. This reduces calls to `read`. * Wrap file reader with `CountingReader`. This counts the bytes read by the underlying reader. We can rewind without seeking first.
This test simulates a situation where the last write to the db is short. Aggregating the changeset after reopening the file should return an error (which includes a partially-aggregated changeset) containing an aggregation of changesets that were fully written. At this point, the test re-writes the final changeset (and this time it successfully writes in full). The file should be recoverable with all changesets, including the last one.
5 tasks
LLFourn
reviewed
Jan 24, 2024
crates/file_store/src/entry_iter.rs
Outdated
| // This syncs the underlying file's offset with the buffer's position. This way, we | ||
| // maintain the correct position to start the next read/write. | ||
| #[allow(clippy::seek_from_current)] | ||
| let _ = self.db_file.seek(io::SeekFrom::Current(0)); |
There was a problem hiding this comment.
Can you change this so we instead just seek the underlying file to the stream_position that the BufReader returns (by recording it and then into_inner or whatever to get the file). This is more explicit about what we're trying to do and doesn't have clippy ignores 🙈.
Something feels off about this. It we be cool if we could get rid of the implicit state being stored in the file position and make the user explicitly start writing at a certain position that we've advised instead.
There was a problem hiding this comment.
@LLFourn How is this?
if let Ok(pos) = self.db_file.stream_position() {
let _ = self.db_file.get_mut().seek(io::SeekFrom::Start(pos));
}Can we do explicit position writes in a separate PR? I think it is out of scope for this PR (+ I would like to use these changes here).
Because we use wrap the file with `BufReader` with the `EntryIter`, we need to sync the `BufReader`'s position with the file's offset when we drop the `EntryIter`. Therefore we have a custom drop impl for `EntryIter`.
evanlinjin
force-pushed
the
file_store_performance
branch
from
d6c5287 to
51bd01b
Compare
LLFourn
approved these changes
Jan 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
EntryIterperformance is improved by reducing syscalls. The underlying file reader is wrapped withBufReader(to reduce calls toreadandseek).Two new tests are introduced. One ensures correct behavior when the last changeset write is too short. The other ensures the next write position is correct after a short read.
Notes to the reviewers
This is extracted from #1172 as suggested by #1172 (review).
Changelog notice
Changed
EntryIterperformance is improved by reducing syscalls.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: