test/benchmarks/benchmark_insert_string.cc (1)

114-117: Consider replacing the hardcoded support_flag “1” with a named constant

Using a named constant (e.g., ALWAYS_SUPPORTED) or a CPU-feature predicate improves clarity at the callsite.

Example:

// constexpr bool ALWAYS_SUPPORTED = true;
BENCHMARK_INSERT_STRING(integer_hash, ::insert_string, ALWAYS_SUPPORTED);
BENCHMARK_INSERT_STRING(rolling_hash, ::insert_string_roll, ALWAYS_SUPPORTED);

BENCHMARK_QUICK_INSERT_STRING(quick_integer_hash, ::quick_insert_string, ALWAYS_SUPPORTED);
BENCHMARK_QUICK_INSERT_STRING(quick_rolling_hash, ::quick_insert_string_roll, ALWAYS_SUPPORTED);

As discussed in the thread, introducing a common define across benchmarks could be a follow-up PR.

Also applies to: 163-164

test/benchmarks/benchmark_insert_string.cc (4)

28-57: Defensive allocation checks in SetUp and null-safe TearDown

Benchmarks should fail fast and clearly on OOM instead of segfaulting. Currently, SetUp() does not check allocations (including the deflate_state itself), and TearDown() dereferences s unconditionally.

Suggested changes within the shown ranges:

     void SetUp(const ::benchmark::State& state) {
-        s = (deflate_state*)zng_alloc(sizeof(deflate_state));
-        memset(s, 0, sizeof(deflate_state));
+        s = (deflate_state*)zng_alloc(sizeof(deflate_state));
+        if (!s) {
+            state.SkipWithError("Failed to allocate deflate_state");
+            return;
+        }
+        memset(s, 0, sizeof(deflate_state));
@@
-        s->window = (uint8_t*)zng_alloc(TEST_WINDOW_SIZE);
+        s->window = (uint8_t*)zng_alloc(TEST_WINDOW_SIZE);
@@
-        s->head = (Pos*)zng_alloc(HASH_SIZE * sizeof(Pos));
-        s->prev = (Pos*)zng_alloc(MAX_WSIZE * sizeof(Pos));
+        s->head = (Pos*)zng_alloc(HASH_SIZE * sizeof(Pos));
+        s->prev = (Pos*)zng_alloc(MAX_WSIZE * sizeof(Pos));
+        if (!s->window || !s->head || !s->prev) {
+            state.SkipWithError("Failed to allocate window/hash tables");
+            return;
+        }
@@
-        for (size_t i = 0; i < TEST_WINDOW_SIZE; i++) {
+        for (size_t i = 0; i < TEST_WINDOW_SIZE; i++) {
             // Create patterns that will exercise the hash function well
             s->window[i] = (uint8_t)((i * 17 + (i >> 4) * 31 + (i >> 8) * 13) & 0xFF);
         }
     }
@@
-    void TearDown(const ::benchmark::State& state) {
-        zng_free(s->window);
-        zng_free(s->head);
-        zng_free(s->prev);
-        zng_free(s);
+    void TearDown(const ::benchmark::State& state) {
+        if (!s) return;
+        zng_free(s->window);
+        zng_free(s->head);
+        zng_free(s->prev);
+        zng_free(s);
+        s = nullptr;
     }

---

73-77: Replace magic “-4” with hash read margin derived from MIN_MATCH/HASH_CALC_OFFSET

The bounds check uses a literal 4. Using protocol-aware constants prevents off-by-one errors and keeps the benchmark robust if hashing macros change.

Apply this diff locally to the two checks:

-        if (str_pos + count >= TEST_WINDOW_SIZE - 4) {
+        if (str_pos + count > TEST_WINDOW_SIZE - kHashReadMargin) {
             state.SkipWithError("Parameters exceed window size");
             return;
         }

-        if (start_pos + count >= TEST_WINDOW_SIZE - 4) {
+        if (start_pos + count > TEST_WINDOW_SIZE - kHashReadMargin) {
             state.SkipWithError("Parameters exceed window size");
             return;
         }

And add the following near the top (after includes/defines) to define the margin once:

// Number of bytes required by hash read at position `str`.
// Prefer MIN_MATCH (3) plus any architecture-specific offset the hashing macros use.
static constexpr size_t kHashReadMargin = MIN_MATCH + HASH_CALC_OFFSET;

If HASH_CALC_OFFSET is not visible in this TU on some platforms, fall back to MIN_MATCH or wire this value in via a small helper header shared with insert_string_tpl.h.

Also applies to: 126-130

---

89-91: Prevent the compiler from eliding work; use DoNotOptimize/ClobberMemory

insert_func writes to s->head/s->prev, but capturing the return value (Pos) and hinting the optimizer makes the intent explicit and future-proof.

-            // Benchmark the insert_string function
-            insert_func(s, str_pos, count);
+            // Benchmark the insert_string function
+            Pos last = insert_func(s, str_pos, count);
+            benchmark::DoNotOptimize(last);
+            benchmark::ClobberMemory();

---

102-111: Name the arguments in the benchmark for readability

ArgNames helps interpret the output without guessing what range(0) and range(1) represent.

-    BENCHMARK_REGISTER_F(insert_string_bench, name) \
+    BENCHMARK_REGISTER_F(insert_string_bench, name) \
+        ->ArgNames({"start_pos", "count"}) \
         ->Args({100, 3})        /* Most common case */ \
         ->Args({100, 4})        \
         ->Args({100, 5})        \
         ->Args({100, 7})        \
         ->Args({100, 14})       /* Mid-range cluster */ \
         ->Args({100, 32})       /* Transition point */ \
         ->Args({100, 127})      /* Large cluster around powers of 2 */ \
         ->Args({100, 255})      /* Near maximum observed values */ \
         ->Unit(benchmark::kNanosecond);

-    BENCHMARK_REGISTER_F(quick_insert_string_bench, name) \
+    BENCHMARK_REGISTER_F(quick_insert_string_bench, name) \
+        ->ArgNames({"start_pos", "count"}) \
         ->Args({100, 1})        /* Single insertion (baseline) */ \
         ->Args({100, 100})      /* 100 insertions (measure amortized cost) */ \
         ->Args({16000, 100})    /* 100 insertions at mid window (different hash distribution) */ \
         ->Unit(benchmark::kNanosecond);

Also applies to: 157-161

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.