fastify-plugin
fastify-plugin copied to clipboard
fastify
Validate if a rewrite in TS has any performance penalty
Prerequisites
- [X] I have written a descriptive issue title
- [X] I have searched existing issues to ensure the feature has not already been requested
🚀 Feature Proposal
Maintaining types is hard and error-prone. I opened this issue to make an experiment if a rewrite of this module in TS has any performance penalty. If this experiment succeeds, we can simplify contributions and don't have to maintain TS types separately.
- [ ] Rewrite in TS without breaking changes
- [ ] Validate the performance of JS and TS versions.
@kibertoad
we can simplify contributions and don't have to maintain TS types separately.
is this the truth?
I'm not a blocker here, but I want to say that I'm out of all the packages written in TS
There is a use case for everything. I dont see typescript here as having any benefit. Its like a project on purpose uses hand crafted assembler to ensure high performance and somebody suggests C++ because it is more modern.
I am personally a big fan of typescript and use it in projects regularly. But I am openly against using typescript in any of our packages. And tbh. I am not convinced, that the overhead of controlling if the output of our code is what we wanted in the first place is worth it. Also after an update of typescript, we would need to check again if the code changed in a significant way.
And I am not convinced that rewriting fastify-plugin will solve the original issue in #217 . You have still the same issue because you have to do the same work.
Let's allow the experiment to take place before labeling it as a failure. No long-term decision was made here. @kibertoad wants to give it a shot.
I asked kibertoad regarding your assignment of this task to him and your claim that he wanted to "give it a shot". Actually i asked him, if there is maybe a misunderstanding, because he just wrote that he would test and benchmark if there is a solution in typescript.
I will remove the assignment of kibertoad to this task, because i think it is a misunderstanding. Also i want to avoid the impression, that if he doesnt supply a PR or a fork with the desired typescript implementation that you think we kind of block this feature request from being assessed.
You are free to assign this issue to yourself and implement a typescript rewrite so that kibertoad and others can review it.
Also kibertoad is of course free to pick this task if he wants to do the heavy lifting.
I am happy to write benchmark for this task, but not sure when I'll find time for it
I believe it may not be worthwhile to pursue further investigation if the majority is opposed to it. My rationale for assigning @kibertoad to this issue was based on their comment in https://github.com/fastify/fastify-plugin/issues/217#issuecomment-1637087416. However, it is possible that I misunderstood their perspective. My motivation has waned, and I will close this issue.
I am generally curious where this might go and especially curious to see perf implications of this change. let's not close it just yet. I'll contribute what I can
I believe it may not be worthwhile to pursue further investigation if the majority is opposed to it.
I must say that a fastify-plugin-ts could make everyone happy:
- the experimentation will take place
- the data and users' feedback will show if it works better
This package is quite feature-complete, so it does not change so often (the last feature is almost 1 year old)
For the sake of the experiment I would like to give it a shot to this with a fastify-plugin-ts
Hey! I made a fork with my attempt on migrating the plugin to typescript (still a wip) if you would like to give it a review would be great.
@StarpTech @kibertoad
Based on the experiment (great work @andersonjoseph btw 🚀 ) I do not foresee too many performance regressions, but I'm still curious. Did you have the chance to run any benchmarks?
For this, I'm more about the DX 😅
How do you want to benchmark something which is basically run a limitted amount of times at startup
I was thinking something simpler as just loading X arbitrary plugins to fastify apps, and compare (not necessarily statistically valid).
e.g. I did this just as a quick experiment:
const fp = require('fastify-plugin')
const fpts = require('fastify-plugin-ts')
const Benchmark = require('benchmark')
const fastify = require('fastify')
const suite = new Benchmark.Suite()
suite
.add(
'Fastify Plugin',
async function () {
const app = fastify()
for (let i; i < 10; ++i) {
app.register(
fp((instnace, opts, done) => done(), {
name: i
})
)
}
await app.ready()
await app.close()
},
{
minSamples: 100
}
)
.add(
'Fastify Plugin TS',
async function () {
const app = fastify()
for (let i; i < 10; ++i) {
app.register(
fpts((instnace, opts, done) => done(), {
name: i
})
)
}
await app.ready()
await app.close()
},
{
minSamples: 100
}
)
.on('cycle', function (event) {
console.log(String(event.target))
})
.on('complete', function () {
console.log('Fastest is ' + this.filter('fastest').map('name'))
})
.run({
async: true
})
Too much room for improvement, for sure. We are interested in the ready being called, but sadly we need to pay the bill for the close one due to a memory leak in http module from Node.
So far, the numbers looks similar:
Fastify Plugin x 26,146 ops/sec ±1.57% (185 runs sampled)
Fastify Plugin TS x 26,015 ops/sec ±1.67% (185 runs sampled)
But the script is really naive, many optimizations can be done to fully target what we want to really measure
Did you have the chance to run any benchmarks
I haven’t run any benchmarks yet, but I was thinking of doing something similar to your script, @metcoder95. But I wasn't sure if that’s the best way to measure performance
But I wasn't sure if that’s the best way to measure performance
It should test what we are looking for, which is the internals of the fastify-plugin, which are constituted of mostly the callback part. That's why app.ready is the key one here.
Although, the script can be optimized, especially finding a way to reset the app so a new app and its plugins are loaded so the suite just waits for the app to be ready with app.ready.
Please feel free to adapt it and play with it as you need 👍
The comparison should consider the fp's options imho:
- check fastify version
- check required dependancies
- check required decorators
I added some benchmarks to the repo, I tried to optimize the script to only measure app.ready and also added benchmarks for fp's options. Here are the results on my machine:
Benchmark for simple case
Fastify Plugin x 98,947 ops/sec ±2.54% (79 runs sampled)
Fastify Plugin Typescript x 98,539 ops/sec ±1.91% (85 runs sampled)
Benchmark for fastify version checking
Fastify Plugin x 96,928 ops/sec ±2.53% (78 runs sampled)
Fastify Plugin Typescript x 97,231 ops/sec ±1.88% (82 runs sampled)
Benchmark for fastify dependencies checking
Fastify Plugin x 100,273 ops/sec ±1.89% (80 runs sampled)
Fastify Plugin Typescript x 99,840 ops/sec ±1.68% (83 runs sampled)
Benchmark for fastify decorators checking
Fastify Plugin x 98,176 ops/sec ±2.01% (80 runs sampled)
Fastify Plugin Typescript x 99,913 ops/sec ±1.75% (84 runs sampled)
here's the PR: fastify-plugin-ts/#2
So from the benchmarks, i'd say there's minimal improvement for all the extra complexity?
@Fdawgs isn't that the other way around? if TS doesn't cause perf regression, it should be the way to go due to the increased simplicity of the maintenance?
@jsumners
- You don't need to remember to manually sync types, they are always perfectly accurate;
- You catch more stuff at compilation/linting time
Assuming that TS types already exist and need to be maintained, writing in TS is less effort than writing just the types by hand
@Uzlopak Should there be any?
See the original description: Maintaining types is hard and error-prone. I opened this issue to make an experiment if a rewrite of this module in TS has any performance penalty
To be honest: There shouldnt be any performance difference because the generated code should be close to our current code. So to be honest when i reviewed this PR few months ago, I double checked the code for being congruent to the code base.
But regarding the aspect of introducing typescript... I would prefer javascript. When we use typescript we need also to review if the generated code is still what we expect and dont introduce a regression by simply updating typescript.
To achieve this we need to check in the generated files. So if typescript modifies those files we can track it and see what effectively got changed.
And that is some overhead.
See the original description:
Maintaining types is hard and error-prone. I opened this issue to make an experiment if a rewrite of this module in TS has any performance penalty
My opinion is that it is unnecessary effort. Look at the commit history https://github.com/fastify/fastify-plugin/commits/master/. It's primarily documentation and house keeping updates. This module very rarely needs a code change, and that's usually just to bump some version checks. I don't see any concrete reason to complicate the development of this module simply because someone likes TypeScript instead of JavaScript.
@jsumners
- You don't need to remember to manually sync types, they are always perfectly accurate;
- You catch more stuff at compilation/linting time
Assuming that TS types already exist and need to be maintained, writing in TS is less effort than writing just the types by hand
Maybe those things are true. But I doubt the effort being described is much of anything given my above statement.
To achieve this we need to check in the generated files. So if typescript modifies those files we can track it and see what effectively got changed.
To be fair, if you do not target to old version of ECMAScript. Most of the code would remain the same after compile. It just serve like stripping the types.
This module very rarely needs a code change, and that's usually just to bump some version checks.
Yes, most of the plugins are on the same situation.
If anyone would like to start the refactoring, it should be start with the core first. There is not much to do on plugins because
- Plugins is always constraints by the core
- Plugins is mostly few lines of codes (not much difference in JS and TS)
One can argue it is better to start with the simple first, but once the core is refactored. It highly possible we need to update all plugins again.
