refactor: Revisit Scheduler's public API #4870
Conversation
Previous code marks all fields in the Scheduler structure as public, while at the same time expose enough function to build a debugger. However, by looking at the expose APIs and their docs, one cannot have a clear picture of how to interact with a Scheduler, if they don't read the whole implementation in full. This change aims at exposing enough public APIs of the Scheduler structure, enabling as much outside usage as possible, while at the same time also maintaining a sane view of the Scheduler structure.
|
cc @mohanson |
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
script/src/scheduler.rs:964
- [nitpick] Since boot_vm has been converted from a public API to an internal helper, consider renaming it (e.g., _boot_vm) to clearly indicate its internal usage.
fn boot_vm(&mut self, location: &DataLocation, args: VmArgs) -> Result<VmId, Error> {
script/src/scheduler.rs
Outdated
| @@ -138,13 +139,50 @@ where | |||
| } | |||
| } | |||
|
|
|||
| /// If current scheduler is terminated | |||
| pub fn terminated(&self) -> bool { | |||
| self.states[&ROOT_VM_ID] == VmState::Terminated | |||
There was a problem hiding this comment.
Direct indexing into self.states may panic if ROOT_VM_ID is unexpectedly missing. Consider using a safe accessor like get() with proper error handling.
| self.states[&ROOT_VM_ID] == VmState::Terminated | |
| match self.states.get(&ROOT_VM_ID) { | |
| Some(state) => *state == VmState::Terminated, | |
| None => false, // Return false if ROOT_VM_ID is missing | |
| } |
Copilot uses AI. Check for mistakes.
| }); | ||
| } | ||
|
|
||
| let (id, _) = self.iterate_outer(&Pause::new(), u64::MAX)?; |
There was a problem hiding this comment.
If self.terminated() is false, should we return the remaining_cycles from self.iterate_outer(&Pause::new(), u64::MAX)?; to exit_status?
Probably not, because IterationResult::exit_status.1 is intended to store the consumed_cycles, not the remaining_cycles?
There was a problem hiding this comment.
IterationResult::exit_status.1 should always contain consumed cycles up to the execution point.
script/src/types.rs
Outdated
| /// VM ID that gets executed | ||
| pub executed_vm: VmId, | ||
| /// Exit status | ||
| pub exit_status: Option<(i8, Cycle)>, |
There was a problem hiding this comment.
Suggest add code comment for Cycle:
pub exit_status: Option<(i8, /*! consumed_cyles */ Cycle)>,There was a problem hiding this comment.
I actually split a separate type to better indicate usage.
script/src/scheduler.rs
Outdated
| @@ -774,6 +835,16 @@ where | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn terminated_result(&mut self) -> Result<(i8, Cycle), Error> { | |||
| assert_eq!(self.states[&ROOT_VM_ID], VmState::Terminated); | |||
There was a problem hiding this comment.
How about reuse assert!(self.terminated()); and move this method to be adjacent to pub fn terminated(&self) -> bool since they are both related to termination.
|
The bug should be resolved by now. Feel free to review the updated PR. |
Previous code marks all fields in the Scheduler structure as public, while at the same time expose enough function to build a debugger. However, by looking at the expose APIs and their docs, one cannot have a clear picture of how to interact with a Scheduler, if they don't read the whole implementation in full.
This change aims at exposing enough public APIs of the Scheduler structure, enabling as much outside usage as possible, while at the same time also maintaining a sane view of the Scheduler structure.
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What is changed and how it works?
Proposal: xxx
What's Changed:
Related changes
owner/repo:Check List
Tests
Side effects
Release note