Refactor to abstract storage engine and fix bugs #247
Conversation
zsluedem
force-pushed
the
abstract-storage
branch
from
87aa4e2
to
80ce240
Compare
zsluedem
force-pushed
the
abstract-storage
branch
from
6f8fe8c
to
2753871
Compare
| R: ReputationEntryOp, | ||
| SanCk: SanityCheck<M>, | ||
| SimCk: SimulationCheck, | ||
| SimTrCk: SimulationTraceCheck<M>, |
There was a problem hiding this comment.
@Vid201
The side affect of not introducing dynamic dispatch in rust -> we need to provide these traits in a lot of places.
There is some performance penalty for dynamic dispatch. I need to do some benchmark later to figure out whether dynamic dispatch is better for code reading later. I think we could live with the current codes right now before the benchmark is done.
There was a problem hiding this comment.
Got it.
yeah, the code readibility is not the best, but I think it's fine. We could add some comments for all these traits though.
| pub trait UserOperationCodeHashAct: UserOperationCodeHashOp + ClearOp + Send + Sync {} | ||
| impl<T> UserOperationCodeHashAct for T where T: UserOperationCodeHashOp + ClearOp + Send + Sync {} | ||
|
|
||
| pub struct Mempool<T, Y, X, Z> |
There was a problem hiding this comment.
The mempool here with seperate trait could help separate the lock with data racing.
There was a problem hiding this comment.
I'm just curious - does this pattern have a name? Just interested if this is some standardised approach
There was a problem hiding this comment.
I am actually sure what name should be used actually. This is like a common practice for trait system (mostly for function programming language not oop). The goal is to create minimize method and compose these methods to make a program.
https://scg.unibe.ch/research/traits maybe the lists here are helpful.
bin/silius/src/bundler.rs
Outdated
| ); | ||
| match (args.uopool_mode, args.use_memory) { |
There was a problem hiding this comment.
Possible to move this code to some other crate? match is fine here, but maybe the code for creating uopool can be in uopool crate
There was a problem hiding this comment.
This is the disadvantage of the refactoring. When things comes to be totally type concrete, the compiler would complain a lot about it which leads to the highly duplicate codes here. It is not easy to do that unless we introduce dynamic dispatch for Mempool , Reputation and validator.
I suggest we leave it here right now and I know it is ugly. We could optimize the codes here when I finished dynamic dispatch bench mark and it doesn't cost too much performance penalty. If the performance is an issue for dynamic dispatch, we could only introduce some macro to make codes more readible here.
| R: ReputationEntryOp, | ||
| SanCk: SanityCheck<M>, | ||
| SimCk: SimulationCheck, | ||
| SimTrCk: SimulationTraceCheck<M>, |
There was a problem hiding this comment.
Got it.
yeah, the code readibility is not the best, but I think it's fine. We could add some comments for all these traits though.
| pub trait UserOperationCodeHashAct: UserOperationCodeHashOp + ClearOp + Send + Sync {} | ||
| impl<T> UserOperationCodeHashAct for T where T: UserOperationCodeHashOp + ClearOp + Send + Sync {} | ||
|
|
||
| pub struct Mempool<T, Y, X, Z> |
There was a problem hiding this comment.
I'm just curious - does this pattern have a name? Just interested if this is some standardised approach
This is a very big pr for several issues