tcbrindle / flux

A C++20 library for sequence-orientated programming
https://tristanbrindle.com/flux/
Boost Software License 1.0
441 stars 28 forks source link

Blog post shows perf gaps between flux and other approaches #190

Closed Ignition closed 1 month ago

Ignition commented 1 month ago

https://dev.to/serpent7776/comparing-c-range-libraries-for-filterreverse-case-with-non-trivial-lambda-4lj7

I've not looked into why, could be:

tcbrindle commented 1 month ago

Hi @Ignition, thanks for bringing this to my attention.

I looks very much like we have a bug somewhere that is causing Flux to do far more work than necessary. I see that the author has made the benchmark code available, so I'm going to grab that and start investigating.

This is pretty bad PR for a new library, so I need to get this fixed ASAP.

tcbrindle commented 1 month ago

Well, I think I've found what's making Flux look bad.

On line 246 of the benchmark:

std::vector<Out> fluxranges(const std::vector<Data>& v, std::predicate<Data> auto accept, size_t max_items)
{
    auto filtered = flux::from(std::move(v)).filter(accept).take(max_items);

https://github.com/serpent7776/cpp-ranges-bench/blob/934135e7842ad1280d7c6c9e32ca3b9f858603b2/test.cpp#L244-L246

There's call to flux::from(std::move(v)), which is telling Flux to take ownership of v and iterate over that. Unfortunately, v is const, meaning this is actually going to take a copy of the entire input vector at every entry to the function.

Changing this to flux::ref(v), so that we iterate over a reference to v instead, brings the timings for Flux (in my tests) into line with the other libraries.

I've filed a Github issue reporting this to the author, so hopefully the original blog post can be updated.

tcbrindle commented 1 month ago

The author has kindly updated the blog post with some new timings based on the above change, and Flux is now among the fastest options (as it should be! ;-)).

Thanks again @Ignition for letting me know, and thanks @serpent7776 for the quick update to the post.