Low level optimizations for contiguous sequences

[DeveloperPaul123]

Description

Adds low level optimizations to address #57

Optimizations

• std::memset with fill()
• std::memcmp with equal()
• std::memcmp with compare()
• std::memchr with find()
• std::memmem with search()

[DeveloperPaul123]

@tcbrindle I filled this PR to see if I'm on the right track (any feedback much appreciated) and also to make it know that the work is in progress for that particular issue.

[tcbrindle]

Thanks for working on this! It looks like there's a syntax error which is tripping up the non-Windows builds, but the general approach looks good to me 👍

[codecov]

Codecov Report

Patch coverage: 94.59% and project coverage change: +0.12% 🎉

Comparison is base (051dce9) 97.58% compared to head (96d201f) 97.70%.
Report is 30 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
+ Coverage   97.58%   97.70%   +0.12%     
==========================================
  Files          66       67       +1     
  Lines        2276     2398     +122     
==========================================
+ Hits         2221     2343     +122     
  Misses         55       55              

Files Changed	Coverage Δ
include/flux/core/utils.hpp	100.00% <ø> (ø)
include/flux/op/split_string.hpp	100.00% <ø> (ø)
include/flux/op/fill.hpp	94.11% <91.66%> (-5.89%)	⬇️
include/flux/op/find.hpp	96.15% <93.33%> (-3.85%)	⬇️
include/flux/op/equal.hpp	92.00% <94.11%> (+10.18%)	⬆️
include/flux/op/compare.hpp	97.22% <96.15%> (-2.78%)	⬇️
include/flux/op/output_to.hpp	94.44% <100.00%> (+1.11%)	⬆️

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[DeveloperPaul123]

I've noticed that with the any_of move, split() seems to have broken? I'm getting an exception with this code in test_split.cpp

{
    auto sv = "the quick brown fox"sv;

    auto split = flux::split(flux::ref(sv), ' ');

    using S = decltype(split);

    static_assert(flux::multipass_sequence<S>);
    static_assert(flux::contiguous_sequence<flux::element_t<S>>);

    static_assert(flux::multipass_sequence<S const>);
    static_assert(flux::contiguous_sequence<flux::element_t<S const>>);

    STATIC_CHECK(check_equal(std::move(split).map(to_string_view),
            std::array{"the"sv, "quick"sv, "brown"sv, "fox"sv}));
}

[DeveloperPaul123]

Just looking for an initial round of feedback; I'll have to address the build issues.

[tcbrindle]

There are quite a few comments, but several of them are just minor style things.

This is looking in pretty good shape, we just need to make sure that the conditions for using the specialisations is correct in each case. I'd rather err on the side of caution -- it's better to use a potentially slower code path than give the wrong answer or (worse) end up with UB.

There do seem to be a lot of unnecessary, unrelated formatting changes though -- I guess from clang-format messing things up? I started marking them but gave up as there are quite a few... Please put these lines back to how they were originally so that the diff only includes meaningful changes.

[tcbrindle]

I've noticed that with the any_of move, split() seems to have broken? I'm getting an exception with this code in test_split.cpp

{
    auto sv = "the quick brown fox"sv;

    auto split = flux::split(flux::ref(sv), ' ');

    using S = decltype(split);

    static_assert(flux::multipass_sequence<S>);
    static_assert(flux::contiguous_sequence<flux::element_t<S>>);

    static_assert(flux::multipass_sequence<S const>);
    static_assert(flux::contiguous_sequence<flux::element_t<S const>>);

    STATIC_CHECK(check_equal(std::move(split).map(to_string_view),
            std::array{"the"sv, "quick"sv, "brown"sv, "fox"sv}));
}

This is caused by the new specialisation of find (which split uses) returning the wrong thing in some cases. See the review comments above (the any_of thing is just a coincidence).

[DeveloperPaul123]

I believe I've addressed most, if not all of your comments. Unit tests are also all passing for me. Please take another look when you have a moment. Thanks!

[tcbrindle]

This is looking really good! There's only one change still outstanding, namely the definition of can_memset in fill.hpp

[DeveloperPaul123]

Whoops! I missed that, thanks!

[tcbrindle]

How would you like to handle the different cases in compare()? What would make the most sense if both sequences are empty? Or if only one sequence is empty?

For compare(), I think we get the right behaviour if we do something like:

auto const seq1_size = flux::usize(seq1);
auto const seq2_size = flux::usize(seq2);
auto min_size = std::min(seq1_size, seq2_size);

int cmp_result = 0;

if (min_size > 0) {
    auto const data1 = flux::data(seq1);
    FLUX_ASSERT(data1 != nullptr);
    auto const data2 = flux::data(seq2);
    FLUX_ASSERT(data2 != nullptr);

    cmp_result = std::memcmp(data1, data2, min_size);
}

That is, we only call the C function if the number of bytes is greater than zero, also making sure that the data pointers are not nullptr.

This is getting messier and messier, sorry!

[DeveloperPaul123]

Ok I think this is ready now and all tests pass for me!

[tcbrindle]

We can simplify the test in equal(), because we already know that both sequences are the same size. Other than that, just a couple of tiny code style things.

CodeCov is complaining that the new "size == 0" code-paths aren't being tested, so it's probably worth adding a couple of little tests to cover this -- but I'm happy to do it if you've had enough of this PR already!

Also, it looks like the new compare() specialisation isn't actually reached by any of the existing tests... This definitely needs rectifying before we can commit this, but again, I'm happy to add some unsigned char compare() tests if you don't fancy it.

[DeveloperPaul123]

I was able to add some tests, but not sure if I covered everything you had in mind. I also address all (I think) your feedback from the latest review. Sorry for the silly oversights at times and thanks for all the feedback!

[DeveloperPaul123]

Looks like I didn't cover all the cases for compare(). Some help with this would be appreciated.

[tcbrindle]

All the actual implementation code looks great now!

compare() is still missing a couple of test cases, I've made some suggestions below.

CodeCov is also complaining that we're not testing the case of two zero-length contiguous sequences in equal(). Something like

std::array<int, 0> arr;
STATIC_CHECK(flux::equal(arr, arr) == true);

in test_equal.cpp should keep it happy I think?

[DeveloperPaul123]

Done!

[tcbrindle]

It's done!! 🎉

Thanks so much!