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.

Sign up for GitHub

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

Jump to bottom

feat: add OnDiskStorage #246

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

feat: add OnDiskStorage #246

wants to merge 18 commits into from

Conversation

kien-rise
Copy link
Contributor

@kien-rise kien-rise commented Aug 5, 2024 •
edited
Loading

Usage

cargo bench

Benchmark result

# OnDiskStorage
Average: x2.06
Max: x4.37
Min: x0.94

# InMemoryStorage
Average: x1.9
Max: x4.13
Min: x0.87

@kien-rise kien-rise force-pushed the 1636-on-disk branch 2 times, most recently from a3df4b4 to ec8ee89 Compare August 5, 2024 11:49
@kien-rise kien-rise requested a review from hai-rise August 5, 2024 11:53
@kien-rise kien-rise marked this pull request as ready for review August 5, 2024 11:53
kien-rise
kien-rise commented Aug 5, 2024
View reviewed changes
examples/to_mdbx.rs Outdated Show resolved Hide resolved
@kien-rise kien-rise force-pushed the 1636-on-disk branch 2 times, most recently from c2a00cc to bdc0491 Compare August 6, 2024 15:26
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
examples/to_mdbx.rs Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
examples/to_mdbx.rs Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
examples/to_mdbx.rs Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
examples/to_mdbx.rs Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
examples/to_mdbx.rs Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
examples/to_mdbx.rs Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
examples/run_on_mdbx.rs Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
src/storage/on_disk.rs Outdated
&self,
address: &alloy_primitives::Address,
) -> Result<Option<super::AccountBasic>, Self::Error> {
let tx = self.inner.begin_ro_txn()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Can we reuse the same read-only transaction (pool) instead of creating a new one on each read?
  2. Is it more performant to run concurrent transactions in separate threads for parallel mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are for performance optimization only, right? Can we postpone to another PR?

At least we should measure the current performance first. Otherwise, these optimizations look premature to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should benchmark and optimize right from this PR, so we don't have to check in and maintain unwanted code if it fails to demonstrate great speedup over in-memory storage. We can break the deduplicating bytecodes task down because each part is an instant improvement we want (to maintain).

These specific questions are important to improve our understanding. So that if on-disk storage shows promise but MDBX flops then hopefully we know why and seek a better alternative, like a more async I/O-friendly database and schema to avoid locks among worker threads.

So yeah, let's add a benchmark and optimize from there.

hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
src/storage/on_disk.rs Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
src/storage/on_disk.rs Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
src/storage/on_disk.rs Outdated Show resolved Hide resolved
hai-rise
hai-rise reviewed Aug 7, 2024
View reviewed changes
src/storage/on_disk.rs Outdated
) -> Result<Option<super::AccountBasic>, Self::Error> {
let tx = self.inner.begin_ro_txn()?;
let Some(balance) = tx
.open_table(Some("balance"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it slow to call open_table on every read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a benchmark to answer this question.

@kien-rise kien-rise marked this pull request as draft August 8, 2024 15:12
hai-rise
hai-rise reviewed Aug 9, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@kien-rise kien-rise marked this pull request as ready for review August 12, 2024 03:07
@kien-rise kien-rise marked this pull request as draft August 13, 2024 05:15
@kien-rise kien-rise force-pushed the 1636-on-disk branch 5 times, most recently from 8c8c4dd to e721b87 Compare August 17, 2024 17:50
@kien-rise kien-rise force-pushed the 1636-on-disk branch 4 times, most recently from c3fabfc to 176192b Compare August 19, 2024 06:44
@kien-rise kien-rise force-pushed the 1636-on-disk branch 2 times, most recently from 638251a to ed9448b Compare August 19, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hai-rise hai-rise Awaiting requested review from hai-rise

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kien-rise @hai-rise