-
Notifications
You must be signed in to change notification settings - Fork 20
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
[feat] Implement Stateful for VmChipComplex #1211
Conversation
51eace6
to
a7fef1a
Compare
This comment has been minimized.
This comment has been minimized.
Self(Arc::new(VariableRangeCheckerChip::new(bus))) | ||
} | ||
|
||
pub fn bus(&self) -> VariableRangeCheckerBus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we implement deref https://doc.rust-lang.org/std/ops/trait.Deref.html from SharedVariableRangeCheckerChip to Arc, can we avoid all these self.0.xx
functions?
@@ -489,6 +534,30 @@ impl<F: PrimeField32> SystemBase<F> { | |||
} | |||
} | |||
|
|||
impl<F: PrimeField32> Stateful<SystemBaseState<F>> for SystemBase<F> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
semi-related but not directly for this PR: shouldn't trait Stateful
specify the state S should be Serialize + Deserialize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the trait-level, It seems not a must because Stateful
can be used in memory(maybe?).
crates/vm/src/arch/extensions.rs
Outdated
} | ||
|
||
fn store_state(&self) -> VmInventoryState { | ||
// TODO: parallelize this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's stopping you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some implementations of Executor
/Periphery
are not Send
+ Sync
due to RefCell
/Box
. I think we can address this if this is a bottleneck.
@@ -431,6 +463,12 @@ pub struct VmChipComplex<F: PrimeField32, E, P> { | |||
bus_idx_max: usize, | |||
} | |||
|
|||
#[derive(Clone, Serialize, Deserialize)] | |||
pub struct VmChipComplexState<F> { | |||
base: SystemBaseState<F>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just a matter of where you invoke bitcode, but wouldn't it match the other chip's pattern more if you bitcode deserialized to SystemBaseState
without load state? otherwise you will need to specify this state in protobuf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intend to avoid unnecessary serialization because bitcode
is not zero-copy. The existing implementation only serialize one more time when sending the struct. I wanted to mention this but didn't find a good place for comments.
@@ -694,6 +694,13 @@ impl<F: PrimeField32> MemoryController<F> { | |||
pub fn offline_memory(&self) -> Arc<Mutex<OfflineMemory<F>>> { | |||
self.offline_memory.clone() | |||
} | |||
pub fn get_memory_logs(&self) -> Vec<MemoryLogEntry<F>> { | |||
// TODO: can we avoid clone? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could make a separate take_memory_logs
fn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but store_state(&self)
so I cannot take..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's safe to change the function signature to store_state(self)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh why is it not store_state(&mut self)
: some sync thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after addressing @luffykai 's comments
This comment has been minimized.
This comment has been minimized.
3d63ce9
to
1b0f937
Compare
This comment has been minimized.
This comment has been minimized.
Commit: 3242120 |
1st commit:
Stateful
for system chips.SharedVariableRangeCheckerChip
.2st commit:
Stateful
forVmChipComplex
.ExecutionSegment::new_for_proving
which constructs aExecutionSegment
just for proving.ExecutionSegment::store_chip_complex_state
to store states.Note: some memory states need to clone. We should to try to avoid it in the future.
closes INT-2951