perf: Improve performance

[Uzlopak]

I assume that a Set is more performant than a Map with checking the value.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[Uzlopak]

We could replace codes Object with a codes-Map and after warning we could actually delete the warning from the codes Map

[Uzlopak]

@jsumners

I resolved that remark. Open a new remark if you still disagree.

[Uzlopak]

I added label semver-major because the emitted-property gets changed from a Map to Set, breaking the api contract.

[jsumners]

@jsumners

I resolved that remark. Open a new remark if you still disagree.

Please don't mark other people's reviews resolved unless they indicate it is resolved.

[Uzlopak]

Could you please describe what you have in mind? I dont expect that you will take over this PR. But you have to give me more than a "I think this PR has shown that the current implementation can remove the codes plain JavaScript object and utilize a single Map for emitted", so that I could have the slightest chance go implement what you have in mind.

Imho the double structure should have the best performance. So i am in doubt that having a single structure will have benefits other than a little bit less memory

[jsumners]

When we add a new code, we add it to the emitted map with a value of false. This indicates two things: 1. the code has been defined and 2. it has not been emitted yet. This seems to make the codes POJO redundant.

[climba03003]

Using a single object would be something like below.

const codes = new Map()

function create (name, code, message) {
  // ...
  return { name, code, message: formatted, emitted: false }
}

function emit (code, a, b, c) {
  const info = codes.get(code)
  if (info === undefined) throw new Error(`The code '${code}' does not exist`)
  if (info.emitted === true) return
  
  // ...
  
  info.emitted = true
}

[Uzlopak]

I benched a little. It seems like that using Objects in both cases boosts the performance significantly. As if v8 optimizes it away.

uzlopak@Battlestation:~/workspace/process-warning$ npm run benchmark

> process-warning@2.0.0 benchmark
> node benchmark/benchmark.js

warn x 1,250,369,939 ops/sec ±0.17% (98 runs sampled)
(node:109564) [FST_ERROR_CODE] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)

[Uzlopak]

Same benchmark on master

uzlopak@Battlestation:~/workspace/process-warning$ node benchmark/benchmark.js 
warn x 155,841,579 ops/sec ±0.24% (96 runs sampled)
(node:114029) [FST_ERROR_CODE] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)

[Uzlopak]

@mcollina

Would you please give a statement?

If my changes are negligible than please close this PR.

[mcollina]

Is this sitting in a hot path of some kind? How did you reach the conclusion to optimize this?

The change looks ok to me.

[Uzlopak]

node16:
before:

uzlopak@Battlestation:~/workspace/process-warning$ node benchmark/benchmark.js 
warn x 39,713,659 ops/sec ±0.98% (97 runs sampled)
(node:119186) [FST_ERROR_CODE_1] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)
(node:119186) [FST_ERROR_CODE_3] FastifyWarning: message

after:

uzlopak@Battlestation:~/workspace/process-warning$ node benchmark/benchmark.js 
warn x 77,635,234 ops/sec ±0.40% (98 runs sampled)
(node:119394) [FST_ERROR_CODE_1] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)
(node:119394) [FST_ERROR_CODE_3] FastifyWarning: message

In node14 the I even get 105m ops/s. Really strange.

@mcollina
I assumed, that this can be potentially performance eater, when adding those warnings to deprecated functionality in fastify, which means that if you update fastify-core you get also a performance penalty even though the code is still valid till v5.

...

[climba03003]

The main performance gain is the return line moved above the check of codes exist.
Since the function here is simple enough, one more check means double of time.

[jsumners]

For what it's worth, the exported emitted object needs to remain a hash. I have literally just needed to update the hash during testing to verify two different cases emit the same warning.

[Uzlopak]

@jsumners

Good point. I will modify it.

[Uzlopak]

Closing in favor of #70