perf(registry): skip the shared-label branch for metrics that have none

[milcho0604]

Towards #800. You recommended landing the formatLabels() cleanup first, to see what it was worth on its own. I did not measure hoisting exclude to the caller separately, so treat that part as unmeasured. What I did measure is one level out.

getMetricsAsString() built the formatted labels, spread them into a second array along with the flattened shared labels, then filtered that into a third. The filter never filtered labels, because formatLabels() cannot return a falsy element, and the empty string from an empty flatten was the only thing it removed. A guard on the push does the same work with one array. That is where the time was.

Separately, sharedLabels was destructured with a {} default. Only Histogram sets the field, so every series of every other metric allocated that object, ran Object.hasOwn against it once per label, and handed flattenSharedLabels() a key its WeakMap would never see again. Branching on the field being absent skips all of it. A histogram declared without label names still shares an empty object, so it keeps the slow path; making Histogram omit it is a separate change.

Your sketch had if (sharedLabels) and I went with !== undefined. A collector passing sharedLabels: null throws on main, and the truthy test turns that into a silent render with the shared labels missing. Happy to switch if you would rather null render as no shared labels, it is a behaviour change either way and I kept main's.

The numbers are three reps of npm run benchmarks per Node major on 22, 24 and 26, median per case, against trunk at 1908ddb. On the registry metrics() cases this is 16%, 42 of 42 positive, +9% to +22%. Averaging the whole registry suite gives about 10%, since getMetricsAsJSON() is a third of those cases and cannot reach the change. Everything else in the harness sits at +0.07% median, 51 of 93 positive. Measured the same way, 623a74f is 4.9% and taking the helper out on its own is 4.2%, so removing the filter after that is the 12 points and the helper removal cost a little. faceoff's own verdicts on the run read registry ⇒ metrics() no labels ⇒ current (1.24x faster) and similar down the list.

The test is a histogram with two shared labels and a value that needs escaping. On main you can delete the dedup exclusion, or the escaping on shared label values, and all 551 tests still pass. Emitting only the first shared label is caught by one OpenMetrics snapshot in another file. The second test pins the null case, which nothing covered either.

I rendered 234 cases against main on both content types and compared hashes, covering 17 collector shapes including null, primitives and an object with a shadowed prototype. Identical byte for byte, down to which error is thrown and from where. Bypassing the deduplication moves the digest, so the comparison is not asleep.

[jdmarshall]

Rerunning benchmarks because of the preponderance of inconclusive tests.

(I implemented that feature with great enthusiasm and I confess I'm a little irritated by how often it fires, and a little scared about how many times I or coworkers have used ratty data to justify making a change that did not in fact make the code 5% faster, just harder to maintain)

[jdmarshall]

Looks like around 4% average, which is better than anyone has done in a while, if less than I had hoped for.

However, this is one of those categories where an optimization also makes the code easier to read. And I will always take those even if the speed is nothing to write home about. You combine three or four of those together and you get some more interesting numbers. They're easy to defend because they improve code quality at the same time.

Unfortunately I don't have time to review this right this second. But if the timing works out I might consider this for v0.16, because then we can include a modest improvement to metrics() in the release notes.

[jdmarshall]

@milcho0604 If you're looking for other things to do, I believe there are a couple places in the code where we do Array.from(Iterator).map() because Iterator.map and Iterator.filter didn't exist in Node 20. In fact I think I had to modify a PR because I tried to use it and the build failed. We are now >= 22. I don't know if it's faster, and by how much. But it feels like an extra object copy can't be good for throughput.

[milcho0604]

Short answer: at benchmark-like cardinality it is worth about ±2–3% and the sign depends on the Node major; at very high cardinality the render call itself gets 7–20% faster on 24 and 26 while 22 is inconsistent-to-slightly-slower under default heap settings. Since it helps two supported majors and mildly hurts the third, I have not opened a PR.

Scope

Only one site qualifies: Histogram.getForPromString() (lib/histogram.js:123), where Array.from(this.store.values()) exists only to be .map().reduce()-ed — LabelMap.values() hands back the raw Map iterator (util.js:339), so .map() is available on it directly.

The rest are not candidates. registry.js:49, gauge.js:121 and counter.js:121 have to return real arrays, which index.d.ts declares; util.js:326 builds an error message; cluster.js:65 and worker.js:101 sort. LabelGrouper.values() (util.js:441) is the other literal Array.from(iterator).filter(), but nothing in lib/ calls it — metricAggregators.js:48 uses forEach.

Numbers

Real path only, one build per process, order randomised per pair. "4 of 6 pairs" means the change was the faster side in 4 of 6 independent paired runs; positive percentages mean faster.

series rendered	Node 22.23.2	Node 24.11.0	Node 26.5.1
series rendered	Node 22.23.2	Node 24.11.0	Node 26.5.1
---	---	---	---
~3.9k, isolated render call	1 of 10 pairs, median −2.7%	6 of 10, median +1.0%	10 of 10, +0.7%..+6.0%
140k, isolated render call	1 of 10, −14.4%..+1.2%	6 of 6, +7.3%..+19.9%	6 of 6, +6.9%..+19.1%
280k, isolated render call	0 of 3	4 of 4, +5.4%..+15.1%	3 of 3
~3.9k, whole registry.metrics() via faceoff	no resolvable change	no resolvable change	no resolvable change

Two things to read carefully there. The percentages are for getForPromString() alone; a whole registry.metrics() at the same 140k cardinality only moves about 3–5%, because the rest of rendering dilutes it. And the faceoff row means "not resolvable above that harness's noise floor" rather than "identical" — it is two full runs, each normalised against the paths this change cannot reach (getMetricsAsJSON, observe, startTimer), giving +0.5pp and −0.0pp.

The Node 22 part

On 22 the change coincided with more GC — --cpu-prof and an independent PerformanceObserver on gc agree in direction, with both pause time and collection count up (in one 120-call run, 390–415 ms over ~205 collections became 557–572 ms over ~253). On 26 the pause time goes the other way while the collection count stays flat, so there it is cheaper collections rather than fewer. Node 24 gets faster with GC essentially unchanged, which is why I would call GC the leading explanation rather than a proven cause.

Running 22 with --max-semi-space-size=64 turns the regression into +7.9%..+14.6% in 4 of 4 pairs. I read that as heap configuration moderating the result — a larger young generation can also simply move collection outside the measured window — and not as something a library can ask of its users.

I also tried Array.from(this.store.values(), extractBucketValuesForExport(this)), which removes the same intermediate array without iterator helpers. Not a win either: 0 of 4 pairs on Node 24.

Correctness and compatibility

Output is byte-identical across the shapes for the same registry. Neither callback uses the index or source-array arguments (histogram.js:296, :315), the outer data array is not reused, and the iterator is created and fully consumed synchronously after collect() resolves, so live-iterator semantics cannot diverge from the snapshot here.

Availability is not an issue: the Iterator global landed in 22.0.0 itself (nodejs/node#51362), and Bun 1.3.14 has the helpers too; the suite passes on 22, 24, 26 and Bun.

One methodology note, since it changed my answer: isolated micro-benchmarks of the pattern pointed the other way entirely, and only the real path reversed it. I would trust only the real-path numbers here.

What I am not claiming

Not that iterator.map() is generally faster, not a 7–20% improvement for real deployments, not that GC is proven to be the cause, and not that 140k series is representative.

Happy to send the one-liner if you want it for its own sake, but on this evidence I would not present it as a performance change.

[jdmarshall]

Those AI generated summaries are brutal to read, yo.

Node 22 exits LTS in April of 2027. People who are still using that LTS version are probably not on the upgrade treadmill. So I'm okay with making that change in a semver major release, and filing a release note that tells people this may be slightly slower on Node 22. And then they can choose how or if they wish to opt-in.

[jdmarshall]

If you need a large comment, you probably need a different implentation.

This function doesn't need to exist at all. All we need is

if (sharedLabels) {
    const distinctLabels = labels.filter(label => {
        ...
    };

    labelParts = [...formatLabels(distinctLabels),
        ...
    ].filter(Boolean); // TODO: should this be run on the inputs instead of the outputs?
}

There's not enough code here to support the indirection. You're better reading the funky code in the funky conditional directly.

[milcho0604]

Done in 6d8bbf5. The helper is gone and formatLabels() is back to main's, unchanged.

[jdmarshall]

Thanks! Rule of Three felt like a good mantra here. The change I was asking for actually broke up the existing Ro3

[milcho0604]

Fair point on the summaries.

The helper is gone and formatLabels() is main's again, byte for byte. Your TODO turned out to have a third answer, which is that the filter can go entirely. formatLabels() only ever returns name="value" strings, so the empty string from flattenSharedLabels() was the only falsy thing it could drop, and a guard on the push covers it.

I re-ran your revision first. 623a74f measures 4.9% on the registry metrics() cases here, three reps per Node major on 22, 24 and 26, median per case. The current one is 16% on the same cases. If you average the whole registry suite the way I think you did, it comes out around 10%, because getMetricsAsJSON() is a third of those cases and the change can't reach it. Everything else in the harness sits at +0.07%. The CI benchmark on this head lands in the same place, 15.6% and 10.3%, so you do not have to take my machine's word for it.

Worth saying plainly that the gain is the filter, not the helper. Taking the helper out on its own measured 4.2%, slightly below where it started, and dropping the filter after that is worth 12 points. Your sketch kept the filter, so it was the TODO that paid, not the part I was arguing about.

I did not measure hoisting exclude to the caller separately, so "isn't where the win is" is an assertion rather than a number. Happy to do that as a follow-up if you want it.

I'll open the iterator change separately with the Node 22 note.

[jdmarshall]

If you want to add the !== undefined we can still squeeze that in. The last PR for v0.16 is still awaiting review.

[jdmarshall]

Did this increase code coverage? I'm surprised we don't already have these tests.

[milcho0604]

Statements and lines stay at 98.4%, branches go 96.72% to 98.48% on registry.js. The paths were already being executed, what was missing was any assertion on what they produce.

On main you can delete the dedup exclusion, or the escaping inside flattenSharedLabels(), and the suite is still 551 green. Dropping every shared label after the first is noticed by exactly one thing, an OpenMetrics snapshot in the exemplar tests.

[jdmarshall]

Why not inline this into the previous line?

[milcho0604]

Folded. It needed a bit more than moving the line, so flag it if you would rather I put it back.

With sharedLabels in that destructure prefer-const fires on it, since the other two are reassigned below and it is not. Getting a single const destructure means the reassignments become derived values, so the _total suffix is now seriesName and the defensive copy is seriesLabels. Same conditions as before on both. The 234-case differential against main is still byte identical, including absent and null labels, default label collisions and the OpenMetrics counter naming.

[jdmarshall]

wait, why is this back?

[milcho0604]

That line deserved commentary and I skipped it, sorry. It happened in two steps and I only explained the second one.

I did implement the caller side filter, two ways. The entries shape and the shipped shape ran head to head in one process against trunk, and the literal object-copy translation ran separately under the same protocol.

The filter itself is a real win. Kept as a filter on the entries array, with formatLabels() staying one-argument, it beats main by 10% on all 42 metrics() cases. Translating your sketch literally, copying the non-shared labels into a fresh object, measured about 10% slower than main, and it drops a label named __proto__ when a custom collector supplies one, because plain assignment on {} hits the prototype setter. So I threw that shape out. Head to head the entries version trails the shipped one on all 42 cases, median about 4%, the shallowest 1% and the deepest 8%, control centered on zero within about 3%. The difference is one extra filtered array per shared-label series, and histogram series are 93% of what the benchmark renders.

That's the whole reason the second argument came back, it won the measurement. If the one-argument API matters more than the 4%, the entries version is already written and passes the suite. Your call.

[jdmarshall]

Looking for author commentary about how we ended up back at 2 argument formatLabels