Skip to content

build: amalgamate.py --with-log and --include-prefix, so more embedders can use the generated files - #169

Merged
fcostaoliveira merged 4 commits into
HdrHistogram:mainfrom
fcostaoliveira:feat/amalgamate-log
Oct 2, 2026
Merged

fcostaoliveira merged 4 commits into
HdrHistogram:mainfrom
fcostaoliveira:feat/amalgamate-log

Conversation

@fcostaoliveira

Copy link
Copy Markdown
Contributor

What

Two opt-in options for script/amalgamate.py, plus a check script, so that the projects that vendor a flattened copy of the library can switch to the generated files:

  • --with-log also emits the log codec: hdr_histogram_log.c, hdr_encoding.c, hdr_time.c and their public headers hdr_histogram_log.h, hdr_time.h. Each .c has its private headers inlined.
  • --include-prefix hdr/ makes the generated .c include "hdr/hdr_histogram.h", for trees that keep the public headers in a hdr/ directory.
  • script/check-amalgamation.sh verifies both, and is run by the amalgamation CI job and by the release job before anything is attached.

The release now also attaches hdr_histogram-amalgamation-with-log-<version>.zip. docs/AMALGAMATION.md gains a section on migrating an existing vendored copy.

No library source changes. Without the new flags the generator's output is byte-identical to what is on main (checked for the default and for --malloc-include).

Why

I tried replacing the vendored copies of Redis, Valkey, memtier_benchmark and Node.js with the generated files, now that #162, #164, #165, #166 and #167 are in. Two of the four did not work as is:

  • memtier_benchmark also uses the log codec (hdr_log_*) and the time helpers. The log files include private headers (hdr_tests.h, hdr_histogram_internal.h, hdr_encoding.h, hdr_endian.h) that the release assets do not ship, plus <hdr/...> includes, so compiling them next to the core amalgamation failed on all three files.
  • Node.js keeps src/ plus include/hdr/. The generated hdr_histogram.c includes "hdr_histogram.h", which does not resolve there: fatal error: 'hdr_histogram.h' file not found.

Redis and Valkey work with the existing options (Valkey's CMakeLists.txt also needs one line removed, since it lists hdr_atomic.h; that is in the new doc section).

Validation

  • check-amalgamation.sh passes with gcc and clang under dash: each of the four amalgamated files preprocesses to the same translation unit as its source and builds with -Wall -Wextra -Wmissing-prototypes -Werror; a log encode/decode round trip works using only the generated files; --with-log --malloc-include routes the core and the codec through the caller's allocator; and with --include-prefix hdr/ the header is byte-identical to include/hdr/hdr_histogram.h and the file compiles with -Isrc -Iinclude. It fails (exit 1) when the generator is deliberately broken.
  • Every run: step of both workflows was executed locally, including the new two-zip packaging and checksums.
  • Redis (checkout @ 9af6e958d), replaced with no Makefile change: builds with 0 compiler warnings; the latency-monitor, info, redis-cli and redis-benchmark suites pass (122 tests). redis-benchmark histogram output steps by 0.1 ms up to 2.1 ms and by 1.0 ms after, as the iterator setter should.
  • Valkey (an export of its tree): builds with 0 warnings; latency-monitor, info, valkey-benchmark and valkey-cli suites pass (142 tests).
  • memtier_benchmark (@ 4ebfa71): replaced, builds from a fresh autoreconf/configure/make. The same workload (2 threads x 5 clients x 20,000 requests with --hdr-file-prefix and --json-out-file) through the old and the new build records 200,000 requests in both, and the compressed histograms written by hdr_log_encode decode correctly with the independent Python hdrh library. memtier's own integration tests were not run.
  • Node.js (@ 463711aa): the generated file, in Node's layout and with histogram.gyp unchanged, compiles inside Node's own build with Node's flags, and Node's src/histogram.cc compiles against the replaced header. Node's code never stores a negative count (subtract refuses or clamps to 0, the CBOR reader accepts unsigned only), so it fits the non-negative counts contract. The full Node link and its test suite had not finished when this was opened; I will comment with the result.

Not tested

  • The real gh release upload and the release: published trigger need an actual release. A manual "Run workflow" exercises everything except the upload.
  • Only x86-64 Linux with gcc 13 and clang 18. The check script is not run on macOS or Windows.

🤖 Generated with Claude Code

fcostaoliveira and others added 3 commits October 2, 2026 11:19
…eyond the core

Redis and Valkey vendor only the core, but memtier_benchmark also uses the log
codec (hdr_log_*) and the time helpers, and Node.js keeps the upstream src/ +
include/hdr/ layout. The release assets could not replace either:

- the log files include private headers (hdr_tests.h, hdr_endian.h,
  hdr_encoding.h, hdr_histogram_internal.h) that the assets do not ship, and
  <hdr/...> includes that a flat directory does not have;
- the generated .c included "hdr_histogram.h", which does not resolve when the
  header lives in include/hdr/.

--with-log also emits hdr_histogram_log.c, hdr_encoding.c and hdr_time.c with
their private headers inlined, plus their public headers rewritten for a flat
layout. --include-prefix hdr/ makes the generated files include
"hdr/hdr_histogram.h" and leaves the header byte-identical to the upstream one.
Without the new flags the output is byte-identical to before.

script/check-amalgamation.sh verifies both variants (per-file preprocessor
equivalence with the sources, -Werror builds, a log encode/decode round trip,
and the hdr/ layout) and is run by the amalgamation CI job and by the release
job before anything is attached. The release now also attaches a
hdr_histogram-amalgamation-with-log-<version>.zip. docs/AMALGAMATION.md gains a
section on migrating an existing vendored copy.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…c use the caller's allocator)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

🤖 Automated first-pass review — a human maintainer's review is still required before merge.

The generator changes look fine, and the default/--malloc-include output stays the same. One thing to fix: check-amalgamation.sh builds with -D_GNU_SOURCE (CMake adds it on UNIX too, CMakeLists.txt:54). Without it, the amalgamated hdr_time.c won't compile under -std=c99 on glibc, because clock_gettime/CLOCK_MONOTONIC aren't declared. Either note that in the --with-log section of docs/AMALGAMATION.md, or drop the define from the check so it tests what an embedder actually gets. Smaller points: --with-log --include-prefix hdr/ together isn't exercised by the check, and the per-project migration recipes in the doc will go stale as Redis, Valkey, memtier and Node move on. I'd also wait for the Node link/test result you said you'd post.

@fcostaoliveira

Copy link
Copy Markdown
Contributor Author

Node.js result, as promised in the description: a full build of nodejs/node @ 463711aa with deps/histogram replaced finished (./configure && make, gcc 13, x86-64 Linux) and the resulting node v27.0.0-pre runs.

What changed in deps/histogram: src/hdr_histogram.c and include/hdr/hdr_histogram.h replaced by the output of script/amalgamate.py --include-prefix hdr/, and src/hdr_atomic.h and src/hdr_tests.h deleted. histogram.gyp, BUILD.gn and unofficial.gni are untouched.

The histogram-related tests all pass: 21 run, 21 ok, 0 failed, 0 skipped (parallel/test-perf-hooks-histogram* including the diff, import, snapshot, stats, qrde, qrde-oracle, qrde-worker and record-zero cases, the sliding-window, timerify and monitor-event-loop-delay tests, and sequential/test-performance-eventloopdelay and test-perf-hooks-histogram-heapdump). The binary contains symbols that exist only in the new code (hdr_iter_linear_set_value_units_per_bucket, hdr_record_value_capped, hdr_total_count, the AVX2 scan), so the replacement is what was linked.

I ran only the histogram-related tests, not Node's whole suite.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@fcostaoliveira
fcostaoliveira merged commit e9fca77 into HdrHistogram:main Oct 2, 2026
32 checks passed
@fcostaoliveira
fcostaoliveira deleted the feat/amalgamate-log branch October 2, 2026 14:33
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