Skip to content

Mpt with rocksdb #1

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

Open
wants to merge 17 commits into
base: multi-vm-experiment
Choose a base branch
from
Open

Conversation

PsychoPunkSage
Copy link

This PR is related to experiment-2 in Multi-VM-Experiment

Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
@vpanchal-supra vpanchal-supra self-requested a review April 21, 2025 06:58
@@ -0,0 +1,175 @@
use reth_db_api::{table::Table, DatabaseError};

Choose a reason for hiding this comment

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

This file seems stale because we are not using any content of this file anywhere

@@ -0,0 +1,222 @@
use metrics::{Counter, Gauge, Histogram};

Choose a reason for hiding this comment

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

This also seems stale

/// Write options
write_opts: WriteOptions,
/// Marker for transaction type
_marker: PhantomData<bool>,

Choose a reason for hiding this comment

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

Whats the use-case?

}

/// Get the column family handle for a table
fn get_cf<T: Table>(&self) -> Result<CFPtr, DatabaseError> {

Choose a reason for hiding this comment

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

Why does this function returns raw pointer of cf, instead we can also reference

Copy link
Author

Choose a reason for hiding this comment

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

I don't remember why I did that... but is was related to internal impl of ColumnFamily which prevents mutable references ig (not remember exactly). Initially I was using references... but later on had to revert to this..

Choose a reason for hiding this comment

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

As this function in used in that places where the reference of cf is created from raw pointer, so I think we can simply replace raw pointer with reference.

RocksTrieCursorFactory::new(Box::leak(tx))
}

pub fn hashed_cursor_factory(&self) -> RocksHashedCursorFactory<'_>

Choose a reason for hiding this comment

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

As this function is only expect to be invoked for read modded RocksTransaction so it's would be good to only implement this function for true value of WRITE

// Convert the raw pointer back to a reference safely
// This is safe as long as the DB is alive, which it is in this context
let cf_ptr = self.get_cf::<T>()?;
let cf = unsafe { &*cf_ptr };

Choose a reason for hiding this comment

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

I think this unsafe code can be easily replaced with safe code.

Copy link
Author

Choose a reason for hiding this comment

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

Is there any safe way to deal with Raw-pointers? Aren't they come other unsafe section of Rust?

Choose a reason for hiding this comment

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

Instead of using raw pointer we can use simple reference

Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
@PsychoPunkSage
Copy link
Author

@vpanchal-supra
Made all the necessary changes... please have a look at it

Signed-off-by: Abhinav Prakash <[email protected]>
Signed-off-by: Abhinav Prakash <[email protected]>
@PsychoPunkSage
Copy link
Author

@vpanchal-supra I have started some review regarding the error. Please have a look at it.

Thank You

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.

2 participants