chore: tests for genReqIdFactory and minor changes by Uzlopak · Pull Request #4783 · fastify/fastify

Uzlopak · 2023-06-03T00:59:50Z

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

github-actions · 2023-06-03T01:03:03Z

Node: 14
PR: [1] 872k requests in 30.08s, 164 MB read
MAIN: [1] 814k requests in 30.09s, 153 MB read

Node: 16
PR: [1] 1682k requests in 30.07s, 316 MB read
MAIN: [1] 1665k requests in 30.05s, 313 MB read

Node: 18
PR: [1] 790k requests in 30.07s, 148 MB read
MAIN: [1] 771k requests in 30.06s, 145 MB read

…rformance

github-actions · 2023-06-03T01:44:59Z

Node: 14
PR: [1] 1758k requests in 30.05s, 330 MB read
MAIN: [1] 1752k requests in 30.05s, 329 MB read

Node: 16
PR: [1] 1862k requests in 30.09s, 350 MB read
MAIN: [1] 1842k requests in 30.04s, 346 MB read

Node: 18
PR: [1] 1383k requests in 30.05s, 260 MB read
MAIN: [1] 1354k requests in 30.05s, 254 MB read

climba03003

Can we have a memory benchmark on the initial consumption?
From the current benchmark, the ram usage is increased around 3 - 6 MB.
We have no indication does it worth for the such gain.

The only problem is when the user provide a custom generation function, the memory consumption still there.

lib/reqIdGenFactory.js

metcoder95

Going naive from my side, I might think that the increase in memory overhead is caused by the (double)array allocation.

As @climba03003 pointed out, is there a way we can lazy-allocate them so if user provide a custom factory, we do not incur in the overhead?

lib/reqIdGenFactory.js

Uzlopak · 2023-06-03T11:21:13Z

@climba03003

How do you benchmark for memory?

I modified it, and assume that it will use now less memory and only if we use our request id generator.

This is my benchmark:

'use strict'
const Benchmark = require('benchmark')
const reqId = require('./lib/reqIdGenFactory')()

const suite = new Benchmark.Suite;

suite.add('reqId', function() {
    reqId()
})
// add listeners
.on('cycle', function(event) {
  console.log(String(event.target));
})
.on('complete', function() {
  console.log('Fastest is ' + this.filter('fastest').map('name'));
})
// run async
.run();

the original:

node bench.js 
reqId x 6,586,817 ops/sec ±0.60% (87 runs sampled)
Fastest is reqId

this PR:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/fastify$ node bench.js 
reqId x 202,195,836 ops/sec ±1.00% (94 runs sampled)
Fastest is reqId

So I kind of doubt, that SMI optimization is even relevant. If I add SMI optimization to this PR, then the performance drops to 56 Mio. Ops/s. But if I "provoke" the SMI deoptimization by setting reqMajorCounter to 2147483647 then I still get 202 Mio. Ops/s. So it seems that the bottleneck is more the req-${(++reqMajorCounter).toString(36)}

I guess, we could improve the perf more by pregenerating the last three digits, but for that imho the memory overhead is not worth.

So from 6 Mio. Ops to 200 Mio. Ops is imho a good perf gain.

github-actions · 2023-06-03T11:25:58Z

Node: 14
PR: [1] 793k requests in 30.08s, 149 MB read
MAIN: [1] 807k requests in 30.07s, 152 MB read

Node: 16
PR: [1] 1729k requests in 30.07s, 325 MB read
MAIN: [1] 1736k requests in 30.06s, 326 MB read

Node: 18
PR: [1] 1630k requests in 30.05s, 306 MB read
MAIN: [1] 1665k requests in 30.05s, 313 MB read

Uzlopak · 2023-06-03T11:26:33Z

Strange. Now the benchmarks are slower? :(

Imho the benchmarks are maybe depending on how busy the github runner is. It does not make sense, as generating the request ids should give perf optimizations.

github-actions · 2023-06-03T11:32:07Z

Node: 14
PR: [1] 1776k requests in 30.06s, 334 MB read
MAIN: [1] 1726k requests in 30.06s, 324 MB read

Node: 16
PR: [1] 852k requests in 30.09s, 160 MB read
MAIN: [1] 836k requests in 30.09s, 157 MB read

Node: 18
PR: [1] 1542k requests in 30.05s, 290 MB read
MAIN: [1] 1547k requests in 30.06s, 291 MB read

Uzlopak · 2023-06-04T14:47:51Z

Any feedback?

lib/reqIdGenFactory.js

RafaelGSS · 2023-06-04T22:39:00Z

@Uzlopak Try using https://github.com/fasterthanlime/mevi to measure memory consumption.

RafaelGSS · 2023-06-04T22:41:30Z

Any feedback?

The results seem consistent. I've run it in my machine quickly, and it's indeed slower than the main. Also note the benchmarks run in a Linux host, on macOS it can be surprisingly different

UPDATE: I don't know why you expect the new patch to be fast than the current version. IMO It's clearly performing more computations.

mcollina

Such a big array always breaks people and adds quite a bit to the minimum amount of memory necessary to use Fastify.

Uzlopak · 2023-06-05T09:33:55Z

I will reinvestigate.

github-actions · 2023-06-05T13:02:10Z

Node: 14
PR: [1] 1749k requests in 30.06s, 329 MB read
MAIN: [1] 1781k requests in 30.05s, 335 MB read

Node: 16
PR: [1] 1408k requests in 30.08s, 265 MB read
MAIN: [1] 1458k requests in 30.07s, 274 MB read

Node: 18
PR: [1] 1326k requests in 30.06s, 249 MB read
MAIN: [1] 1217k requests in 30.08s, 229 MB read

…nit tests

Uzlopak · 2023-07-08T07:53:50Z

As we can not agree on perf improvements, this is basically about adding more fine grained unit tests.

Uzlopak · 2023-07-08T12:42:51Z

@mcollina

Can you approve please?

mcollina

lgtm

github-actions · 2024-07-08T00:24:42Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

perf: improve performance of request id generation

d4320e4

Uzlopak added the benchmark Label to run benchmark against PR and main branch label Jun 3, 2023

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Jun 3, 2023

use 0 comparison instead of object reference comparison for better pe…

11017f7

…rformance

Uzlopak added the benchmark Label to run benchmark against PR and main branch label Jun 3, 2023

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Jun 3, 2023

climba03003 reviewed Jun 3, 2023

View reviewed changes

Eomm reviewed Jun 3, 2023

View reviewed changes

metcoder95 reviewed Jun 3, 2023

View reviewed changes

lib/reqIdGenFactory.js Outdated Show resolved Hide resolved

Uzlopak added 2 commits June 3, 2023 13:09

optimize

0720c56

Merge branch 'main' into perf-req-id

48ac23b

Uzlopak added the benchmark Label to run benchmark against PR and main branch label Jun 3, 2023

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Jun 3, 2023

Uzlopak added the benchmark Label to run benchmark against PR and main branch label Jun 3, 2023

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Jun 3, 2023

RafaelGSS reviewed Jun 4, 2023

View reviewed changes

lib/reqIdGenFactory.js Outdated Show resolved Hide resolved

mcollina requested changes Jun 5, 2023

View reviewed changes

smaller lookup array

90deba4

Uzlopak added the benchmark Label to run benchmark against PR and main branch label Jun 5, 2023

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Jun 5, 2023

Uzlopak added 2 commits July 8, 2023 09:52

remove perf changes, add jsdoc, export as named export and complete u…

08ee74e

…nit tests

Merge branch 'main' into perf-req-id

5f694c7

Uzlopak changed the title ~~perf: improve performance of request id generation~~ chore: tests for genReqIdFactory and minor changes Jul 8, 2023

Uzlopak requested a review from mcollina July 8, 2023 07:54

Uzlopak added 2 commits July 8, 2023 12:42

Merge branch 'main' into perf-req-id

dd127b7

Merge branch 'main' into perf-req-id

bd3cb9c

mcollina approved these changes Jul 8, 2023

View reviewed changes

Uzlopak merged commit 2401461 into main Jul 8, 2023

Uzlopak deleted the perf-req-id branch July 8, 2023 16:18

github-actions bot locked as resolved and limited conversation to collaborators Jul 8, 2024

Uh oh!

Conversation

Uzlopak commented Jun 3, 2023

Checklist

Uh oh!

github-actions bot commented Jun 3, 2023

Uh oh!

github-actions bot commented Jun 3, 2023

Uh oh!

climba03003 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uzlopak commented Jun 3, 2023

Uh oh!

github-actions bot commented Jun 3, 2023

Uh oh!

Uzlopak commented Jun 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 3, 2023

Uh oh!

Uzlopak commented Jun 4, 2023

Uh oh!

Uh oh!

RafaelGSS commented Jun 4, 2023

Uh oh!

RafaelGSS commented Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Jun 5, 2023

Uh oh!

github-actions bot commented Jun 5, 2023

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uzlopak commented Jun 3, 2023 •

edited

Loading

RafaelGSS commented Jun 4, 2023 •

edited

Loading