bufferer

[cainthebest]

1. Avoiding unnecessary copying: The new_with_data method creates a new Vec<u8> by copying the input data. Instead, it would be more efficient to store a reference to the input data, since the data is not modified.

pub struct Bufferer<'a> {
    data: &'a [u8],
    endianess: Endianess,
    position: usize,
}

impl<'a> Bufferer<'a> {
    pub fn new_with_data(endianess: Endianess, data: &'a [u8]) -> Self {
        Bufferer {
            data,
            endianess,
            position: 0,
        }
    }
    ...
}

2. Removing unnecessary bounds checking: The check_size method checks if the buffer has enough bytes for the requested operation. However, the subsequent code assumes that the buffer has enough bytes, which means that the check is redundant. Removing this check can improve performance.

3. Using slice methods: Instead of converting a slice into an array, you can use slice methods directly. For example, instead of:

let source_data: [u8; 2] = (&self.data[self.position..self.position + 2])
    .try_into()
    .unwrap();

you can write:

let source_data = &self.data[self.position..self.position + 2];

4. Using byteorder crate: The byteorder crate provides functions to convert between integer types and byte representations. Using this crate can simplify the code and improve performance. For example, the get_u16 method can be written as:

use byteorder::{ByteOrder, LittleEndian, BigEndian};

pub fn get_u16(&mut self) -> GDResult<u16> {
    if self.check_size(2) {
        return Err(PacketUnderflow);
    }

    let value = match self.endianess {
        Endianess::Little => LittleEndian::read_u16(&self.data[self.position..]),
        Endianess::Big => BigEndian::read_u16(&self.data[self.position..]),
    };

    self.position += 2;
    Ok(value)
}

5. Using Cow for string values: The get_string_utf8 and get_string_utf16 methods create a new String value by copying the input data. Instead, you can use the Cow type to avoid unnecessary copying. This type represents either a borrowed reference or an owned value, depending on the context. For example, the get_string_utf8 method can be written as:

use std::borrow::Cow;

pub fn get_string_utf8(&mut self) -> GDResult<Cow<str>> {
    let sub_buf = &self.data[self.position..];
    if sub_buf.is_empty() {
        return Err(PacketUnderflow);
    }

    let first_null_position = sub_buf.iter().position(|&x| x == 0).ok_or(PacketBad)?;
    let value = std::str::from_utf8(&sub_buf[..first_null_position])
        .map_err(|_| PacketBad)?;
    self.position += value.len() + 1;

    Ok(Cow::Borrowed(value))
}

6. Avoid using the unwrap() method, as it can panic if the conversion fails. Instead, consider using expect() or match to handle errors more gracefully.

7. Use const fn to precompute constant values at compile time, which can improve the performance of the code at runtime.

8. Use the #[inline] attribute to instruct the Rust compiler to inline small functions, which can improve the performance of the code.

9. Profile the code using a profiler such as perf or valgrind to identify bottlenecks and hotspots in the code, and optimize accordingly.

10. LTO (Link Time Optimization) is a compiler optimization technique that allows the compiler to perform optimizations across multiple translation units during the linking stage of the build process.

[CosminPerRam]

Thanks a lot for the suggestions!

My notes:

1. There is the problem that sometimes, the bufferer is created and returned, outliving the data's lifespan.
   For example: src/protocols/valve/protocol.rs:203 returns a newly made bufferer:
   Ok(Bufferer::new_with_data(Endianess::Little, &data)).

2. I don't understand what do you mean by the subsequent code assumes that the buffer has enough bytes, which means that the check is redundant, do you mean that the check is unnecessary? What confuses me is that the following provided examples from the other points uses the function.

3. Added. Looks great, even better, return the remaining buffer by reference, using remaining_data.

4. Added.

5. Have to look more into it.

6. Good idea, will add.

7. What functions would be candidates to make then const? Isn't this something that the compiler can optimize away?

8. This StackOverflow answer states that:

One limitation of the current Rust compiler is that it if you're not using LTO (Link-Time Optimization), it will never inline a function not marked #[inline] across crates.

Do you mean to add #[inline] if LTO is not enabled?

9. Will look into it.

10. Do you suggest enabling fat lto on the crate? If yes, this would make no. 8 useless, right?

Please DO correct me if I got anything wrong, still a beginner with Rust!