Do not pre-allocate memory on streams #1682
Conversation
|
I have tested this in corrosion and it significantly reduced memory usage. As far as I can tell everything is working as intended but I'd like somebody to thoroughly review it. |
There was a problem hiding this comment.
Thanks, this looks like a nice improvement to memory use!
The rule of thumb should be that a stream becomes allocated (i.e. Some) as soon as any frames for it are received, or the user manipulates it. This follows from the need to have somewhere to store the information carried in those frames/manipulations.
| // the line. Best to short-circuit. | ||
| let stream = entry.get_mut().as_mut().unwrap(); | ||
| let stream = match entry.get_mut().as_mut() { | ||
| Some(s) => s, |
There was a problem hiding this comment.
nit: this would be a great case or let-else.
| @@ -797,24 +832,26 @@ impl StreamsState { | |||
| expanded | |||
| } | |||
|
|
|||
| pub(super) fn max_send_data(&self, id: &StreamId) -> VarInt { | |||
There was a problem hiding this comment.
It would be nice to squash all the commits and then extract this as a separate prefix commit.
| let stream = self | ||
| .state | ||
| .send | ||
| .get_mut(&self.id) | ||
| .map(|s| s.get_or_insert_with(|| Box::new(Send::new(max_send_data)))) |
There was a problem hiding this comment.
I feel like there are a few recurring patterns here that would make good candidates for moving into a method somewhere to raise the level of abstraction. We could address that after merging this, but would be great if you can take a whack at it.
| @@ -216,7 +216,7 @@ impl<'a> SendStream<'a> { | |||
| .state | |||
| .send | |||
| .get_mut(&self.id) | |||
| .map(|s| s.get_or_insert_with(|| Box::new(Send::new(max_send_data)))) | |||
| .map(get_or_insert_send(max_send_data)) | |||
There was a problem hiding this comment.
Was actually hoping for something that can also abstract over the self.state.send.get_mut() of it all.
|
Shall we merge or are we waiting for a commit history cleanup from me? :) |
|
I have a cleaned up branch but was failing to push it up to your remote... let me just push it here. |
|
Made a PR for the forward port: #1686 |
This PR wraps
RecvandSendstreams inOption<Box<T>>to prevent significant memory pre-allocation.Still a few things to verify / do:
Nonevalue when it should beSomeNoneor aSomeexactly...(Hopefully) Fixes #1677