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

Optimize importer-offline (batch writing + more parallelism) #1168

Closed
wants to merge 1 commit into from

Conversation

marcospb19-cw
Copy link
Contributor

@marcospb19-cw marcospb19-cw commented Jun 19, 2024

This PR divides importer-offline's task block-importer into block-executor and block-saver, to separate EVM and storage writing, now both can run in parallel

Besides that, this enabled our RocksDB storage to perform block writes in batches, as a consequence, this leaves execution time as the new bottleneck for importer-offline, which hints that we don't need to optimize the storage further to improve this binary.

Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5] 4
🧪 Relevant tests No
🔒 Security concerns No
⚡ Key issues to review Logging and Monitoring:
Ensure that all background tasks such as 'storage-loader', 'block-executor', and 'block-saver' are properly logged and monitored. The current implementation logs errors, but it may benefit from more detailed logging at each significant step, especially given the parallel execution context.
Error Handling:
The use of .unwrap() in run_external_block_executor when sending block_batch to writer_batch_tx could lead to panics if the channel is closed or full. It's recommended to handle this error gracefully.
Cloning Optimization:
In src/eth/primitives/pending_block.rs, the method split_transactions clones each transaction. If possible, consider optimizing to avoid cloning, especially if the transactions are large or this operation is called frequently.
Tracing Fields:
The commented-out spans in StratusStorage methods like read_account and read_slot were removed to potentially optimize performance. If tracing is still required, consider adding lightweight tracing or logging specific, necessary details only.
Thread Safety and Concurrency:
The use of synchronous channels (mpsc::sync_channel) and their interaction in a multi-threaded environment should be carefully reviewed to ensure there are no deadlocks or excessive blocking, especially since the channel capacities are quite small.

Copy link

Failed to generate code suggestions for PR

@marcospb19-cw marcospb19-cw force-pushed the optimize-importer-offline branch 2 times, most recently from 183ff0b to 89f4da0 Compare June 19, 2024 16:20
@marcospb19-cw marcospb19-cw force-pushed the optimize-importer-offline branch 2 times, most recently from 09a0b3f to b7cf84f Compare July 2, 2024 15:32
@marcospb19-cw marcospb19-cw changed the title run importer-offline in parallel (and optimize) Optimize importer-offline (batch writing + more parallelism) Jul 2, 2024
@marcospb19-cw marcospb19-cw force-pushed the optimize-importer-offline branch from b7cf84f to 9f3eabc Compare July 2, 2024 17:18
///
/// REMOVE THIS: Decrease by one to give room for the pending block.
/// TODO: why `max/2-1` doesn't work?
const STORAGE_WRITER_BATCHES_SIZE: usize = INMEMORY_TEMPORARY_STORAGE_MAX_BLOCKS / 3;
Copy link
Contributor Author

@marcospb19-cw marcospb19-cw Jul 8, 2024

Choose a reason for hiding this comment

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

Auto-review requested changes: update these comments and quickly check if it's obvious why it behaves this way.

@marcospb19-cw marcospb19-cw marked this pull request as ready for review August 5, 2024 20:10
This commit divides importer-offline's task block-importer into
block-executor and block-saver, to separate EVM and storage writing,
now both can run in parallel

Besides that, this enabled our RocksDB storage to perform block writes
in batches, as a consequence, this leaves execution time as the new
bottleneck for importer-offline, which hints that we don't need to
optimize the storage further to improve this binary.
@marcospb19-cw marcospb19-cw force-pushed the optimize-importer-offline branch from 9f3eabc to 621c7a2 Compare August 5, 2024 20:17
@marcospb19-cw marcospb19-cw marked this pull request as draft August 5, 2024 20:18
@marcospb19-cw
Copy link
Contributor Author

Recent smaller changes achieved enough speed improvements in importer-offline, it's unclear now if we need to further improve it or not, closing for now.

@marcospb19-cw marcospb19-cw deleted the optimize-importer-offline branch August 20, 2024 21:43
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.

1 participant