Improve benchmarks #258

sirenkovladd · 2023-09-03T22:48:34Z

Add async benchmarks
use process.hrtime()

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

sirenkovladd · 2023-09-03T22:48:51Z

master

❯ node ./benchmark/benchmark.js
Request x 331,715 ops/sec ±4.20% (81 runs sampled)
Custom Request x 25,142 ops/sec ±2.08% (85 runs sampled)
Request With Cookies x 232,410 ops/sec ±6.46% (82 runs sampled)
Request With Cookies n payload x 213,823 ops/sec ±3.60% (81 runs sampled)
ParseUrl x 1,313,090 ops/sec ±1.78% (89 runs sampled)
ParseUrl and query x 115,189 ops/sec ±1.75% (89 runs sampled)

fix-banchmark

Request x 345031 ops/sec ±15.56% (1725172 runs sampled)
Custom Request x 27734 ops/sec ±7.82% (138679 runs sampled)
Request With Cookies x 246922 ops/sec ±7.64% (1234610 runs sampled)
Request With Cookies n payload x 216127 ops/sec ±9.23% (1080638 runs sampled)
ParseUrl x 922286 ops/sec ±12.55% (4611434 runs sampled)
ParseUrl and query x 110163 ops/sec ±5.72% (550814 runs sampled)
read request body JSON x 95963 ops/sec ±12.21% (479814 runs sampled)
read request body buffer x 113153 ops/sec ±9.63% (565767 runs sampled)
read request body readable x 57184 ops/sec ±19.42% (285924 runs sampled)
Response write end x 29143 ops/sec ±65.02% (145726 runs sampled)
Response writeHead end x 25164 ops/sec ±197.25% (125837 runs sampled)

Uzlopak

i am -1 of a self implemented benchmark.js

Uzlopak · 2023-09-03T23:03:19Z

use alternatively tinybench, if you dislike benchmark.js. it is also used by vitest.

sirenkovladd · 2023-09-03T23:11:17Z

Cool, I'll switch to a tinybench if you don't mind)

sirenkovladd · 2023-09-03T23:16:23Z

❯ node ./benchmark/benchmark.js
┌─────────┬──────────────────────────────────┬─────────────┬────────────────────┬───────────┬─────────┐
│ (index) │            Task Name             │   ops/sec   │ Average Time (ns)  │  Margin   │ Samples │
├─────────┼──────────────────────────────────┼─────────────┼────────────────────┼───────────┼─────────┤
│    0    │            'Request'             │  '343,888'  │ 2907.9191370450485 │ '±1.92%'  │ 171945  │
│    1    │         'Custom Request'         │  '28,236'   │ 35414.67135123085  │ '±2.08%'  │  14120  │
│    2    │      'Request With Cookies'      │  '256,366'  │ 3900.6658665664277 │ '±1.75%'  │ 128184  │
│    3    │ 'Request With Cookies n payload' │  '215,695'  │ 4636.170700281984  │ '±2.07%'  │ 107848  │
│    4    │            'ParseUrl'            │ '1,062,369' │ 941.2924302962186  │ '±1.25%'  │ 531185  │
│    5    │       'ParseUrl and query'       │  '119,659'  │ 8357.076274398258  │ '±0.80%'  │  59830  │
│    6    │     'read request body JSON'     │  '84,748'   │ 11799.619041712936 │ '±1.48%'  │  42375  │
│    7    │    'read request body buffer'    │  '114,046'  │ 8768.364248272579  │ '±1.58%'  │  57024  │
│    8    │   'read request body readable'   │  '63,504'   │ 15746.902949084779 │ '±3.14%'  │  31753  │
│    9    │       'Response write end'       │  '27,661'   │  36151.2651495023  │ '±13.34%' │  13831  │
│   10    │     'Response writeHead end'     │  '35,904'   │  27851.3512740605  │ '±6.76%'  │  17953  │
│   11    │          'base inject'           │  '18,365'   │ 54450.594459135114 │ '±13.36%' │  9183   │
└─────────┴──────────────────────────────────┴─────────────┴────────────────────┴───────────┴─────────┘

Uzlopak

I approve, but prefer that a core maintainer OKs the switch to tinybench.

Uzlopak · 2023-09-04T00:57:53Z

benchmark/benchmark.js

+  .add('Response writeHead end', async function () {
+    const req = new Request(mockReq)
+    await new Promise((resolve) => {
+      const res = new Response(req, () => resolve())


Suggested change

const res = new Response(req, () => resolve())

const res = new Response(req, resolve)

Uzlopak · 2023-09-04T00:58:03Z

benchmark/benchmark.js

+  .add('Response write end', async function () {
+    const req = new Request(mockReq)
+    await new Promise((resolve) => {
+      const res = new Response(req, () => resolve())


Suggested change

const res = new Response(req, () => resolve())

const res = new Response(req, resolve)

benchmark/benchmark.js

mcollina

I'm -1 on the usage of tinybench because:

it's written in typescript, which generates code slower than hand-crafted JavaScript.
a benchmark framework should be written with the intent to be as low overhead as possible
it forces the use of an async function, when a promise is enough.

kibertoad · 2023-09-04T09:09:03Z

it's written in typescript, which generates code slower than hand-crafted JavaScript.

I still haven't seen any conclusive evidence for that, can you share any?

But even if we assume that there is an overhead, would that overhead be consistent across all of the options tested, hence comparison would still be accurate?

kibertoad · 2023-09-04T09:10:11Z

it forces the use of an async function, when a promise is enough.

There is no such thing as "requiring an async function" in TS. All you can do is expect something to return a promise within an interface. Whether that is achieved by making function async, or just by returning a promise directly, makes no difference to TS. So it's the usage that is wrong, it can be adjusted to avoid async.

mcollina · 2023-09-04T09:14:25Z

I still haven't seen any conclusive evidence for that, can you share any?

I have not done a scientific study on this, it's more about in-the-field examples.
https://github.com/WebReflection/raytrace provides a good example.

The fundamental problem is that TS can generate quite a bit of added code that's either suboptimal or just useless.

Normally this is not a problem, but for something that must be as low overhead as possible, it's a risk I do not want to have.

There is no such thing as "requiring an async function" in TS. All you can do is expect something to return a promise within an interface. Whether that is achieved by making function async, or just by returning a promise directly, makes no difference to TS. So it's the usage that is wrong, it can be adjusted to avoid async.

They actually force it: https://github.com/tinylibs/tinybench/blob/v2.5.0/src/utils.ts#L31

kibertoad · 2023-09-04T09:16:11Z

I have not done a scientific study on this, it's more about in-the-field examples.
https://github.com/WebReflection/raytrace provides a good example.

I've been meaning to research that one in greater depth, I guess now's a good reason to do so. But from what I remember they were comparing separately implemented TS and JS versions of the same algorithm, not the same code with added types and transpilation step, so it's somewhat an oranges to apples comparison.

kibertoad · 2023-09-04T09:18:58Z

They actually force it: https://github.com/tinylibs/tinybench/blob/v2.5.0/src/utils.ts#L31

Oooof. Yes, that seems unnecesarily restrictive. Are there any better alternatives?

Uzlopak · 2023-09-04T09:23:23Z

I partially worked with @Aslemammad on tinybench v1 (v2 was a complete rewrite by himself). I guess he can fix the async issue.

On the other hand... Tinybench has, even if ts is adding overhead, still less overhead than benchmark
Js. Simply because it has less deps than benchmark.js. e.g. benchmark js relies heavily on lodash.

Tinybench has the benefit, that when used with 0x or deoptigate your report does not get tainted by lodash and co. This is always super annoying.

Uzlopak · 2023-09-04T09:24:47Z

Lol.

isAsyncFunction looks like code i wrote.

mcollina

lgtm, ok

sirenkovladd · 2023-09-04T10:21:03Z

Oooof. Yes, that seems unnecesarily restrictive. Are there any better alternatives?

tinylibs/tinybench#52

sirenkovladd · 2023-09-11T21:00:42Z

Do you have any blocks to merge PR?

kibertoad · 2023-09-11T21:02:41Z

@sirenkovladd Shouldn't a new version of tinybench be released first? I think the only blocker was the hard requirement for async, as long as that is addressed, should be good to go.

Uzlopak · 2023-09-11T21:24:23Z

@sirenkovladd
Please fix the merge conflicts till next version of tinybench gets released :)

sirenkovladd · 2023-09-12T11:11:35Z

FYI, done

Uzlopak · 2023-09-12T11:30:03Z

Please run a final time the benchmarks and post them here, please

sirenkovladd · 2023-09-12T11:52:25Z

master

Request x 333,541 ops/sec ±1.55% (84 runs sampled)
Custom Request x 25,426 ops/sec ±1.52% (85 runs sampled)
Request With Cookies x 226,453 ops/sec ±0.62% (94 runs sampled)
Request With Cookies n payload x 203,879 ops/sec ±2.38% (84 runs sampled)
ParseUrl x 1,429,320 ops/sec ±2.22% (91 runs sampled)
ParseUrl and query x 152,945 ops/sec ±0.21% (97 runs sampled)

PR

┌─────────┬──────────────────────────────────┬─────────────┬────────────────────┬───────────┬─────────┐
│ (index) │            Task Name             │   ops/sec   │ Average Time (ns)  │  Margin   │ Samples │
├─────────┼──────────────────────────────────┼─────────────┼────────────────────┼───────────┼─────────┤
│    0    │            'Request'             │  '347,861'  │ 2874.7101649480496 │ '±1.98%'  │ 173931  │
│    1    │         'Custom Request'         │  '28,979'   │ 34507.06578717414  │ '±3.51%'  │  14552  │
│    2    │      'Request With Cookies'      │  '268,781'  │ 3720.4932407779993 │ '±2.39%'  │ 134391  │
│    3    │ 'Request With Cookies n payload' │  '254,264'  │ 3932.905963058997  │ '±1.93%'  │ 127133  │
│    4    │            'ParseUrl'            │ '1,261,205' │ 792.8919175847672  │ '±0.39%'  │ 630603  │
│    5    │       'ParseUrl and query'       │  '149,455'  │ 6690.956283663417  │ '±0.57%'  │  74728  │
│    6    │     'read request body JSON'     │  '95,291'   │ 10494.162926894045 │ '±1.65%'  │  47650  │
│    7    │    'read request body buffer'    │  '127,813'  │ 7823.8837416164315 │ '±1.68%'  │  63959  │
│    8    │   'read request body readable'   │  '63,992'   │ 15626.766763486188 │ '±3.38%'  │  31997  │
│    9    │       'Response write end'       │  '32,399'   │  30864.4391807877  │ '±13.35%' │  16200  │
│   10    │     'Response writeHead end'     │  '38,136'   │ 26221.47444951067  │ '±7.28%'  │  19073  │
│   11    │          'base inject'           │  '17,928'   │ 55776.52583691374  │ '±14.57%' │  8965   │
└─────────┴──────────────────────────────────┴─────────────┴────────────────────┴───────────┴─────────┘

Uzlopak · 2023-09-12T12:14:05Z

@mcollina

Are you happy :)?

Uzlopak · 2023-09-12T12:16:31Z

wait. you did not fix the remarks by mcollina. so you can now remove the async and return directly the promise. Then again benchmarks please.

sirenkovladd · 2023-09-12T13:17:44Z

┌─────────┬──────────────────────────────────┬─────────────┬────────────────────┬───────────┬─────────┐
│ (index) │            Task Name             │   ops/sec   │ Average Time (ns)  │  Margin   │ Samples │
├─────────┼──────────────────────────────────┼─────────────┼────────────────────┼───────────┼─────────┤
│    0    │            'Request'             │  '344,454'  │ 2903.1371215737463 │ '±2.06%'  │ 172228  │
│    1    │         'Custom Request'         │  '26,937'   │ 37122.354565240974 │ '±3.46%'  │  13469  │
│    2    │      'Request With Cookies'      │  '242,493'  │ 4123.821491915785  │ '±1.74%'  │ 121247  │
│    3    │ 'Request With Cookies n payload' │  '225,006'  │ 4444.306685991836  │ '±1.73%'  │ 112504  │
│    4    │            'ParseUrl'            │ '1,095,375' │  912.92936133353   │ '±0.53%'  │ 547688  │
│    5    │       'ParseUrl and query'       │  '127,661'  │ 7833.186162870036  │ '±0.41%'  │  63831  │
│    6    │     'read request body JSON'     │  '88,130'   │ 11346.851367115709 │ '±1.22%'  │  44066  │
│    7    │    'read request body buffer'    │  '120,823'  │ 8276.531522503943  │ '±1.45%'  │  60412  │
│    8    │   'read request body readable'   │  '56,321'   │ 17755.187702620893 │ '±2.74%'  │  28161  │
│    9    │       'Response write end'       │  '28,837'   │ 34676.64944992473  │ '±9.10%'  │  14419  │
│   10    │     'Response writeHead end'     │  '27,481'   │ 36387.79125106221  │ '±27.87%' │  13741  │
│   11    │          'base inject'           │  '18,936'   │ 52807.191164733515 │ '±13.80%' │  9469   │
└─────────┴──────────────────────────────────┴─────────────┴────────────────────┴───────────┴─────────┘

sirenkovladd changed the title ~~add new benchmark~~ Make the benchmark more useful Sep 3, 2023

Uzlopak requested changes Sep 3, 2023

View reviewed changes

add new benchmark

e46d4d7

sirenkovladd force-pushed the fix-benchmark branch from 40b9218 to e46d4d7 Compare September 3, 2023 23:15

add parse errors

82311f5

Uzlopak approved these changes Sep 4, 2023

View reviewed changes

Uzlopak reviewed Sep 4, 2023

View reviewed changes

mcollina reviewed Sep 4, 2023

View reviewed changes

benchmark/benchmark.js Outdated Show resolved Hide resolved

benchmark/benchmark.js Outdated Show resolved Hide resolved

mcollina requested changes Sep 4, 2023

View reviewed changes

mcollina approved these changes Sep 4, 2023

View reviewed changes

sirenkovladd added 2 commits September 12, 2023 00:55

Merge remote-tracking branch 'origin/master' into fix-benchmark

3b53e5b

update tinybench

851b5ec

Uzlopak changed the title ~~Make the benchmark more useful~~ Improve benchmarks Sep 12, 2023

make benchmark return promise

6e7006c

Uzlopak merged commit f0eb74b into fastify:master Sep 12, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve benchmarks #258

Improve benchmarks #258

sirenkovladd commented Sep 3, 2023

sirenkovladd commented Sep 3, 2023

Uzlopak left a comment

Uzlopak commented Sep 3, 2023

sirenkovladd commented Sep 3, 2023

sirenkovladd commented Sep 3, 2023

Uzlopak left a comment

Uzlopak Sep 4, 2023

Uzlopak Sep 4, 2023

mcollina left a comment

kibertoad commented Sep 4, 2023 •

edited

kibertoad commented Sep 4, 2023 •

edited

mcollina commented Sep 4, 2023

kibertoad commented Sep 4, 2023 •

edited

kibertoad commented Sep 4, 2023

Uzlopak commented Sep 4, 2023

Uzlopak commented Sep 4, 2023

mcollina left a comment

sirenkovladd commented Sep 4, 2023

sirenkovladd commented Sep 11, 2023

kibertoad commented Sep 11, 2023

Uzlopak commented Sep 11, 2023

sirenkovladd commented Sep 12, 2023

Uzlopak commented Sep 12, 2023

sirenkovladd commented Sep 12, 2023

Uzlopak commented Sep 12, 2023

Uzlopak commented Sep 12, 2023

sirenkovladd commented Sep 12, 2023

	const res = new Response(req, () => resolve())
	const res = new Response(req, resolve)

Improve benchmarks #258

Improve benchmarks #258

Conversation

sirenkovladd commented Sep 3, 2023

Checklist

sirenkovladd commented Sep 3, 2023

Uzlopak left a comment

Choose a reason for hiding this comment

Uzlopak commented Sep 3, 2023

sirenkovladd commented Sep 3, 2023

sirenkovladd commented Sep 3, 2023

Uzlopak left a comment

Choose a reason for hiding this comment

Uzlopak Sep 4, 2023

Choose a reason for hiding this comment

Uzlopak Sep 4, 2023

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

kibertoad commented Sep 4, 2023 • edited

kibertoad commented Sep 4, 2023 • edited

mcollina commented Sep 4, 2023

kibertoad commented Sep 4, 2023 • edited

kibertoad commented Sep 4, 2023

Uzlopak commented Sep 4, 2023

Uzlopak commented Sep 4, 2023

mcollina left a comment

Choose a reason for hiding this comment

sirenkovladd commented Sep 4, 2023

sirenkovladd commented Sep 11, 2023

kibertoad commented Sep 11, 2023

Uzlopak commented Sep 11, 2023

sirenkovladd commented Sep 12, 2023

Uzlopak commented Sep 12, 2023

sirenkovladd commented Sep 12, 2023

Uzlopak commented Sep 12, 2023

Uzlopak commented Sep 12, 2023

sirenkovladd commented Sep 12, 2023

kibertoad commented Sep 4, 2023 •

edited

kibertoad commented Sep 4, 2023 •

edited

kibertoad commented Sep 4, 2023 •

edited