feat: EXC-1754 Evict sandboxes based on their priorities #1967
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adambratschikaye
approved these changes
Oct 15, 2024
Contributor
adambratschikaye
left a comment
adambratschikaye
left a comment
There was a problem hiding this comment.
Thanks! Just left some minor comments.
| let mut evicted = candidates; | ||
|
|
||
| for candidate in candidates.into_iter() { | ||
| for candidate in remaining_candidates.into_iter() { |
Contributor
There was a problem hiding this comment.
I guess now we could be iterating through all the canisters in more cases (if we don't hit evict_at_most). I guess that's fine since the list is short and this only runs every 10 seconds or so - agree?
Contributor
There was a problem hiding this comment.
I think we might need to change the 10s logic to something smaller, but I'm not super sure yet, let's keep this under discussion.
Comment on lines
77
to
81
| use std::time::{Duration, Instant}; | ||
|
|
||
| use ic_test_utilities_types::ids::canister_test_id; | ||
| use ic_types::AccumulatedPriority; | ||
|
|
Contributor
There was a problem hiding this comment.
Could we add some tests that actually depend on the new logic?
| min_active_sandboxes: usize, | ||
| max_active_sandboxes: usize, | ||
| max_sandbox_idle_time: Duration, | ||
| state_reader: Arc<dyn StateReader<State = ReplicatedState>>, |
Contributor
There was a problem hiding this comment.
I think this argument could just be a reference instead of an owned Arc. Then you won't need to Arc::clone each time you call it.
|
|
||
| fn get_latest_state(&self) -> Labeled<Arc<ReplicatedState>>; | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
accidental newline?
dragoljub-djuric
changed the base branch from
master
to
dimitris/scheduler-changes-sandbox-count
dragoljub-djuric
changed the base branch from
dimitris/scheduler-changes-sandbox-count
to
master
dragoljub-djuric
changed the base branch from
master
to
dimitris/scheduler-changes-sandbox-count
dragoljub-djuric
changed the base branch from
dimitris/scheduler-changes-sandbox-count
to
master
dragoljub-djuric
changed the base branch from
master
to
dimitris/scheduler-changes-sandbox-count
dragoljub-djuric
changed the base branch from
dimitris/scheduler-changes-sandbox-count
to
master
dragoljub-djuric
changed the base branch from
master
to
Add_new_dashboards_to_testnet
berestovskyy
changed the title
No functional changes, just moving functions in one place.
This change is functionally equivalent but necessary for the upcoming priority-based eviction.
dragoljub-djuric
changed the base branch from
master
to
andriy/exc-1754-apply-priority-at-the-end
berestovskyy
force-pushed
the
andriy/exc-1754-apply-priority-at-the-end
branch
from
9142bae to
ab95547
Compare
Base automatically changed from
andriy/exc-1754-apply-priority-at-the-end
to
master
November 6, 2024 18:25
dragoljub-djuric
changed the base branch from
master
to
andriy/exc-1787-scheduler-divergence-debug
dragoljub-djuric
changed the base branch from
andriy/exc-1787-scheduler-divergence-debug
to
master
dragoljub-djuric
changed the base branch from
master
to
andriy/exc-1787-scheduler-divergence-debug
dragoljub-djuric
changed the base branch from
andriy/exc-1787-scheduler-divergence-debug
to
master
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the canister is executed, it is most likely its Sandbox will not be evicted from the cache, since we are keeping Sandboxes based using LRU logic. At the same time, the Scheduler decreases its priority so it is less likely that it will be executed. So our caching of Sandbox processes is almost the worst possible.
Solution:
Propagate Scheduler priorities for the place we evict Sandbox processes and do evictions based on the lowest priority. That change should decrease the number of cache misses.
Link to follow-up with minimized number of reads to scheduler priorities.
Note:
Furthermore: the scheduler priorities used are from the round before the current round, because the snapshots are saved only at the end of the round, and
apply_scheduling_strategy()is run before executing canisters in the round. But that should not influence results by a lot.It remains to explore if there is an easy way to move
apply_scheduling_strategy()after all canisters are executed in the round. In that case, priorities taken from the last snapshot will be exactly the priorities for the current round.