Mitata benchmark #548
Mitata benchmark #548
Conversation
dosubot
bot
added
size:XL
README.md
Outdated
| ║ EdDSA - fast-jwt (sync with cache) │ 10000 │ 202628.41 op/sec │ ± 3.12 % │ + 3080.69 % ║ | ||
| ╚═════════════════════════════════════╧═════════╧══════════════════╧═══════════╧═════════════════════════╝ | ||
| ``` | ||
| See [BENCHMARKS.md](./benchmarks/BENCHMARKS.md) |
There was a problem hiding this comment.
why don't we name the file README so when you navigate to the folder it shows immediately?
benchmarks/decode.mjs
Outdated
| } | ||
|
|
||
| runSuites().catch(console.error) | ||
| if (fileURLToPath(import.meta.url) === process.argv[1]) runSuites().catch(console.error) |
There was a problem hiding this comment.
you're probably looking for import.meta.filename (please beware of compatibility though)
benchmarks/sign.mjs
Outdated
| } | ||
|
|
||
| runSuites().catch(console.error) | ||
| if (import.meta.filename === process.argv[1]) runSuites().catch(console.error) |
There was a problem hiding this comment.
if we keep .catch-ing errors in this way, are we not potentially hiding errors and therefore risk that they go unnoticed as they did already?
benchmarks/auth0.mjs
Outdated
| } | ||
|
|
||
| await saveLogs('auth0') | ||
| } | ||
|
|
||
| runSuites().catch(console.error) | ||
| runSuites() |
There was a problem hiding this comment.
nit: let's take this one step further. top level await has been here for a long time, let's use it!
benchmarks/update_benchmarks.mjs
Outdated
| ${verifyBenchmark.map(printDetail).join('\n')} | ||
| ` | ||
|
|
||
| await writeFile(new URL('README.md', import.meta.url), pageMarkdownContent, 'utf8') |
| [`${algorithm} - jsonwebtoken (async)`]: function (done) { | ||
| jsonwebtokenSign(payload, privateKey, { algorithm, noTimestamp: true }, done) | ||
| [`${algorithm} - jsonwebtoken (async)`]: async function () { | ||
| return new Promise(resolve => jsonwebtokenSign(payload, privateKey, { algorithm, noTimestamp: true }, resolve)) |
There was a problem hiding this comment.
think about what you're doing here and if this change really makes sense :) this was necessary because you were dealing with callbacks. now you have promises, so none of this is necessary
There was a problem hiding this comment.
this is true for other node-rs / fast-jwt which have promise API indeed, and do not need the new Promise(etc..) ; but in this specific case you highlighted of jsonwebtoken lib, I only see the sign function overloads with/without callback. what are you suggesting?
There was a problem hiding this comment.
when the callback based version is needed, will need to stick to the existing pattenr, but it looks to me like there are no more occurrences of that pattern.
| [`${algorithm} - @node-rs/jsonwebtoken (sync)`]: function () { | ||
| nodeRsSignSync({ data: payload }, privateKey, { algorithm: Algorithm[algorithm.toUpperCase()] }) | ||
| }, | ||
| [`${algorithm} - @node-rs/jsonwebtoken (async)`]: function (done) { | ||
| nodeRsSign({ data: payload }, privateKey, { algorithm: Algorithm[algorithm.toUpperCase()] }).then(() => done()) | ||
| [`${algorithm} - @node-rs/jsonwebtoken (async)`]: async function () { |
There was a problem hiding this comment.
strictly speaking, async is not required either unless something's awaited in the body of the function, but ok it's a small detail
Closes #543
benchmark:updateto generate it