Use one length and capacity variable for whole struct

[boomshroom]

As it stands, this crate appears to make separate fields each with their own Vec. This would duplicate the length and capacity values for each field. This may not be a huge problem, but it would triple the size of the struct as it grows and can lead to the different Vecs falling out of sync.

The main alternative would be to use unsafe and raw pointers (or NonNull pointers). That said, managing the unsafe would almost certainly be more effort than keeping the Vecs in sync. Regardless, I think this could be a useful discussion to be had. Is cutting the struct to a third worth having unsafe code to vet? (probably not)

[Luthaf]

Yes, this is a known limitation of this crate. I think it would be worthwhile to have a single pointer for each member of the struct, plus two usize (len & capacity). I hope implementing SoA with pointer would help in removing some bound checks.

having unsafe code to vet?

We could re-use part of std::vec::Vec implementation here, adapating it as needed. But switching to unsafe code would mean that we would need way more testing on this crate.

[newpavlov]

Additional advantage will be that compiler will know that length of each field is always equal to each other, which may significantly help with optimizations.

[mangelats]

Hey, I've been trying to make something similar for a few days.

Using the experimental RawVec instead of Vec and having a length field shouldn't be too difficult and would allow to have a single len. You are already mimicking Vec interface and most (if not all) unsafe code in Vec seems to be related with using a raw pointers to a buffer as a parameter; which I don't think is relevant enough when you are using it for a SoA since you have multiple buffers anyway.

The other option would be to make some kind of RawVec but having to multiple raw buffers simultaneously allowing to have a single cap. It would be more difficulty since it's very low level (working directly with allocators) and would require extensive use of unsafe and experimental features. Also need to be aware of corner cases like zero sized objects. On top of that they seem to be improving RawVec on the main Rust crate as well as allocators.

The obvious downside of both is that they are based on experimental API. Maybe it could be enabled with a feature. If you are fine with it, I'll try to make the a version using RawVec instead of Vec and make a PR 😄

[Luthaf]

By RawVec you mean using the one in alloc, right? It looks like it is unstable with no path for stabilization, meaning using this would lock soa-derive on nightly.

I think I would rather keep this crate compatible with stable Rust, since one of my goals with it is writing software that's distributed as source and compiled by non programmers. Having to explain the differences between stable & nightly would be more work on top of explaining them they need a Rust compiler.

One possible way forward would be to use an alternative RawVec from a crate, potentially copy-pasted from rust's alloc. If such crate do not yet exists, creating it should be easy, and would allow us to use it with stable Rust.

If the differences between standard Vec and RawVec versions of the code can be kept minimal, I would also be fine with having RawVec enabled with a feature. This would also allow testing the initial assumption that having a single len field for all buffers gives us some performance gains.

[mangelats]

That's the one. RawVec uses parts that are even more unstable than itself... so I don't think copy-pasting would be of any help.

I'm intrigued to see if the compiler is smart enough to leverage the fact to have the same length for all the vectors. I'll make a feature to enable it and make a PR.

[pickfire]

Oh, I thought this is the default but looks like it is not, I wonder why people would use this rather typing out the Vec themselves?

It could be done on stable by using NonNull probably, keeeping the pointers seperately.

[Luthaf]

The advantage of soa_derive is on the API side: being able to consider a XXXVec exactly like a Vec<XXX>, and use `vec.push(e: XXX)` and all other Vec methods directly. One could type the Vec and forward to the methods directly, but this would be high boilerplate code.

…

Le 3 août 2020 à 04:14, Ivan Tham ***@***.***> a écrit : Oh, I thought this is the default but looks like it is not, I wonder why people would use this rather typing out the Vec themselves? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[mangelats]

A bit of an update now that I've spend a few hours implementing a single length version.

An advantage that we didn't discuss is that having a single length makes it impossible to have the underlying arrays unsynchronized. With the current version it is as simple as vec.field.push(value) to desynchronise them. Which leads me the the next point: it'd be better if the user would not have access to the underlying structure (currently Vec). This would ensure that the user is not calling things it should not as well as allowing us to change the underlying logic without any change by the users.

Right now, the tests rely on having access the underlying Vec. This makes it not work with any other implementation. The workaround I found is to call as_slice every time but it's quite verbose and I feel like we should test it more in the way that people will use it.
Also the current version have a lot of functions. I'd split them into "core" and "extra" so we can make a first version of the new implementation without implementing every single function (most of them need to be implemented by hand).

So, what I would do is:

1. Make another issue to define and improve the interface (like Use trait to link the origin struct to generated soa struct #24).
2. Remake tests so they don't depend on the fact that you can access the underlying Vecs.
3. Make and try the new implementation.

This is quite some work but I feel it would be worth it. How do you feel about it?

[Luthaf]

I agree that preventing desynchronization is good, as long as we can still provide access to individual slices with or without soa_zip!.

Right now, the tests rely on having access the underlying Vec. This makes it not work with any other implementation.

Do you have a specific example of this? They should work fine with slice access, not full Vec. If they don't, I agree they need fixing =)

Also the current version have a lot of functions. I'd split them into "core" and "extra" so we can make a first version of the new implementation without implementing every single function (most of them need to be implemented by hand).

Part of the appeal of soa_derive is to get access to all Vec functions without writing to code manually. I fear that splitting between core/extra would break this. At the same time, I see that re-implementing everything would be quite a lot of work, so how about creating a branch out of master to work on this new interface? This allow piecewise changes and integration in other projects with git dependencies, while still landing the full interface in master.

Make another issue to define and improve the interface (like #24).

I'm not sure I see the link between #24 and this, could you elaborate?

---

let me know if I'm unclear!

[pickfire]

@copying Can I see the rough plan of the new expanded structure? Using one length should be easier compared to using one capacity since the allocator should align it according to the size.

[mangelats]

Sorry @Luthaf, I think I explained myself poorly.

The tests are using the underlying Vec. Getting slices (using as_slice) works just fine but makes the tests more verbose.

Also the current version have a lot of functions. I'd split them into "core" and "extra" so we can make a first version of the new implementation without implementing every single function (most of them need to be implemented by hand).

Here I was referring to the tests. Dumb me didn't realise that I didn't write it.
Right now most tests are in the same file which means that to test only the "core" parts you have to specify all the test functions manually. Splitting in 2 makes testing the core much simpler. I also like the idea of having a branch with less tests.
And I absolutely agree that having all the other functions is good and helps programmers.

Make another issue to define and improve the interface (like #24).

The point here is that there can be improvements to the interface and might be better to do some before finishing the other implementation.

Also, reading everything again it feels like I've not implemented almost anything. Actually most code is done in a branch called single-length on my fork. I'm currently making it so expect bugs, tests not working, and dirty git history.
That version allows you to swap between single length with a feature: add --features "use_raw_vec" when calling cargo to swap it =)

[mangelats]

@pickfire Right now the structure I'm working with is:

struct ExampleVec {
  field1: RawVec<T1>,
  field2: RawVec<T2>,
  len: usize
}

This structure doesn't work with a structure that has a field named len. So I'm actually thinking about switching it to:

struct ExampleVecFields {
  pub field1: RawVec<T1>,
  pub field2: RawVec<T2>
}

struct ExampleVec {
  fields: ExampleVecFields,
  len: usize
}

[pickfire]

Can derive even change it into this?

struct ExampleVecFields {
  pub field1: RawVec<T1>,
  pub field2: RawVec<T2>
}

struct ExampleVec {
  fields: ExampleVecFields,
  len: usize
}

[mangelats]

We use procedural macros so you can inject as much code as you want :)

PD: This package don't change the original structure, it only adds more code