Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add COW element access on transients? #122

Closed
arximboldi opened this issue Feb 18, 2020 · 7 comments
Closed

Add COW element access on transients? #122

arximboldi opened this issue Feb 18, 2020 · 7 comments

Comments

@arximboldi
Copy link
Owner

This would allow to do std::sort directly saving some copies. Not sure whether it is a good idea though. There are some drawbacks.

@kevin--
Copy link

kevin-- commented Jan 20, 2022

attempting to do std::sort on a immer::vector -- is this just out of the question right now, or must you copy your contents to an STL container to perform std::sort?

@arximboldi
Copy link
Owner Author

Yes, right now one has to copy the elements into a std::vector. I've considered adding COW element access, but I actually doubt it will be faster in this case. Since every element is touched, more than once actually, I think the relatively high constant on random access on immer::vector is higher than copying it all into a std::vector and back (which when done properly should be pretty fast!).

@arximboldi
Copy link
Owner Author

arximboldi commented Jan 21, 2022

Which I realize, the fastest way to convert an immer::vector to std::vector is maybe not obvious. Actually, I'm not sure which of these two versions is actually faster:

template <typename T>
std::vector<T> to_std(const immer::vector<T>& v)
{
    auto r = std::vector<T>{};
    r.reserve(v.size());
    immer::copy(v, std::back_inserter(v));
    return r;
}

std::vector<int> to_std(const immer::vector<int>& v)
{
    auto r = std::vector<int>{};
    r.reserve(v.size());
    immer::for_each_chunk(v, [&](auto b, auto e) {
        r.insert(r.end(), b, e);
    });
    return r;
}

I'm sure now: https://godbolt.org/z/d5n1Wj8MK

@arximboldi
Copy link
Owner Author

This makes me think, maybe this is such a common thing to do that a family of to_std and from_std functions would make sense in Immer!

@kevin--
Copy link

kevin-- commented Jan 21, 2022

yes that makes a lot of sense to provide the std conversion routines. I have written the from_std equivalent for interop with a serialization library... For now I am maintaining a sorted collection in flex_vector as described in #105 using std::lower_bound

Thanks also for the tip on BENCHMARK in godbolt!

@tusooa
Copy link
Contributor

tusooa commented Jul 1, 2022

template <typename T>
std::vector<T> to_std(const immer::vector<T>& v)
{
    auto r = std::vector<T>{};
    r.reserve(v.size());
    immer::copy(v, std::back_inserter(v));
    return r;
}

I'm sure now: https://godbolt.org/z/d5n1Wj8MK

it should be immer::copy(v, std::back_inserter(r));

the resulting test result is not that ridiculous, though 1 is still slower.

-------------------------------------------------------------
Benchmark                   Time             CPU   Iterations
-------------------------------------------------------------
benchmark_to_std_1    2517592 ns      1367175 ns          454
benchmark_to_std_2    1232508 ns       527364 ns         1259

@arximboldi
Copy link
Owner Author

You are right, sorry!

Corrected godbolt: https://godbolt.org/z/h3cvr7qen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants