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?

[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]

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.