Using intrinsics to optimize counting HyperLogLog trailing bits

[mapleFU]

Godbolt link: https://godbolt.org/z/3YPvxsr5s

__builtin_ctz would generate shorter code than hand-written loop.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.44%. Comparing base (08aaeea) to head (ea6505c).
Report is 22 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #846      +/-   ##
============================================
- Coverage     70.59%   70.44%   -0.16%     
============================================
  Files           112      114       +2     
  Lines         61512    61725     +213     
============================================
+ Hits          43427    43481      +54     
- Misses        18085    18244     +159     

Files	Coverage Δ
src/hyperloglog.c	91.17% <100.00%> (-0.05%)	⬇️
src/intrinsics.h	100.00% <100.00%> (ø)

... and 26 files with indirect coverage changes

[enjoy-binbin]

LGTM, can you try to do some benchmarks? I guess it may not be measurable, or the improvement is negligible.

[mapleFU]

I don't have end-to-end benchmark

#include <random>
#include <cstdint>
#include <bit>

constexpr int n = 1000;

static void LoopBased(benchmark::State& state) {
    std::mt19937 rng(0);
    std::uniform_int_distribution<int64_t> uniform_dist(1, 1125899906842624);
    std::vector<int64_t> value;
    for (int i = 0; i < n; ++i) {
       value.push_back(uniform_dist(rng));
    }
    int cnt = 0;
    for (auto _ : state) {
       int count = 1;
       int64_t hash = value[cnt % n];
       int bit = 1;
       while ((hash & bit) == 0) {
         count++;
         bit <<= 1;
       }
       ::benchmark::DoNotOptimize(count);
       ++cnt;
    }
}
// Register the function as a benchmark
BENCHMARK(LoopBased);

static void CtzBased(benchmark::State& state) {
    std::mt19937 rng(0);
    std::uniform_int_distribution<int64_t> uniform_dist(1, 1125899906842624);
    std::vector<int64_t> value;
    for (int i = 0; i < n; ++i) {
       value.push_back(uniform_dist(rng));
    }
    int cnt = 0;
    for (auto _ : state) {
       int count = 1;
       int64_t hash = value[cnt % n];
       count += std::countl_zero(static_cast<uint64_t>(hash));
       ::benchmark::DoNotOptimize(count);
       ++cnt;
    }
}
BENCHMARK(CtzBased);

Quickbench has some error message and I run it on my x86 3800x cpu with gcc12 and Release -O2

-----------------------------------------------------
Benchmark           Time             CPU   Iterations
-----------------------------------------------------
LoopBased        1.36 ns         1.36 ns    474427671
CtzBased        0.479 ns        0.479 ns   1000000000

[mapleFU]

Also this is quickbench in x86 platform: https://quick-bench.com/q/7dlzmaBJDz9Xnfzhsk-1Iw7TP0I
I believe the loop based impl is slower

[enjoy-binbin]

LGTM, @zuiderkwast @PingXie any other ideas before the merge?

[madolson]

It might make sense to have a #define for __builtin_ctzll incase it's not supported in the compiler.

[mapleFU]

It might make sense to have a #define for __builtin_ctzll incase it's not supported in the compiler.

#if defined(__clang__) || defined(__GNUC__)
  return static_cast<int>(__builtin_clzll(value));
#elif defined(_MSC_VER)
  unsigned long index;
  i_BitScanReverse64(&index, value);
  return 63 - static_cast<int>(index);
#else
  int bitpos = 0;
  while (value != 0) {
    value >>= 1;
    ++bitpos;
  }
  return 64 - bitpos;
#endif

I do this because server uses __builtin_clz ( see:

valkey/src/dict.c

Line 1624 in 7424620

return 8 * sizeof(long) - __builtin_clzl(size - 1);

,

valkey/src/server.c

Line 836 in 7424620

int clz = mem > 0 ? __builtin_clzl(mem) : size_in_bits;

)

I also find some code checks that:

valkey/deps/hdr_histogram/hdr_histogram.c

Line 164 in 7424620

return __builtin_clzll(value); /* smallest power of 2 containing value */

So I don't know the idiom here :-(

@madolson do you think something like this is ok? ( borrowed from https://github.com/apache/arrow/blob/b33f040640c7ccb3e6a8406e4d3158608c597025/cpp/src/arrow/util/bit_util.h#L198 )

[PingXie]

Yeah, using __builtin_ctz is good.

So I don't know the idiom here :-(

We need an indirection in config.h. Here is an example

158 #if __GNUC__ >= 3
159 #define likely(x) __builtin_expect(!!(x), 1)
160 #define unlikely(x) __builtin_expect(!!(x), 0)
161 #else
162 #define likely(x) (x)
163 #define unlikely(x) (x)
164 #endif

[mapleFU]

@PingXie I've add an ad-hoc counting_trailing_bits. I didn't add it in config.h because this doesn't like likely, which is an empty operation when compiler doesn't have the hint. Instead, it should fall into loop here.

[zuiderkwast]

LGTM

[PingXie]

@PingXie I've add an ad-hoc counting_trailing_bits. I didn't add it in config.h because this doesn't like likely, which is an empty operation when compiler doesn't have the hint. Instead, it should fall into loop here.

You should be able to move this function to config.h. Make sure you keep inline so that you won't upset the linker with multiple copies of the same function, which also hurts performance, even if the linker doesn't complain (as in the MSVC case).

I would highly recommend moving any intrinsic function wrappers or their "emulation" to config.h. This maximizes their re-usability.

[zuiderkwast]

@zuiderkwast do you have a strong opinion against intrinsic.c?

No, I don't mind.

[mapleFU]

So, from the discussion here, I need add a intrinsics.h and put CountingTrailingZeros in intrinsics.h

My remaining questions:

1. Like Using intrinsics to optimize counting HyperLogLog trailing bits #846 (comment) , should I put other intrinsics here?

[mapleFU]

@PingXie I've added the intrinsics.h now

[PingXie]

Thanks @mapleFU!

[mapleFU]

@PingXie Comment resolved, mind take a look again?

[PingXie]

This change looks good to me. I do have a question about one of your comments but I don't think it would affect the correctness. Thanks for your patience @mapleFU!

[mapleFU]

@enjoy-binbin @madolson could we move forward and check in this? Or there're more things I need to do?

[madolson]

I think @PingXie will merge it, although there are a bunch of test failures I am looking through. (Seem like some old issues, let me re-trigger them)

[PingXie]

There are still some test failures but I don't think they are related.

[mapleFU]

Thanks all!