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
Improve benchmarks #258
Improve benchmarks #258
Conversation
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am -1 of a self implemented benchmark.js
use alternatively tinybench, if you dislike benchmark.js. it is also used by vitest. |
Cool, I'll switch to a tinybench if you don't mind) |
40b9218
to
e46d4d7
Compare
❯ 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 │
└─────────┴──────────────────────────────────┴─────────────┴────────────────────┴───────────┴─────────┘ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve, but prefer that a core maintainer OKs the switch to tinybench.
benchmark/benchmark.js
Outdated
.add('Response writeHead end', async function () { | ||
const req = new Request(mockReq) | ||
await new Promise((resolve) => { | ||
const res = new Response(req, () => resolve()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const res = new Response(req, () => resolve()) | |
const res = new Response(req, resolve) |
benchmark/benchmark.js
Outdated
.add('Response write end', async function () { | ||
const req = new Request(mockReq) | ||
await new Promise((resolve) => { | ||
const res = new Response(req, () => resolve()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const res = new Response(req, () => resolve()) | |
const res = new Response(req, resolve) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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? |
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 |
I have not done a scientific study on this, it's more about in-the-field examples. 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.
They actually force it: https://github.com/tinylibs/tinybench/blob/v2.5.0/src/utils.ts#L31 |
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. |
Oooof. Yes, that seems unnecesarily restrictive. Are there any better alternatives? |
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 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. |
Lol. isAsyncFunction looks like code i wrote. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, ok
|
Do you have any blocks to merge PR? |
@sirenkovladd Shouldn't a new version of tinybench be released first? I think the only blocker was the hard requirement for |
@sirenkovladd |
FYI, done |
Please run a final time the benchmarks and post them here, please |
master
PR
|
Are you happy :)? |
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. |
|
Add async benchmarks
use process.hrtime()
Checklist
npm run test
andnpm run benchmark
and the Code of conduct