performance pass - #121
Conversation
|
added the benchmarking stuff to see how much this improves stuff and uh. wow.
|
0910116 to
4d14227
Compare
|
the biggest thing is that the heartbeat resource calc changed from O(VMs) to O(1) which is massive |
TheHeroBrine422
left a comment
TheHeroBrine422
left a comment
There was a problem hiding this comment.
Overall I like the ideas, but there are minor issues.
On top of this, scheduler_actor.rs is getting really hard to follow. We need better documentation and/or to split the module into several parts.
@korewaChino should likely review the ch_driver changes since I am not super familar with that section of the codebase, although it looks fine to me.
| // This could likely be done pretty easily by having two get data messages. | ||
| // Option 1: One that creates a session and sends the full data and then only sends diffs after that. | ||
| // Option 2: we can have one that sends full data, and then another that only sends data that changes. | ||
| // Option 2: One that sends full data, and then another that only sends data that changes. |
There was a problem hiding this comment.
These todo comments should be removed since this PR does this todo.
| .then_some(*vmid) | ||
| })) | ||
| .collect() | ||
| ) -> Vec<Ulid> { |
There was a problem hiding this comment.
This is likely slower then the old implementation?
While the observed slice would have all the vmids, the placement map also has pretty much all the vmids.
So now we have to do an O(n^2) operation every time we run this function, since we do a linear search over every vmid on the agent?
If the placements map was small, then this might be better, but it should be just as big as the observed slice.
The previous solution was O(n) since while we still have to add every placement VM, checking the deduplication is O(1) with a hash set.
| } | ||
| } | ||
|
|
||
| fn reconcile_agent_delta( |
There was a problem hiding this comment.
the placements hashmap is supposed to be the desired state of the scheduler. If a vm is removed from an agent, we likely should be looking at the placement map to reschedule the agent. not just removing it from the placement map like it doesnt exist.
There should probably be doc comments on the various scheduler data structures to mention this and just generally what the intent of the variables is.
| @@ -529,13 +612,33 @@ impl SchedulerActor { | |||
| /// - the cache likely needs to be updated automatically when a new vm is scheduled for info like used resources, because otherwise we have to deal with latency on that data we are using | |||
| /// and then if someone tries to schedule lets say 10 VMs in a batch, we could end up scheduling them all to the same agent because the metadata hasn't updated. | |||
| /// - there are a few solutions for this but they all kinda suck, mostly due to also making sure we deal with latency properly. I am ignoring the issue for now. | |||
There was a problem hiding this comment.
this comment is now on the wrong function, and is stale.
2 participants
i just wanted to do a larger pass of everything performance-wise since small improvements add up in a system like this
Stack created with GitHub Stacks CLI • Give Feedback 💬