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

unroll search #2034

Merged
merged 13 commits into from
Jan 13, 2025
Merged

unroll search #2034

merged 13 commits into from
Jan 13, 2025

Conversation

DenisYaroshevskiy
Copy link
Collaborator

@DenisYaroshevskiy DenisYaroshevskiy commented Dec 1, 2024

unrolling search while also putting the "check for match" in an isolated place to make the codegen smaller.

I need to add more tests for iteration but other than that should be revieable.

The benchmarks for this also need improvements, the simplest (and not a very good one) is find for one char.

Here is the numbers for that (you should look at the best one always).

This is timing for find (baseline)

/name:find 0/size:10000/type:char/algorithm:eve::algo::find_if/percentage:100/padding:0      55.5 ns         55.5 ns     12491744
/name:find 0/size:10000/type:char/algorithm:eve::algo::find_if/percentage:100/padding:8      55.8 ns         55.8 ns     12606370
/name:find 0/size:10000/type:char/algorithm:eve::algo::find_if/percentage:100/padding:16     89.2 ns         89.2 ns      7866881
/name:find 0/size:10000/type:char/algorithm:eve::algo::find_if/percentage:100/padding:24     87.9 ns         87.9 ns      7962216
/name:find 0/size:10000/type:char/algorithm:eve::algo::find_if/percentage:100/padding:32     62.9 ns         62.9 ns     11197921
/name:find 0/size:10000/type:char/algorithm:eve::algo::find_if/percentage:100/padding:40     60.6 ns         60.6 ns     11972006
/name:find 0/size:10000/type:char/algorithm:eve::algo::find_if/percentage:100/padding:48     75.7 ns         75.7 ns      9416124
/name:find 0/size:10000/type:char/algorithm:eve::algo::find_if/percentage:100/padding:56     73.2 ns         73.2 ns      9590033

This is the performance for search for a single char now:

/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:0         166 ns          166 ns      4209565
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:8         129 ns          129 ns      5348094
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:16        126 ns          126 ns      5584025
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:24        152 ns          152 ns      4592188
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:32        150 ns          150 ns      4663308
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:40        129 ns          129 ns      5412031
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:48        125 ns          125 ns      5611149
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:56        166 ns          166 ns      4205887

This is the performance for search for a single char before:

----------------------------------------------------------------------------------------------------------------------------------
Benchmark                                                                                        Time             CPU   Iterations
----------------------------------------------------------------------------------------------------------------------------------
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:0         140 ns          140 ns      4937022
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:8         200 ns          200 ns      3499883
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:16        200 ns          200 ns      3511696
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:24        138 ns          138 ns      5085738
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:32        137 ns          137 ns      5099563
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:40        167 ns          167 ns      4232450
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:48        167 ns          167 ns      4253506
/name:find 0/size:10000/type:char/algorithm:eve::algo::search/percentage:100/padding:56        138 ns          138 ns      5061660

EVE_FORCEINLINE bool main_loop(Traits tr, I& f, auto unroll_l, S l, Delegate& delegate) const
requires(get_unrolling<Traits>() == 1)
{
(void)unroll_l;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can mark the paramter [[maybe_unused]], it is better than random cast to void IMO.

EVE_FORCEINLINE bool main_loop(Traits tr, I& f, [[maybe_unused]] auto unroll_l, S l, Delegate& delegate) const

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update ont hsi samll details ?
The PR is fine once fixed and can be merged

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't have time to fix this until the weekend.
  2. I still want to add more testing for iteration.

It's good if everything else seems ok.

@DenisYaroshevskiy DenisYaroshevskiy merged commit a7cfc03 into main Jan 13, 2025
32 checks passed
@DenisYaroshevskiy DenisYaroshevskiy deleted the unroll_search branch January 13, 2025 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants