Speed up seriously slowed down sled reservation #9545
Conversation
The 3/4 local storage PR added the ability to allocate local storage during sled reservation. This works but computing the entire set of all possible allocations is expensive. This PR wraps the logic into an Iterator, and yields all valid allocations one at a time.
|
|
||
| while let Some(possible_allocation) = self.queue.pop() { | ||
| let PossibleAllocations { | ||
| allocations, | ||
| candidates_left, | ||
| request_index, | ||
| } = possible_allocation; |
There was a problem hiding this comment.
feel free to ignore me but, this could also now be
| while let Some(possible_allocation) = self.queue.pop() { | |
| let PossibleAllocations { | |
| allocations, | |
| candidates_left, | |
| request_index, | |
| } = possible_allocation; | |
| loop { | |
| let PossibleAllocations { | |
| allocations, | |
| candidates_left, | |
| request_index, | |
| } = self.queue.pop()?; |
which...is a different way of saying the same thing that is probably neither worse nor better in any kind of way
| #[derive(Clone, Debug)] | ||
| struct PossibleAllocationsForRequest<'a> { | ||
| request: &'a LocalStorageDisk, | ||
| candidate_datasets: HashSet<CandidateDataset>, | ||
| } | ||
|
|
||
| struct PossibleAllocations { | ||
| allocations: Vec<LocalStorageAllocation>, | ||
| candidates_left: HashSet<CandidateDataset>, | ||
| request_index: usize, | ||
| } |
There was a problem hiding this comment.
it might be nice for these to have some commentary explaining what the difference between them is?
There was a problem hiding this comment.
In edc0b56 I added comments and changed the types to distinguish between an incomplete list of allocations, and a completed one. I think this makes more sense to read, let me know what you think
| allocations_to_perform: Vec<PossibleAllocationsForRequest<'a>>, | ||
| queue: BinaryHeap<PossibleAllocations>, |
There was a problem hiding this comment.
it would be nice if there was some commentary explaining what these fields mean...
| // | ||
| // Start from no allocations made yet, a list of allocations to perform | ||
| // (stored in `requests`), and all of the available zpools on the sled. | ||
| // Use a BinaryHeap to get a sorted list. |
There was a problem hiding this comment.
maybe worth saying "sorted by the number of possible allocations", since otherwise, you have to go and track down the Ord impl to figure that out?
There was a problem hiding this comment.
I removed this line in edc0b56, I think it reads better.
| sled_target: SledUuid, | ||
| zpools_for_sled: NonEmpty<ZpoolGetForSledReservationResult>, | ||
| local_storage_disks: &'a [LocalStorageDisk], | ||
| ) -> Result<Self, ()> { |
There was a problem hiding this comment.
take it or leave it: this maybe could just be an Option? It's not an error, it's just...there are no valid allocations here`.
There was a problem hiding this comment.
Definitely that makes sense, done in edc0b56
3 participants
The 3/4 local storage PR added the ability to allocate local storage during sled reservation. This works but computing the entire set of all possible allocations is expensive. This PR wraps the logic into an Iterator, and yields all valid allocations one at a time.