Add and use over-allocation metric to decide when to defragment

[geieredgar]

Previous discussion: #996

[Ralith]

Another approach might be to keep a separate table of allocations, and map chunks to allocations. That should allow us to track memory overhead precisely by comparing buffered data to net allocation size, but is substantially more complex, so I'm not sure it'd be worthwhile.

[geieredgar]

Rebased my changes on current main.

[djc]

This implementation looks solid to me! Some minor issues below:

[djc]

@Matthias247 would you have a chance to review the current iteration of these changes? (Otherwise we'll probably just merge them; we can always improve on them later, of course.)

[Matthias247]

Will definitely take a look. However it's a bit of a busy week at the moment, so it might be start of next week.

[Matthias247]

The buffered/allocated strategy in the chunks, and taking the defragment decision based on it seem good to me.

However I think defragment migh have a bug and some wrong assumptions (size >= allocation_size) which don't make it as efficient as it could be.

While efficiency can further be improved in follow-ups, the data ordering bug should be checked and fixed.

[Matthias247]

One more general realization I had: The old mechanism covered the case where a client sent lots of overlapping ranges, and thereby caused excessive memory usage despite having a limited receive buffer. This change as it is won't do that anymore, since all chunks could still have good utilization.

Nevertheless I think the current approach is good too.

Maybe deduplication should happen on insert. But that actually means having to scan all fragments for duplicate ranges, and organizing the buffer more in a form of Vec<(Buffer, Range)> than a heap. Also not perfect in case there are lots of fragments.

[djc]

One more general realization I had: The old mechanism covered the case where a client sent lots of overlapping ranges, and thereby caused excessive memory usage despite having a limited receive buffer. This change as it is won't do that anymore, since all chunks could still have good utilization.

Hmm, that definitely seems like an issue. I think we should still cover that scenario.

[geieredgar]

I added two new commits. The first fixes #1009 by adding a deduplicate function that is called when switching from ordered to unordered mode. The second commit adds a end field to Assembler and uses min(self.buffered, self.end - self.bytes_read) instead of only self.buffered for the calculation of over_allocationand threshold. Therefore, if we have a lot of duplicate bytes buffered, deduplicate and defragment will be called,

There is still one flaw that might be abused by a malicious peer: After calling deduplicate, an already defragmented Buffermight be defragmented again, leading to unnecessary allocations. I will add a new commit to fix that problem with a slightly more complex defragment, deduplicate and Buffer.

[djc]

@geieredgar please take the opportunity to basically add comments for all of Matthias' questions. Would be good to get the documentation in there since this is obviously a tricky topic and it would be nice to make the explanations easier to access if we ever need to iterate more on this. Plus, it will help make my review easier.

[geieredgar]

@djc Some of Matthias' questions became obsolete after my latest refactor, for the others I added comments.

[Matthias247]

Thanks for the hard work of doing all the changes! This looks a lot simpler now. I just pointed out some style nits and added some questions, but I'm fine with merging as is.

[Matthias247]

Looks great to me now!
Thanks for all the changes

Addressed

[djc]

Alright, thanks for seeing this through!