search

Snowiiii / Pumpkin

Empowering everyone to host fast and efficient Minecraft servers.
https://snowiiii.github.io/Pumpkin/
MIT License
3.31k stars 118 forks source link

VarInt api rewrite #237

Open Vrtgs opened 2 weeks ago

Vrtgs commented 2 weeks ago

Description

reduce code duplication, and have better code (no allocation during serialization (a very common operation), single calls to write instead of multiple) and pass VarInt by value, after all they are just an i32

Testing

I joined the minecraft server

and added a test that is ignored by default (i ran it tho) that double checks that I do serialize and deserialize the VarInts and VarLongs properly

Checklist

Things need to be done before this Pull Request can be merged.

suprohub commented 2 weeks ago

I want simd in varint

StripedMonkey commented 2 weeks ago

I want simd in varint

I doubt varints are a good candidate for simd on their own considering the relatively short runs they make. It's unlikely to see much gains. You are welcome to prototype that however.

suprohub commented 2 weeks ago

I want simd in varint

I doubt varints are a good candidate for simd on their own considering the relatively short runs they make. It's unlikely to see much gains. You are welcome to prototype that however.

https://lib.rs/crates/varint-simd

suprohub commented 2 weeks ago

Screenshot_20241107-090941_Iceraven.png

Fastest encoer and decoder

StripedMonkey commented 2 weeks ago

And if we were doing nothing but crunching millions of varints per second I would agree with you. Just like how I'd agree that SIMD would be faster if we were doing nothing but adding millions of numbers together in sequence.

SIMD requires significant "wind up time" to achieve those kinds of numbers. Because VarInts are actually somewhat sparsely laid out in the data parsing, it's unlikely we'll see major improvements simply because of that.

StripedMonkey commented 2 weeks ago

Regardless, this is a conversation for elsewhere. Please open a new issue or discussion thread.

Snowiiii commented 2 weeks ago

I already see a lot of new unwraps..., I will take a look at this when i have time. But we generally should avoid unwraps

Vrtgs commented 2 weeks ago

I already see a lot of new unwraps..., I will take a look at this when i have time. But we generally should avoid unwraps

Yeah I agree, but since the scope of this PR is supposed to be small I didn't want to rewrite sections, and if the unwrap are triggered that is a bug and it should be fixed, it's just that casting i32 to usize is a faliable operation

Vrtgs commented 2 weeks ago

I want simd in varint

As it stands, I was able to squeeze most of encoding and decoding of varints, because the old implementations needlessly allocated and had multiple write calls, but this isn't really the point of the pr, I agree though it's very cool to have, the api is written in such a way now tho that we can swap the internal implementation any time and all varint encoding would use that new implementation

Vrtgs commented 2 weeks ago

Overall, I don't think this is a bad PR, and applaud your efforts! The VarInt API is definitely not in the best place right now, and I am for any improvements we can make to the API.

Alex or others may take issue with the overuse of macros (imo) for relatively marginal gains, but I'm alright with it for now.

I believe (and have been working on in #105 ) that VarInt shouldn't be a data type present at all, but a serialization format, essentially 

#[serde(with = "var_int")]
len: i32

On any struct which requires an i32 to be formatted as a varint for the transport protocol. I'm quite busy IRL, and it's a long process. If you feel like taking a stab you're welcome to.

This is a good first step, I approve of all the changes except for the branding one.

And honestly I agree, varint is an encoding standard, and I thought about it too, that rather than a datatype that varint should just be a module with all of the methods in varint as just simple functions, but I didn't want to rewrite serialization as well, do I opted for the less invasive option

Snowiiii commented 2 weeks ago

I really dislike having .get() now everywhere. @StripedMonkey solution of using 

#[serde(with = "var_int")]
my_var_int: i32

Sounds really nice, This lets us directly use the field and we also don't have to implement any std::ops:: Things

StripedMonkey commented 2 weeks ago

Like was mentioned, it requires a lot of rework to make the packets use Serde over the funky encode/decode stuff that already exists (which is equivalent to a fn (de)serialize(...))