Make the generated vector-like object use a single length #26

mangelats · 2020-09-03T11:15:01Z

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

Luthaf · 2020-09-03T12:38:03Z

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

mangelats · 2020-09-03T20:46:25Z

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

Luthaf · 2020-09-13T16:42:44Z

soa-derive-internal/src/iter.rs

-                #visibility fn iter(&self) -> Iter {
-                    Iter(#create_iter)
-                }
+            pub struct VecIter<'a> {


Why replace the iterator type as well? The previous one should still be constructible through the individual slices. The previous type should also optimize a bit better since we get all the marker traits (ExactSizeIterator and friends) from std, as well as specialization which std can use.

Couldn't understand what the std was doing (RawVec doesn't have .iter()) because the code is not straight forward, so I made my simple iterator. Actually 2 days ago I realized that the standard is simply converting to slice and using its iterator. So yes, we could keep the old code with minor changes.

A point for a future debate would be if making a well-implemented custom iterator may be made more performant than using nested zips (but with the obvious downside of more code maintenance)

Luthaf · 2020-09-13T16:44:21Z

soa-derive-internal/src/iter.rs

+                    if self.n >= self.vec.len() {
+                        return (0, Some(0))
+                    }
+                    let left = self.vec.len() - self.n;


nitpick: how about remaining instead of left?

Luthaf · 2020-09-13T16:46:14Z

soa-derive-internal/src/iter.rs

-            impl<'a> IntoIterator for #slice_mut_name<'a> {
-                type Item = #ref_mut_name<'a>;
-                type IntoIter = #detail_mod::IterMut<'a>;
+        impl<'a> IntoIterator for &'a #vec_name {


These implementation where guarded by if let Visibility::Public(_) = *visibility previously, why remove it?

Actually remade it when replacing the type. I'll reverse this file and change it to use the slices instead of a custom iterator

Luthaf · 2020-09-13T16:46:53Z

soa-derive-internal/src/iter.rs

-            impl<'a> IntoIterator for &'a mut #vec_name {
-                type Item = #ref_mut_name<'a>;
-                type IntoIter = #detail_mod::IterMut<'a>;
+        impl<'a> IntoIterator for &'a mut #vec_name {


This is also missing implementations for slice & slice mut

Luthaf · 2020-09-13T16:47:15Z

soa-derive-internal/src/ptr.rs

@@ -191,7 +191,7 @@ pub fn derive(input: &Input) -> TokenStream {
                    })
                }
            }
-
+            


could you remove the additional whitespace?

I'm so sorry about that. It's a misconfiguration of my editor.

Luthaf · 2020-09-13T16:58:10Z

soa-derive-internal/src/vec.rs

-            #[doc = #vec_name_str]
-            /// ::shrink_to_fit()`](https://doc.rust-lang.org/std/vec/struct.Vec.html#method.shrink_to_fit)
-            /// shrinking all fields.
-            pub fn shrink_to_fit(&mut self) {


To be added back later, right?

Luthaf · 2020-09-13T16:58:53Z

soa-derive-internal/src/vec.rs

            }

            /// Similar to [`
            #[doc = #vec_name_str]
            /// ::truncate()`](https://doc.rust-lang.org/std/vec/struct.Vec.html#method.truncate)
            /// truncating all fields.
            pub fn truncate(&mut self, len: usize) {
-                #(self.#fields_names_1.truncate(len);)*
+                unsafe {


Is this taken from std::vec::Vec implementation?

It's a modified version of it for multiple raw vectors :)

And it's the same for most other methods.

I should add comments stating that

Luthaf · 2020-09-13T17:00:23Z

soa-derive-internal/src/vec.rs

-                #(self.#fields_names_1.push(#fields_names_2);)*
+                self.reserve(1);
+                #(write_to_raw_vec(&mut self.data.#fields_names_1, #fields_names_2, self.len);)*
+                self.len += 1;
            }

            /// Similar to [`
            #[doc = #vec_name_str]
            /// ::len()`](https://doc.rust-lang.org/std/vec/struct.Vec.html#method.len),
            /// all the fields should have the same length.


this part of the doc is no longer required \o/

Luthaf · 2020-09-13T17:13:43Z

soa-derive-internal/src/vec.rs

            }

            /// Similar to [`
            #[doc = #vec_name_str]
            /// ::insert()`](https://doc.rust-lang.org/std/vec/struct.Vec.html#method.insert).
            pub fn insert(&mut self, index: usize, element: #name) {
+                fn insert_into_raw_vec<T>(buf: &mut RawVec<T>, len: usize, value: T, index: usize) {
+                    unsafe {
+                        // infallible


Could you expand on what you mean here?

This is from the original std code.

I believe that what is saying is that this code is actually safe. Not 100% sure though.

Luthaf · 2020-09-13T17:15:19Z

soa-derive-internal/src/vec.rs

-
-            /// Create a slice of this vector matching the given `range`. This
-            /// is analogous to `Index<Range<usize>>`.
-            pub fn slice(&self, range: ::std::ops::Range<usize>) -> #slice_name {


Is this to be added back later?

Luthaf · 2020-09-13T17:19:24Z

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.

mangelats · 2020-09-14T13:47:03Z

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 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?

Most methods are not implemented

Luthaf · 2020-09-20T17:06:48Z

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 · 2020-09-20T17:09:47Z

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!

mangelats · 2020-09-23T16:13:44Z

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.

Luthaf reviewed Sep 13, 2020

View reviewed changes

Make the generated vector-like object use a single length

7284f8f

Most methods are not implemented

mangelats force-pushed the single-length-2 branch from cc9dc38 to 7284f8f Compare September 14, 2020 21:26

mangelats marked this pull request as draft October 2, 2020 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the generated vector-like object use a single length #26

Make the generated vector-like object use a single length #26

mangelats commented Sep 3, 2020 •

edited

Loading

Luthaf commented Sep 3, 2020

mangelats commented Sep 3, 2020

Luthaf Sep 13, 2020

mangelats Sep 14, 2020

Luthaf Sep 13, 2020

mangelats Sep 14, 2020

Luthaf Sep 13, 2020

mangelats Sep 14, 2020

Luthaf Sep 13, 2020

Luthaf Sep 13, 2020

mangelats Sep 14, 2020

Luthaf Sep 13, 2020

mangelats Sep 14, 2020

Luthaf Sep 13, 2020

mangelats Sep 14, 2020 •

edited

Loading

mangelats Sep 14, 2020

mangelats Sep 14, 2020

Luthaf Sep 13, 2020

mangelats Sep 14, 2020

Luthaf Sep 13, 2020

mangelats Sep 14, 2020

Luthaf Sep 13, 2020

Luthaf commented Sep 13, 2020

mangelats commented Sep 14, 2020

Luthaf commented Sep 20, 2020

Luthaf commented Sep 20, 2020

mangelats commented Sep 23, 2020

@@ @@ -191,7 +191,7 @@ pub fn derive(input: &Input) -> TokenStream { @@
                                   })
                               }
                           }

Make the generated vector-like object use a single length #26

Are you sure you want to change the base?

Make the generated vector-like object use a single length #26

Conversation

mangelats commented Sep 3, 2020 • edited Loading

Luthaf commented Sep 3, 2020

mangelats commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mangelats Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luthaf commented Sep 13, 2020

mangelats commented Sep 14, 2020

Luthaf commented Sep 20, 2020

Luthaf commented Sep 20, 2020

mangelats commented Sep 23, 2020

mangelats commented Sep 3, 2020 •

edited

Loading

mangelats Sep 14, 2020 •

edited

Loading