Performance regression with fastify.head method #4533
Comments
|
I think the compromise made on startup with the change is a good tradeoff if we look for consistency in the behavior of the framework (in this case not calling the Doing a quick assessment, I'm assuming the removal/check of the silent sibling But happy to hear more thoughts about it 🙂 |
|
I can confirm this performance bottleneck. You also need the wildcard option set to false. You can basically reproduce it with this: 'use strict'
const path = require('path')
const fastify = require('fastify')({ logger: { level: 'trace' } })
fastify
.register(require('..'), {
// go crazy and expose big fat node_modules folder.
root: path.join(__dirname, '../node_modules'),
wildcard: false,
})
.listen({ port: 3000 }, err => {
if (err) throw err
})Setting wildcard to true avoids that issue and the server spins up immediatly. Also the memory footprint is horrible with wildcards: false as each file gets its own route. |
|
I guess it is worth mentioning that originally I faced it with https://github.com/nestjs/serve-static/blob/master/lib/loaders/fastify.loader.ts#LL33C38-L33C38 So maybe it makes sense to fix usage in nestjs |
|
I believe the calculation time exist anyway. It looks like there is a regression because the works moved from When you check with the overall time, it should not be large difference. {
"name": "repo",
"version": "1.0.0",
"description": "",
"main": "index.mjs",
"author": "",
"license": "ISC",
"dependencies": {
"@fastify/static": "^6.6.1",
"fastify41": "npm:fastify@4.1.0",
"fastify42": "npm:fastify@4.2.0"
}
}import Static from '@fastify/static'
import Fastify41 from 'fastify41'
import Fastify42 from 'fastify42'
import { dirname, join } from 'path'
import { fileURLToPath } from 'url'
console.time('fastify 4.1.0 warmup')
const fastify41 = Fastify41()
fastify41.register(Static, {
root: join(dirname(fileURLToPath(import.meta.url)), 'node_modules'),
wildcard: false
})
await fastify41.ready()
console.timeEnd('fastify 4.1.0 warmup')
// fastify 4.1.0 warmup: 1.205s
console.time('fastify 4.2.0 warmup')
const fastify42 = Fastify42()
fastify42.register(Static, {
root: join(dirname(fileURLToPath(import.meta.url)), 'node_modules'),
wildcard: false
})
await fastify42.ready()
console.timeEnd('fastify 4.2.0 warmup')
// fastify 4.2.0 warmup: 1.056sTime of Route register on fastify@4.1.0Time of Route register on fastify@4.2.0 |
|
I think we can speed wildcard up quite a bit by processing the files when they are ready and not all at once. |
|
Tbh, the wildcard option in fastify-static has the wrong name and behavour. I quote:
|
If this is the behavior, we can fix handle it and return a 404 instead. It's not hard. Have y got a reproduction?
There are valid cases for not specifying a wildcard, most specifically if you are developing an SPA and you want everything to be redirected to the same entry route. |
|
I'm ok to change the name of the option. |
|
I have investigated this problem a bit. |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Prerequisites
Last working version
4.1.0
Stopped working in version
4.2.0
Node.js version
18.12.0
Operating system
macOS
Operating system version (i.e. 20.04, 11.3, 10)
13.0
💥 Regression Report
After merging #4052
fastify.headmethod takes a much more time.Previously it took less than 1ms, after this PR it takes 50+ ms.
We are using this method there https://github.com/fastify/fastify-static/blob/master/index.js#L352-L354 and with a large amount of files, it takes a lot of time to start the server.
Steps to Reproduce
You can just use the console.time around https://github.com/fastify/fastify-static/blob/master/index.js#L352-L354
Expected Behavior
I think the performance regression of adding a route should be much lower in this case
The text was updated successfully, but these errors were encountered: