Use bytes::Bytes instead of Vec<u8> to represent binary payloads

[kelnos]

As requested, moved to a separate PR :)

[kelnos]

Did you consider to change the packet_buf in the v3_binary_encoder from Vec to Bytes

I did do that. Or are you mean the decoder, not the encoder? If so, I did look at that, but they both use std::io::BufRead.read_until(), which specifically takes a Vec<u8> to write into. Maybe there's a way around that with a larger change or refactor, but I wasn't ready to tackle looking into that yet.

[Totodore]

I added a little commit to refactor the v3_bin_packet_encoder fn when calculating the length of the packet.

For further optimisations, here we could switch from &str/String to Bytes and therefore avoid useless re-allocations because we can cheaply slice the Bytes struct: https://github.com/Totodore/socketioxide/blob/39d700a3bef9e5d5b97a147bc6795c1dcb53c193/engineioxide/src/packet.rs#L159C1-L159C61 This will be the subject of other PRs though.

I only have one remaining question maybe your opinion will help me to decide. What do you think between having public interface requiring impl Into<Bytes> rather than Bytes directly to avoid API breakage and also make the use of Bytes transparent (the user don't have to explicitly specify Bytes::from_*).

[kelnos]

What do you think between having public interface requiring impl Into<Bytes> rather than Bytes directly to avoid API breakage and also make the use of Bytes transparent (the user don't have to explicitly specify Bytes::from_*).

Yeah, that seems like a good idea to me. Are there any downsides to that? I guess larger code size for each monomorphized function if the user uses several different things that implement Into<Bytes>, but that's something the user can control themselves if it's a problem.

One thing that will be an API break is changing the argument type in EngineIoHandler::on_binary(), I don't think there's a way around that.

[Totodore]

One thing that will be an API break is changing the argument type in EngineIoHandler::on_binary(), I don't think there's a way around that.

It is mainly the socketioxide API which is important. It is not a problem to change the engineioxide API.

[Totodore]

Yeah, that seems like a good idea to me. Are there any downsides to that? I guess larger code size for each monomorphized function if the user uses several different things that implement Into, but that's something the user can control themselves if it's a problem.

@kelnos do you plan to implement this anytime soon ? Once it is done we can merge this. If you don't have enough free time for this I can work on it and merge the PR if your ok with this.

[kelnos]

Ah, sorry, I completely forgot there was more to do here & was going to ping you about getting it merged. Sure, I can take care of that.

[kelnos]

Hm, I'm unable to run the tests now; it looks like you changed how the test harness works, but when I:

export RUSTFLAGS="--cfg socketioxide-test"

And then run cargo test, I get:

error: failed to run `rustc` to learn about target-specific information

Caused by:
  process didn't exit successfully: `/home/brian/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/rustc - --crate-name ___ --print=file-names --cfg test=socketioxide-test --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg` (exit status: 1)
  --- stderr
  error: invalid `--cfg` argument: `test=socketioxide-test` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")

Not really sure what's expected here.

Also in CONTRIBUTING you have a section that links to "common scripts" for running the test, but that link seems to not go anywhere now.

[kelnos]

Ah, looks like you put socketioxide-test in the doc, but it's actually socketioxide_test -- underscore, not dash. Weird that the dash caused such an obscure error message though.

[kelnos]

I only have one remaining question maybe your opinion will help me to decide. What do you think between having public interface requiring impl Into<Bytes> rather than Bytes directly to avoid API breakage and also make the use of Bytes transparent (the user don't have to explicitly specify Bytes::from_*).

Ok started working on this after finishing the rebase and fixing conflicts. I'm a little concerned about extra overhead here, though.

For example:

impl ConfOperators {
    pub fn bin<B: Into<Bytes>>(mut self, binary: Vec<B>) -> Self {
        self.binary = binary.into_iter().map(Into::into).collect();
        self
    }
}

Even in the case where a Vec<Bytes> is passed, we have to re-wrap it in a new Vec, which is a bit of a waste. What do you think?

[Totodore]

Even in the case where a Vec is passed, we have to re-wrap it in a new Vec, which is a bit of a waste. What do you think?

The doc on the collect in vec specify that in certain case it will do the mutation "in place" without allocating for a new Vec. Moreover if the user directly specify a Bytes struct, the Into will have no effect and therefore there won't be any modification (I think?).

Maybe we could try to benchmark this but I don't think it is a limiting factor.

According to the implementation, our use case matches the optimization:

• https://doc.rust-lang.org/src/alloc/vec/spec_from_iter.rs.html#54

[kelnos]

Ah, neat. And while that's only an implementation detail, that seems like something that's unlikely to change.

[kelnos]

Also I just realized I force-pushed without first pulling your additional commit that refactored the v3_packet_encoder. Hopefully you still have that locally and can push it again?

[kelnos]

At any rate, I think I made the changes you asked for.

[Totodore]

At any rate, I think I made the changes you asked for.

Thanks, I'll review this this week! :)

[Totodore]

I added some other changes:

• I removed some generic Into<Bytes> from the less used/specialized API
• I changed the bound from Vec<Into<Bytes>> to IntoIterator<Into<Bytes>>. The underlying code doesn't change but it will be more flexible for the user :).

Sorry I should have been more clear on what API bits should be flexible and what can stay fixed.

[kelnos]

No worries and thank you for taking care of it!