Skip to content

perf: improve instantation speed by dynamic formatter generation #103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented May 8, 2023

As I see that @trim21 is active in this repo, I wanted to suggest a change, which has maybe more impact than #100

This is just a poc. It is not complete regarding the serialization.

To have some useful performance benchmark, i disabled stacktrace generation.

before:

uzlopak@Battlestation:~/workspace/fastify-org/fastify-error$ node benchmarks/instantiate.js
instantiate Error x 5,245,780 ops/sec ±0.37% (192 runs sampled)
instantiate FastifyError x 238,523,493 ops/sec ±0.07% (196 runs sampled)
instantiate FastifySpecialError x 20,584,535 ops/sec ±0.54% (194 runs sampled)

after:

uzlopak@Battlestation:~/workspace/fastify-org/fastify-error$ node benchmarks/instantiate.js
instantiate Error x 5,208,812 ops/sec ±0.22% (192 runs sampled)
instantiate FastifyError x 238,148,483 ops/sec ±0.08% (195 runs sampled)
instantiate FastifySpecialError x 144,541,760 ops/sec ±0.12% (194 runs sampled)

Obviously it gets slower if type casting is feature complete. But I assume, that it is still faster than calling util.format every time.
Maybe it is useless, as stacktrace is always the biggest perf bottleneck. But in the case, somebody disables stacktrace on production services (which is i think anyway best practice) it shoiuld have an impact on perf.

Checklist

lib/formatFn.js Outdated
const amountOfPlaceholders = countSpecifiers(message)

if (amountOfPlaceholders === 0) {
return () => message
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just use string variable in FastifyError

Copy link
Contributor

Choose a reason for hiding this comment

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

instantiate Error x 1,895,804 ops/sec ±1.62% (184 runs sampled)
instantiate FastifyError x 173,772,171 ops/sec ±2.15% (179 runs sampled)
instantiate FastifySpecialError x 102,105,886 ops/sec ±2.52% (179 runs sampled)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More likely to build FastifyError dynamically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't get it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nevermind.

Anyhow. this is just a poc. If there is interest, I could improve it.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 8, 2023

@ivan-tymoshenko
This is maybe very similar to fast-json-stringify

@ivan-tymoshenko
Copy link
Member

@Uzlopak sorry, I don't understand. Can you explain, please?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 9, 2023

new benchmarks:
before:
node benchmarks/instantiate.js
instantiate Error x 5,148,366 ops/sec ±0.28% (191 runs sampled)
instantiate FastifyError x 121,849,157 ops/sec ±0.50% (190 runs sampled)
instantiate FastifyError arg 1 x 14,033,361 ops/sec ±0.25% (194 runs sampled)
instantiate FastifyError arg 2 x 10,830,852 ops/sec ±0.33% (191 runs sampled)

after:
node benchmarks/instantiate.js
instantiate Error x 5,049,694 ops/sec ±0.26% (193 runs sampled)
instantiate FastifyError x 236,957,612 ops/sec ±0.09% (193 runs sampled)
instantiate FastifyError arg 1 x 83,758,765 ops/sec ±0.91% (185 runs sampled)
instantiate FastifyError arg 2 x 63,728,097 ops/sec ±2.10% (179 runs sampled)

Like I wrote above, i disabled stack tracing to get a more comparable result. Generating the Stack is horrible slow.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 9, 2023

@ivan-tymoshenko

I use now the message string as pattern to create the format function once instead of calling format(message, args) every time.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 9, 2023

With default stackTraceLimit

before:
node benchmarks/instantiate.js
instantiate Error x 322,998 ops/sec ±2.65% (137 runs sampled)
instantiate FastifyError x 231,124 ops/sec ±1.33% (188 runs sampled)
instantiate FastifyError arg 1 x 171,481 ops/sec ±1.44% (182 runs sampled)

after:
node benchmarks/instantiate.js
instantiate Error x 304,488 ops/sec ±3.00% (138 runs sampled)
instantiate FastifyError x 224,038 ops/sec ±1.68% (187 runs sampled)
instantiate FastifyError arg 1 x 188,652 ops/sec ±1.69% (188 runs sampled)
instantiate FastifyError arg 2 x 180,878 ops/sec ±1.80% (176 runs sampled)

In conclusio:

It is signigificant faster if we set stackTraceLimit to 0. If we use default stackTraceLimit, we are not definetly not faster.

@Uzlopak Uzlopak changed the title POC - perf: improve instantation speed by dynamic formatter generation perf: improve instantation speed by dynamic formatter generation May 9, 2023
@@ -3,14 +3,19 @@
const benchmark = require('benchmark')
const createError = require('..')

Error.stackTraceLimit = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

there is already benchmarks/no-stack.js for this?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I don't think benchmarking without stacktraces is worthwhile. Stacktraces are incredibly useful to determine where a problem originated.

What is the goal of the change? What are are you trying to achieve?

@jsumners
Copy link
Member

I think we already had a discussion and determined that we do not want to omit stack traces. They are the purpose of the error.

@trim21
Copy link
Contributor

trim21 commented May 10, 2023

What is the goal of the change? What are are you trying to achieve?

improve string formatting perf. disabling stack trace is would make it easier to focus on the perf of string formatting.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 11, 2023

I think, that in production Environment disabling stacktraces is a viable decision. In development using stacktraces make obviously sense.

@jsumners
Copy link
Member

Sure, it's a viable decision. But I would recommend against it. Without the stack trace you are left with very little information to determine the cause of issues.

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.

5 participants