soa-derive
soa-derive copied to clipboard
Make the generated vector-like object use a single length
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
Thanks you for the PR! I'll have a look at it when I can find some time, probably this weekend.
By the way, this PR points to master but in reality it should point a new branch.
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.
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_lenandcommon. The reason for that is that the code for some files has nothing in common from the two implementations. I tried doing something likesingle_len_vec.rsand 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 addition→make test about what we expect about it (and ensure that it fails)→implement it→PR.
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?
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.
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!
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.