Skip to content

feat(benchmarks): generate the README comparison table in CI - #535

Merged
barjin merged 9 commits into
masterfrom
feat/comparison-benchmark
Sep 4, 2026
Merged

feat(benchmarks): generate the README comparison table in CI#535
barjin merged 9 commits into
masterfrom
feat/comparison-benchmark

Conversation

@barjin

@barjin barjin commented Aug 31, 2026

Copy link
Copy Markdown
Member

Closes #469 — replaces the hand-measured table from #523 with a benchmark harness that measures the latest published clients against a local server in CI, monthly or on demand, and opens a PR when the numbers move.

…ng it

The comparison table added in #523 was measured by hand, so it goes stale as
soon as any of the compared projects ships a release and nobody can check it.
This replaces the numbers with a harness that produces them: a local Node.js
HTTP/2 origin, one benchmark script per ecosystem driving the latest published
release of every client, and a script that rewrites the table between markers in
the README. A monthly workflow runs the lot and opens a PR when the numbers move.

Nothing is pinned and no lockfile is committed — the point is to compare what
the ecosystems ship today, impit included.

Two things the harness found that the hand-written table got wrong. got-scraping
was reported as HTTP/1.1-only because HTTP/2 died with a GOAWAY after roughly a
thousand requests; that was Node's Rapid-Reset mitigation on the test server
counting got's per-response RST_STREAM, not a got-scraping defect, so it is
switched off in the origin and got-scraping is now measured over h2 like
everything else. And the origin reports its connection count, which shows
cycletls opening a fresh TLS connection per request rather than reusing a warm
one — previously invisible in its throughput number, now footnoted.
@github-actions github-actions Bot added this to the 148th sprint - Tooling team milestone Aug 31, 2026
@github-actions github-actions Bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Aug 31, 2026
barjin added 3 commits August 31, 2026 15:25
…umbers

The first CI run showed why best-of-N is the wrong statistic here: node-tls-client
ranged from 900 to 3095 req/s across its eleven runs, so its best would have put
it above impit while its median sits a third below. That spread is intrinsic to
the client rather than machine noise, which is exactly the case best-of-N
flatters. The table now quotes the median, the result files keep best and worst,
and a client whose runs swing by more than 1.5x gets a footnote saying so.

The committed numbers are now the ones the workflow measured on a runner instead
of the ones from a loaded laptop.
@barjin barjin changed the title feat(benchmarks): measure the client comparison instead of hand-writing it feat(benchmarks): generate the README comparison table in CI Aug 31, 2026
@barjin
barjin requested a review from Pijukatel September 1, 2026 09:04

@Pijukatel Pijukatel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One security concern from AI:

The checkout step persists APIFY_SERVICE_ACCOUNT_GITHUB_TOKEN as a git credential (checkout v7 stores it in a temp credentials file wired into the repo's git config via includeIf — any code in the job can recover it with git config --get http.https://github.com/.extraheader). The same job then runs install scripts of deliberately unpinned latest npm and PyPI packages, including ones whose postinstalls download and execute Go binaries (cycletls, node-tls-client) — any of them could exfiltrate the privileged token. Fix: check out with persist-credentials: false (reading needs no PAT) and supply the token only to the final push/PR step, which runs no third-party code.

Comment thread benchmarks/python/bench.py
It resolves the newest bogdanfinn/tls-client release at runtime and downloads
tls-client-linux-ubuntu-amd64-{version}.so from it, but v1.16.0 ships only the
xgo builds, so the asset it asks for no longer exists and a fresh install cannot
start. Nothing on our side can pin it.
@barjin
barjin requested a review from Pijukatel September 3, 2026 09:28
@barjin
barjin merged commit b0774fd into master Sep 4, 2026
10 checks passed
@barjin
barjin deleted the feat/comparison-benchmark branch September 4, 2026 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a comparison guide

3 participants