Perf improvements

[mcollina]

This PR does a lot of modernization and updates, and it also vendors critical dependencies because they were bottlenecks; I didn't want to fork them into individual repositories. The goal of those changes is to significantly reduce the overhead of prom-client and make it easier to maintain/test.

⚠ counter ➭ new is 8.628% acceptably slower.
⚠ counter ➭ inc is 4.248% acceptably slower.
✓ counter ➭ inc with labels is 60.66% faster.
⚠ gauge ➭ inc is 1.275% acceptably slower.
✓ gauge ➭ inc with labels is 112.6% faster.
✓ histogram ➭ observe#1 with 64 is 91.29% faster.
✓ histogram ➭ observe#2 with 8 is 68.05% faster.
✓ histogram ➭ observe#2 with 4 and 2 with 2 is 35.47% faster.
✓ histogram ➭ observe#2 with 2 and 2 with 4 is 34.35% faster.
✓ histogram ➭ observe#6 with 2 is 40.55% faster.
✓ histogram ➭ startTimer#1 with 64 is 23.84% faster.
✓ histogram ➭ startTimer#2 with 8 is 27.74% faster.
✓ histogram ➭ startTimer#2 with 4 and 2 with 2 is 18.38% faster.
✓ histogram ➭ startTimer#2 with 2 and 2 with 4 is 20.34% faster.
✓ histogram ➭ startTimer#6 with 2 is 25.85% faster.
✓ summary ➭ observe#1 with 64 is 61.16% faster.
✓ summary ➭ observe#2 with 8 is 56.47% faster.
✓ summary ➭ observe#2 with 4 and 2 with 2 is 26.87% faster.
✓ summary ➭ observe#2 with 2 and 2 with 4 is 28.96% faster.
✓ summary ➭ observe#6 with 2 is 41.48% faster.
✓ registry ➭ getMetricsAsJSON() no labels is 29.44% faster.
✓ registry ➭ getMetricsAsJSON() 1 x 64 is 14.81% faster.
✓ registry ➭ getMetricsAsJSON() 2 x 4 is 7.147% faster.
✓ registry ➭ getMetricsAsJSON() 2 x 8 is 12.15% faster.
✓ registry ➭ getMetricsAsJSON() 6 x 2 is 11.09% faster.
✓ registry ➭ getMetricsAsJSON() 2 x 4, 2 defaults is 13.37% faster.
✓ registry ➭ getMetricsAsJSON() 2 x 2, 4 defaults is 19.87% faster.
✓ registry ➭ metrics() no labels is 24.78% faster.
✓ registry ➭ metrics() no labels and openMetrics is 19.77% faster.
✓ registry ➭ metrics() 1 x 64 is 9.910% faster.
✓ registry ➭ metrics() 1 x 64 and openMetrics is 9.042% faster.
✓ registry ➭ metrics() 2 x 4 is 8.194% faster.
✓ registry ➭ metrics() 2 x 4 and openMetrics is 6.241% faster.
✓ registry ➭ metrics() 2 x 8 is 7.505% faster.
✓ registry ➭ metrics() 2 x 8 and openMetrics is 9.062% faster.
✓ registry ➭ metrics() 6 x 2 is 12.14% faster.
✓ registry ➭ metrics() 6 x 2 and openMetrics is 10.71% faster.
✓ registry ➭ metrics() 2 x 4, 2 defaults is 33.89% faster.
✓ registry ➭ metrics() 2 x 4, 2 defaults and openMetrics is 32.25% faster.
✓ registry ➭ metrics() 2 x 2, 4 defaults is 34.64% faster.
✓ registry ➭ metrics() 2 x 2, 4 defaults and openMetrics is 35.77% faster.
✓ cluster ➭ aggregate() is 3.535% faster.

[SimenB]

This is amazing 😍

I'll review later today

[SimenB]

Windows CI is failing. would quoting the pattern help?

[jdmarshall]

This is a boat load of changes for one PR. Swapping out the test running might have warranted its own branch.

[SimenB]

this is awesome work, thanks for taking the time!

I wonder if we could avoid vendoring the dependencies, tho. I know you said you didn't wanna fork them, but are they unmaintained?

it might make sense to migrate test runner in a separate PR to keep this focussed on the actual code changes, but I don't feel strongly

[SimenB]

can we put the vendored deps in their own vendor/ directory or something to make it explicit?

[SimenB]

probably doesn't matter, but can do the structureOutput part inside the then where values are set?

[SimenB]

make them actually private rather than relying on _ prefix convention?

[SimenB]

ditto private comment (won't repeat in the others - but applies to all new _* methods)

[SimenB]

Suggested change

	if (label === 'quantile')
	throw new Error('quantile is a reserved label keyword');
	if (label === 'quantile') {
	throw new Error('quantile is a reserved label keyword');
	}

[SimenB]

we should update the eslint rule to allow this with _

[jdmarshall]

I’m pretty sure you’re not actually saving much computationally by doing it this way rather than retaining async in the function signature here.

    async _getAsync() {
        if (!this.collect) {
            return this._getSync(); //this got faster ~ Node 18
        }
        
        await this.collect();

        …
    }

sacrificing .5% raw throughput for readability makes it easier for the next person to find another 3% elsewhere, instead of getting bogged down in unstated invariants.

Also I have another PR out there that speeds a lot of this file up other more substantial ways.

[mcollina]

This is actually substantial. If you are collecting N metrics that are not async, you are adding at least N*6 microticks (due to all the other usages I've cut in metrics collection). Those add up pretty quickly.

[jdmarshall]

Having spent more time with benchmarking code (looking at ~100x perf difference, down from 110x in Node 20), I think what you should do is this:

Drop the async/sync function separation entirely. Just have one function that returns either a value or a Promise.

When we are aggregating the values, use a for of loop and typecheck for a promise and await any that are. So only the aggregation function is async. That also should allow you to skip over the Promise.all() if only one or two metrics are async, which is likely to be the dominant case.

You should have benchmarks that assume that 1 out of a set of metrics is async and the rest are sync.

[mcollina]

Drop the async/sync function separation entirely. Just have one function that returns either a value or a Promise.

That would actually be less readable in my experience as there would be some duplicated code/logic required. (That's how I did it in the first place but it was ugly).

[jdmarshall]

But you yourself are making the same tradeoff:

	function readAndProcessMemoryStats() {
                //...
		return readFile('/proc/self/status', 'utf8').then(structureOutput);
	}

[jdmarshall]

I had too many other PRs outstanding so I didn’t post this yet, but I have a PR to replace benchmark-regressions with a workalike that wraps bench-node, which is maintained by a NodeJS core contributor. If I can get him to land one more PR I can clean up some junk in the output and then file that here, clearing the dead deps.

[jdmarshall]

Fixes #671

[jdmarshall]

@SimenB some of those numbers are my numbers because you don’t currently bench trunk against the PR.

[jdmarshall]

Duplicate code?

[mcollina]

?

[jdmarshall]

You have two branches with this same code block in it. I think they could probably be combined by using a list comprehension in the middle.

[SimenB]

@jdmarshall very happy with #728, so happy to land it so we can properly compare benchmarks before landing this one 👍

[jdmarshall]

@mcollina I think if you rebase you'll get a new build.

[jdmarshall]

@SimenB I think this PR is fundamentally 3 PRs and is too big to land as written. If @mcollina doesn't follow up with this before the 26th, then I will pull it down and split out the test framework changes into a separate PR, and then see if there's a good cleave line for what's left. There's just way too much going on here. 21 commits and 61 files changed.

I work on 2 other github projects that already using the nodejs test runner and a third that really should be, I think that set of changes should stand on their own. But they'll also be big enough to warrant a separate PR.

I'll do what I can to retain the provenance of the resultant commits. I'm a pretty decent git surgeon.

[jdmarshall]

Matteo is using node:test.describe which is not supported in node 20, which is still LTS until May.

I don't think any of this can be merged as written.

[jdmarshall]

This is generating failing test results when run without the Jest migration you did. I suspect this needed pinning tests prior to making this change.

[jdmarshall]

Okay. I'm beginning to have some concrete complaints about this PR and I'm gathering a list of commits from this PR that are showing up consistently as PR regressions under bench-node. I will update this comment as I find them so as not to spam the comments.

I modified the benchmarks locally to ignore prom-client@latest so that we are looking only at numbers versus trunk, which avoids confusing the contributions of this PR with those of PRs that have already landed.

• The findBounds() changes generate incorrect results when run in isolation of the test rewrites this PR is built upon.
• digest seems to be a net negative. I believe the hot path is showing a net negative

[mcollina]

Bugs always exists (both here and in the existing implementation). This PR include quite a few breaking changes.

In order to achieve the performance profile I need I'm happy to maintain my own fork.
I know this is an approach I often discourage, but business needs sometimes require moving fast, and I have little time to engage with a long review process atm.

I started this before any of the performance optimizations you did landed, essentially duplicating work/making some of the work I did redundant.

Feel free to hand pick whatever you need. I ask for the provenance of the commits to be kept.

[mcollina]

I leave to @SimenB to close whenever he feels it.

[jdmarshall]

How are you maintaining a branch with red builds? Even after rebasing your code I am seeing failures in “npm test” at every step along the way.

If you go this way you’re going to want the worker threads PR that I wrote as it lets you offload a big part of the aggregation call to another thread, which reduces the event loop stall substantially. That will matter as much as the rest of this combined. Particularly for p95 response times.

[mcollina]

I don't. The actual fork is available at https://github.com/platformatic/prom-client.