soa-derive icon indicating copy to clipboard operation
soa-derive copied to clipboard

Make the generated vector-like object use a single length

Open mangelats opened this issue 5 years ago • 7 comments

So I've finished with a working version.

Notice that there are a lot of methods missing and tests won't work because of that. Also #sao_derive and zip_iter! not implemented for now.

closes #19

mangelats avatar Sep 03 '20 11:09 mangelats

Thanks you for the PR! I'll have a look at it when I can find some time, probably this weekend.

Luthaf avatar Sep 03 '20 12:09 Luthaf

By the way, this PR points to master but in reality it should point a new branch.

mangelats avatar Sep 03 '20 20:09 mangelats

Overall the implementation looks sane, but I think it is missing documentation & tests now that we are using unsafe directly.

Since there is quite a bit of work to be done here, what's your preferred way of going forward? I can create a separate branch to merge this even if it is unfinished. If you want to publish this on crates.io, I would like to keep compatibility with the stable compiler, so that would mean using a feature to enable this optimization. Otherwise you can use the code directly from the git branch in your own code as well.

Luthaf avatar Sep 13 '20 17:09 Luthaf

Thank you for the review! :)

I believe the end goal should be to have this code merged in master, disabled under a feature by default.

I'm thinking what would be the safest way of doing things and I think I came with an idea:

  • Make an issue tracking the smaller issues and a new branch from master to make the PRs against.
  • Decide the code organization. I'm leaning towards having 3 folders (modules): stable, single_len and common. The reason for that is that the code for some files has nothing in common from the two implementations. I tried doing something like single_len_vec.rs and so on but it ends up being a bit dirty if everything is in the same folder.
  • When the other two things are done, remove all tests. Then follow the cycle: open issue for a single method or additionmake test about what we expect about it (and ensure that it fails)implement itPR.

This would generate a lot of really small issues but I feel that everything would be better organized and we will be sure that the tests work. This means discarding this PR in favour of making smaller ones (the code can be copy-pasted from here).

What do you think?

mangelats avatar Sep 14 '20 13:09 mangelats

I agree that this could be built against a separate branch as multiple smaller PR, I've created a nightly branch for this!

Decide the code organization. I'm leaning towards having 3 folders (modules): stable, single_len and common. The reason for that is that the code for some files has nothing in common from the two implementations. I tried doing something like single_len_vec.rs and so on but it ends up being a bit dirty if everything is in the same folder.

Since only the XXXVec part should change, I would prefer to rename the current vec.rs file to stable_vec.rs, and create a new ``nightly_vec.rs`; and then select one with

#[cfg_attr(feature = "nightly", path = "nightly_vec.rs")]
mod vec;

#[cfg_attr(not(feature = "nightly"), path = "stable_vec.rs")]
mod vec;

Keeping the rest of the code the same as much as possible.

When the other two things are done, remove all tests.

I don't see why this is required. I would rather keep the current tests, potentially using unimplemented!() in the generated code where necessary to keep it compiling.

On top of that we would want to add more tests (one for for each Vec function, potentially following the example of std), which can be tracked separately.

Then follow the cycle: open issue for a single method or addition → make test about what we expect about it (and ensure that it fails) → implement it → PR.

That's one way of doing it, I would be fine with it but I feel it would generate a lot of unnecessary noise. What would be wrong with a single issue containing a list of functions to implement (taken from std)? Do you expect the make test about what we expect about it (and ensure that it fails) step to need a lot of discussion?

Anyway, I would be fine with multiple smaller issues, but (as you can see from my reply time) I have somehow limited bandwidth to work on this repository =)


As a starting point, a minimal implementation of single length vec using unimplemented!() as required, adding a cargo feature to select the new implementation and enabling it in CI looks like the way forward to me.

Luthaf avatar Sep 20 '20 17:09 Luthaf

Also, before spending too much time working on tests & setup for this single length optimization, it would be good to have the code in a state able to run benchmarks (even without documentation or tests), to at least validate this is a worthy investment of your time!

Luthaf avatar Sep 20 '20 17:09 Luthaf

Also, before spending too much time working on tests & setup for this single length optimization, it would be good to have the code in a state able to run benchmarks (even without documentation or tests), to at least validate this is a worthy investment of your time!

For me, the added correctness of a single length is reason enough to invest some time to it :) That said, I'm eager to see if it would make a difference. How about we design a benchmark? What would we need to do so?

Since only the XXXVec part should change, I would prefer to rename the current vec.rs file to stable_vec.rs, and create a new nightly_vec.rs; [...]

Actually vec.rs and iter.rs will need to be versioned, but the same could be done with both of them. What I was thinking was exactly the same but separating them in folders (stable/vec.rs, stable/iter.rs, nightly/vec.rs and nightly/iter.rs); either way is fine for me.

As for removing the tests and making them again, I thought that doing tests and code at the same time would improve both, but looking at it now it's probably a waste of time: we can always make betters tests later on.

mangelats avatar Sep 23 '20 16:09 mangelats