str0m icon indicating copy to clipboard operation
str0m copied to clipboard

Reduce number of allocations

Open algesten opened this issue 3 years ago • 7 comments

The impls has a few Vec<u8> arguments. It would be good to look through the instances of these to see where we can re-use buffers (or use ring buffers), and where we can use lifetimes to avoid internal allocation.

Zero allocations is a non-goal, but we can do better than we currently do.

algesten avatar Dec 21 '22 13:12 algesten

It would be nice to add support for rust allocators api. As this implementation works more like state machine, it could benefit a lot from bump allocators which could be reset each polling iteration

victoryaskevich avatar Jan 12 '23 14:01 victoryaskevich

@VictorBulba Cool! I never used those, but sounds interesting!

algesten avatar Jan 12 '23 14:01 algesten

I was thinking some more about this, for both async and sync APIs it's typically to use &[u8] for writing and &mut [u8] for reading. This way the caller is completely in charge of allocation decisions and the reference only lives for the duration of the IO call.

In sans-IO this is not as trivial. In the case of a write the caller needs to pass owned data so progress on writing can be made at a later stage. In str0m this manifests as Writer::write taking a impl Into<Vec<u8>> rather than a &[u8]. For read it similarly manifests as str0m returning Vec<u8>s instead of taking a &mut [u8] and writing into that.

The bump allocator idea is interesting, but it could only be used for temporary allocations during each handle_timeout and poll_output call. For persistent allocations(those returned out from str0m and those given to str0m as input) I'm not sure it would work as well.

k0nserv avatar Sep 11 '23 08:09 k0nserv

Str0m could use some circular buffer which is a contiguous chunk of memory, and allows append bytes to the end, and consuming then from the start.

Something like:

// BytesBuffer is a special circular buffer for str0m

let buf = BytesBuffer::with_capacity(1024);

// It can be written to
buf.write(&[0x01, 0x02, 0x03, 0x04, 0x05, 0x06]);

// Reading updates the circular buffer's offset,
// so consumed memory can be resused
{
    // Read guard keeps mutable reference to the buffer,
    // so it can't be written to while the guard is alive
    //
    // fn consumed_read<'a>(&'a mut self, len: usize) -> ReadGuard<'a>

    let read_guard = buf.consumed_read(3);

    assert_eq!(read_guard.get_bytes(), &[0x01, 0x02, 0x03]);

    drop(read_guard);
}

// Previous read memory has been consumed
assert_eq!(buf.consumed_read(2).get_bytes(), &[0x04, 0x05]);

str0m could use multiple of such buffers for different purposes. When user does write str0m could copy user bytes into that buffer, so it acts as a "queue". Also ReadGuard can be used as a return type for transmit operations, so it references str0m internal bytes chunk, eliminating all those Vec<u8> and Vec<Vec<u8>>

victoryaskevich avatar Sep 12 '23 05:09 victoryaskevich

Parsing a StunMessage currently also allocates because all attributes are collected into a Vec. Given that we only support a constrained set of attributes, this can probably be improved upon by either keeping each attribute in an Option or using something like SmallVec.

Keeping things within an Option or a static array would have the advantage that we likely could make StunMessage Copy! Note that the use of Copy doesn't mean we will copy the payload. The payload is stored by reference so we are only copying references, not the actual data. SmallVec will allocate on the heap once the inline-storage is exceeded so it cannot be Copy.

thomaseizinger avatar Dec 23 '23 23:12 thomaseizinger

Good point!

I favor Option. This is actually exactly equivalent to our RTP header extension values, where do already do it: https://github.com/algesten/str0m/blob/main/src/rtp/ext.rs#L846 – it would be really nice to make StunMessage the same.

algesten avatar Dec 24 '23 06:12 algesten

Good point!

I favor Option. This is actually exactly equivalent to our RTP header extension values, where do already do it: main/src/rtp/ext.rs#L846 – it would be really nice to make StunMessage the same.

I submitted a PR for for that: https://github.com/algesten/str0m/pull/425

thomaseizinger avatar Dec 27 '23 04:12 thomaseizinger

This seems very non-actionable right now. Closing to tidy up.

algesten avatar Jan 11 '25 19:01 algesten