Report db_bench interval latency percentiles

[xingbowang]

Summary:
db_bench already prints interval throughput when --stats_interval_seconds is set, but latency percentiles were only available in the final aggregate histogram. That made it impossible for the regression harness to report true interval P99/P99.9 latency, because the runner could only parse the final histogram.

This diff adds opt-in per-interval latency percentile reporting behind --report_interval_percentiles. When enabled together with --histogram=1 and --stats_interval_seconds > 0, db_bench records per-thread, per-operation histograms for the current interval from the existing Stats::FinishedOps path. At each stats interval boundary, thread 0 merges the worker histograms, prints one IntervalPercentiles: line per operation type with count, P50, P75, P99, P99.9, and P99.99, and clears the interval histograms for the next interval.

The new reporting path is off by default and does not change existing final histogram output. It uses per-thread histogram containers guarded by per-thread locks so workers do not contend on one shared histogram while recording operation latency.

The dependent benchmark harness diff consumes the new IntervalPercentiles: lines to publish interval latency rows to Scuba.

Differential Revision: D115867590

[meta-codesync]

@xingbowang has exported this pull request. If you are a Meta employee, you can view the originating Diff in D115867590.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 148.0s.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 803c5bf

---

Summary

Clean, well-scoped addition of opt-in per-interval latency percentile reporting to db_bench. The per-thread histogram approach with per-thread mutexes is sound. No high-severity issues found.

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. num_ops > 1 records aggregate latency as a single sample — db_bench_tool.cc:2904

• Issue: When FinishedOps is called with num_ops > 1 (e.g., FinishedOps(nullptr, db, 100, kRead) at line 7409, or FinishedOps(nullptr, db, total_ops.load(), kRead) at line 7942), the micros value is the elapsed time for all ops combined, but it is recorded as a single histogram sample. This inflates latency percentiles (e.g., P99) for those operation types. This is a pre-existing behavior in the main hist_ histogram (line 2813), and the new interval histogram inherits the same pattern, so it is consistent — but worth noting since the downstream harness will consume these as per-op latencies.
• Root cause: The existing FinishedOps design records wall-clock delta as one sample regardless of num_ops.
• Suggested fix: No change needed for consistency with existing behavior, but consider documenting this caveat in the flag help text or PR description, so the downstream harness is aware. Alternatively, divide micros by num_ops before adding to the histogram, or add num_ops samples of micros / num_ops.

M2. Per-thread mutex acquired on every Add() call in the hot path — db_bench_tool.cc:2693

• Issue: When --report_interval_percentiles is enabled, every FinishedOps call acquires and releases a std::mutex to record the interval histogram sample. While the lock is per-thread (no cross-thread contention), the lock/unlock overhead itself adds latency to the operation being measured, which can perturb the very latency measurements being taken (observer effect). On modern Linux this is ~20-25ns per lock/unlock cycle.
• Root cause: The per-thread histograms use std::mutex for synchronization between Add (worker thread) and ReportAndReset (thread 0).
• Suggested fix: Consider a lock-free design: since HistogramStat::Add is already lock-free (uses relaxed atomics), the per-thread histograms could use HistogramImpl directly without an external mutex, and ReportAndReset could tolerate the small imprecision from concurrent Add during Merge+Clear. HistogramImpl::Merge and Clear already use internal locking. Alternatively, accept the overhead since this is opt-in for a benchmarking tool and the absolute overhead is small relative to typical DB operation latency.

🟢 LOW / NIT

L1. Pre-existing bug: OperationTypeString maps kCompress twice — db_bench_tool.cc:2667

• Issue: Line 2667 has {kCompress, "uncompress"} instead of {kUncompress, "uncompress"}. This means kUncompress operations will map to the default "op" string in the new ReportAndReset output. This is a pre-existing bug (not introduced by this PR), but the new feature makes it more visible since interval percentiles will label uncompress operations as "op".
• Suggested fix: Fix separately: change {kCompress, "uncompress"} to {kUncompress, "uncompress"}.

L2. elapsed field name is misleading — db_bench_tool.cc:2728

• Issue: The elapsed=%.6f field in the output line shows total wall-clock seconds since benchmark start, not the interval duration. While this is useful as a timestamp for the downstream harness, the field name elapsed could be misinterpreted as the interval duration.
• Suggested fix: Consider renaming to wall_secs or elapsed_total, or adding interval_secs as an additional field.

L3. make_unique preferred over reset(new ...) — db_bench_tool.cc:2676,2699,2711,2718

• Issue: Multiple uses of ptr.reset(new T(...)) where std::make_unique<T>(...) would be preferred per modern C++ style. Examples: thread_histograms_.emplace_back(new ThreadHistograms()), histogram.reset(new HistogramImpl()), merged_histogram.reset(new HistogramImpl()), and interval_stats_reporter.reset(new IntervalStatsReporter(n)).
• Suggested fix: Use std::make_unique consistently.

L4. Default member initialization of reporter_agent_ — db_bench_tool.cc:2773

• Issue: The diff adds = nullptr to reporter_agent_ — a good hygiene fix but a separate concern from the feature.
• Suggested fix: Mention in commit message that the reporter_agent_ nullptr initialization is a drive-by fix.

Cross-Component Analysis

Context	Applicable?	Assessment
Thread ID validity	YES	Thread IDs are 0..n-1 from ThreadState(i, ...). The Add() bounds check handles invalid IDs gracefully (silent return).
BGWriter/special threads	YES	BGWriter threads call FinishedOps and have interval_stats_reporter_ set by RunBenchmark. Their thread IDs are within 0..n-1, so they correctly contribute to interval stats.
Lifetime safety	YES	interval_stats_reporter is owned by unique_ptr in RunBenchmark. All threads are joined before RunBenchmark returns (via shared.num_done < n wait). Safe.
HistogramImpl thread safety	YES	Add() is lock-free (relaxed atomics). Merge() and Clear() acquire internal mutex. External per-thread mutex provides ordering between worker Add and thread-0 Merge+Clear. Safe.
Benchmarks not using RunBenchmark	YES	interval_stats_reporter_ defaults to nullptr. The nullptr check guards this. Safe.

Positive Observations

• Cleanly opt-in with sensible flag prerequisites (--histogram --stats_interval_seconds).
• Per-thread histogram design correctly avoids cross-thread contention.
• Defensive thread_id bounds check in Add().
• Good drive-by fix of reporter_agent_ initialization.
• Structured, machine-parseable output format with labeled key=value pairs.
• Does not affect existing final histogram output.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase