Avoid allocation, use arrays

[LFalch]

Removing Vecs from the API by using e.g. arrays with const generics would make this crate do no allocation whatsoever and could even allow it to not require std. The problem with the amount of allocations needed to use a Rater and the amount of allocation used internally is that it is a significant slowdown and waste and possibly also inhibits the optimiser from understanding what is happening. I have toyed with removing all vectors and got it to work with all tests passing.

We should also consider taking exclusive references to the ratings the update_ratings method to fit more logically with its name and to allow the caller to store them in other ways.

Aditionally, I think Rating should implement Copy. I don't see any good reason to only allow cloning it, especially if this crate is considered done.

[LFalch]

About doing an std feature. I just tried and of course <f64>::sqrt and <f64>::exp are not available with #![no_std] so it would require using other crate to implement them potentially. Probably not worth the effort.

[DataWraith]

Removing Vecs from the API by using e.g. arrays with const generics would make this crate do no allocation whatsoever and could even allow it to not require std.

Avoiding all these allocations would be nice. Can const generics support rating updates with uneven teams (i.e. not all teams have the same number of players)? That use-case was the reason why I went with Vecs originally, IIRC.

We should also consider taking exclusive references to the ratings the update_ratings method to fit more logically with its name and to allow the caller to store them in other ways.

That sounds like a good idea at first glance, as it may get rid of the manual destructuring of the Vecs. I'll have to experiment with it a little to get a feel for the ergonomics. I'm not sure why I didn't do it like this in the first place, there may have been a reason for that I can't remember right now.

Aditionally, I think Rating should implement Copy. I don't see any good reason to only allow cloning it, especially if this crate is considered done.

Agreed.

I've opened the code for the first time in years and found quite a few other things I want to change. The entire serialization machinery is kind-of obsolete now, for example. I may find the time to push some updates tomorrow.

[LFalch]

Can const generics support rating updates with uneven teams (i.e. not all teams have the same number of players)?

Const generics can't support uneven teams no, since there's no way to have an array of differently sized arrays. I was unsure if uneven teams were an intended feature at all because none of the tests had uneven teams but one way to solve that could be to take slices instead: either [&[Rating]; N] or taking exclusive references [&mut[&mut Rating]; N]. I have only ever needed duels and at the very least avoiding allocating 8 vectors in duels would be nice.

I've opened the code for the first time in years and found quite a few other things I want to change. I look forward to seeing the changes you are gonna make :) Will you update the codebase to use edition 2024 too? At least, arrays are nicer to work with since edition 2021 since .into_iter() now does the right thing

[DataWraith]

Const generics can't support uneven teams no, since there's no way to have an array of differently sized arrays. I was unsure if uneven teams were an intended feature at all because none of the tests had uneven teams but one way to solve that could be to take slices instead: either [&[Rating]; N] or taking exclusive references [&mut[&mut Rating]; N].

I've added a test for uneven teams now, and I've been trying to find a better function signature for update_ratings, but no substantial improvement resulted.

The current solution I've settled on still looks like this:

pub fn update_ratings<Ranks>(
    &self,
    teams: Vec<Vec<Rating>>,
    ranks: Ranks,
) -> Result<Vec<Vec<Rating>>, &'static str>
where
    Ranks: AsRef<[usize]>,
{
  ...
}

I tried using slices or mutable slices of Ratings paired with const generics instead, but I couldn't get the "type tetris" to work out in all cases. It is important that the Rater can work with matches where team sizes and the number of teams differ in different matches (and where you may not know about these parameters in advance), and I couldn't find a way to make that happen without the nested Vecs. When trying to use slices, I somehow ended up with &&mut slices I could not mutably dereference -- but that might just be a skill issue on my part. The attempts also ended up with ugly slice syntax everywhere (&[p1][..]), which is probably even worse than having a vec![p1] wrapper from an ergonomics standpoint.

I look forward to seeing the changes you are gonna make :) Will you update the codebase to use edition 2024 too? At least, arrays are nicer to work with since edition 2021 since .into_iter() now does the right thing

Yeah, I updated the edition to 2024.

[LFalch]

Would it be really awful to use tuples? It'd require making a bunch of impls but maybe it's avoid all those vectors. Btw, the error type should probably be an enum so you can actually handle the errors in other ways than just bubbling it up to some error message that won't be understandable for the user.

[DataWraith]

Good idea. I added an Error enum. It might be a bit overkill for the purpose of the library, but at least we're doing it properly.

I'm not sure how using tuples would work with uneven games, but I managed to make update_ratings work using mutable slices, and it turned out to not be that hard; not sure why it wasn't working before.

However, I'm also not sure if I like the ergonomics of the new API yet. You have to sprinkle &mut everywhere, and transitioning from the old API is annoying, so I'll have to think through whether avoiding the allocations is really worth it in that instance. I'll probably take a look at it again on the weekend, as I've already spent too much time on this for now...

[LFalch]

I'm not sure how using tuples would work with uneven games

Yeah, I realised that shortly after typing.

Looking forward to seeing the new release whenever the API has been worked out. I've been toying with making a new one myself too to see what to suggest.

[DataWraith]

Ooof. Sorry, I got completely side-tracked working on a different project.

I'm still somewhat unhappy with the mutable slices (and I've had to add another &mut while fixing a problem), but I've come to the conclusion that you were right: the update_ratings function should actually update the ratings in place, and the added syntax noise is probably no worse than manually destructuring a Vec of Vecs. As a nice side-effect, we can now directly pass in ratings stored in a struct:

struct Player {
  name: &'static str,
  rating: Rating,
}

[...]

let mut team1 = [&mut player.rating];
let mut team2 = [&mut player2.rating];
let mut teams = [&mut team1[..], &mut team2[..]];

update_ratings(&mut teams, [1, 2]);

I did some more testing, and I think I'm happy with the current state of things now. If you have any further feedback about the new changes, I'd be happy to hear it.

Barring any blockers, I'll probably release version 1.0 within the next week or so.