P2P is doing a lot of database lookups #2023
Assignees
Labels
good first issue
Good for newcomers
Comments
|
Without changing the structure of the db to store full blocks instead of compact blocks, the most practical improvement with minimal changes would be to change this to a multi-get operation. |
|
Taking this up :) |
9 tasks
rymnc
added a commit
that referenced
this issue
Aug 21, 2024
…iven height (#2112) ## Linked Issues/PRs <!-- List of related issues/PRs --> - #2023 ## Description <!-- List of detailed changes --> We don't need to fetch the header *and* the consensus header from the storage if either one of them is `None`. If `consensus` evaluates to `None`, we can early return a `None`, but if the consensus header exists, we can then fetch the block header. Performs another optimization in instances where we fetch the `SealedBlock` and only end up using the `entity` wrapped within it, and not the consensus headers. Previously, both the consensus headers and the block were fetched from the db and then returned as a `SealedBlock`, Now, we just check for existence of a given block height in the `SealedBlockConsensus` table, and avoid the allocation for the consensus headers. > note: also restricts how many blocks worth of transactions can be requested over p2p ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [x] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? --------- Co-authored-by: Green Baneling <[email protected]> Co-authored-by: Mårten Blankfors <[email protected]>
This was referenced Aug 28, 2024
Merged
rymnc
added a commit
that referenced
this issue
Aug 30, 2024
…2p req/res protocol (#2135) ## Linked Issues/PRs <!-- List of related issues/PRs --> - #2023 - #2112 ## Description <!-- List of detailed changes --> We bubble up the usage of a new function `log_metrics` to clean up how metrics are being logged in the p2p module. additionally, we also specify a `Gauge` for how many blocks worth of data has been requested, so we can re-use those numbers in our simulations to find the most optimal lookup method. ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [ ] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else?
rymnc
added a commit
that referenced
this issue
Sep 3, 2024
This PR introduces benchmarks for varied methods of database lookups for blocks and included transactions, namely - 1. `header_and_tx_lookup` 2. `full_block_lookup` 3. `multi_get_lookup` We can use these benchmarks to ascertain which form is best for this use-case, and then implement it. It is suggested to alter the bench matrix before running and using a beefy machine to do so. Run it with `cargo bench --bench db_lookup_times` ## Linked Issues/PRs <!-- List of related issues/PRs --> - #2023 ## Description <!-- List of detailed changes --> - moved the implementation of `KeyValueMutate` for rocksdb out of the test module. - each benchmark is located in `benches/benches/db_lookup_times.rs` - stores `FullFuelBlocks` along with the `CompressedBlocks` and `Transactions` table to not affect single header/tx lookup times in other usages. - implemented various traits for the `FullFuelBlocks` table - the [notion doc](https://www.notion.so/fuellabs/DB-Analysis-for-Lookups-f62aa0cd5cdf4c91b9575b8fb45683f7) has more information about the analysis we did. - each benchmark randomizes the block height that is queries *per iteration* to ensure randomness and fair comparison - rocksdb caching has been `disabled` for these benchmarks ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [x] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? --------- Co-authored-by: Mitchell Turner <[email protected]>
9 tasks
rymnc
added a commit
that referenced
this issue
Sep 9, 2024
>[!NOTE] > This PR follows up #2142 with comments after merge ## Linked Issues/PRs <!-- List of related issues/PRs --> - #2142 - #2023 ## Description <!-- List of detailed changes --> - refactors the test database to use a custom set of columns that will not create any new tables/columns in `fuel-core` - refactors the functions to return `Result`s instead of unwrapping everywhere - automated cleaning up of benchmark databases - `KeyValueMutate` impl of `RocksDb` is now behind the `test-helpers` feature flag - moved the `db_lookup_times_utils` to the `src` directory in the benches crate so we can test our insertion/fetching logic ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [ ] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? --------- Co-authored-by: Green Baneling <[email protected]>
|
This is going to be partially solved with the caching layer in #2352 |
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
When we request transactions for the block, each transaction requires a separate lookup, which is very expensive, considering that transactions are randomly distributed throughout the database(because of randomness of
TxId).The text was updated successfully, but these errors were encountered: