-
Notifications
You must be signed in to change notification settings - Fork 25
Add support for caching subtrees to reduce disk io when reading sectors #882
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
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
890c4c9 to
d3cc9ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for caching subtree Merkle roots in the database to optimize disk I/O when reading partial sectors. By storing precalculated subtree roots, the system can read only the required segment of a sector instead of the entire sector when generating Merkle proofs, significantly reducing disk I/O operations.
Key changes:
- Adds
cached_subtree_rootscolumn tostored_sectorstable for storing Merkle subtree roots - Modifies
ReadSectorto return partial sector data with Merkle proofs using cached subtrees - Updates contract storage proof generation to use the new optimized sector reading
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| persist/sqlite/init.sql | Adds cached_subtree_roots BLOB column to stored_sectors table |
| persist/sqlite/migrations.go | Adds migration v46 to add cached_subtree_roots column |
| persist/sqlite/sectors.go | Implements CacheSubtrees and SectorMetadata methods for persistence |
| host/storage/persist.go | Defines interface for CacheSubtrees and SectorMetadata methods |
| host/storage/storage.go | Implements optimized sector reading with subtree caching logic |
| host/storage/volume.go | Modifies ReadSector to support partial reads with offset and length |
| host/contracts/update.go | Updates storage proof building to use new ReadSector signature |
| host/contracts/manager.go | Updates StorageManager interface with new ReadSector signature |
| api/api.go | Updates interface definition and removes unused import |
| api/volumes.go | Updates sector verification to use new ReadSector signature |
| host/storage/storage_test.go | Updates tests to use new ReadSector signature, removes obsolete TestSectorCache |
| internal/integration/rhp/v4/rhp4_test.go | Improves error checking and adds logging |
| go.mod, go.sum | Updates dependencies to core v0.19.0 and coreutils v0.19.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1eaa738 to
197941b
Compare
197941b to
47c97ce
Compare
peterjan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LRU cache on the volume manager seems completely redundant now?
Requires SiaFoundation/core#374 and SiaFoundation/coreutils#367