Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Etag performance by caching fs.stats #36

Open
2 tasks done
Uzlopak opened this issue Jan 31, 2023 · 5 comments
Open
2 tasks done

Improve Etag performance by caching fs.stats #36

Uzlopak opened this issue Jan 31, 2023 · 5 comments

Comments

@Uzlopak
Copy link
Contributor

Uzlopak commented Jan 31, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

馃殌 Feature Proposal

Cache fs.stats result of files to improve performance. For this we could use an lru. If we know that the exposed files and folders are not changed we can store them without issues. But if we know that the files are getting modified it would make sense to use fs.watch and invalidate the lru. But it would be preferable to assume that e.g. fastify-static is running on a system were the files are not changed.

Motivation

I think the performance bottleneck of this library is the fs.stars call. By caching we could improve the performance.

Example

No response

@climba03003
Copy link
Member

climba03003 commented Jan 31, 2023

I would wait for nodejs/node#46345

I am thinking, are we open into race-condition of read file.
Since, fs.stat and fs.createReadStream is called in different time. The stat data maybe out-dated during the two operation.

If true, would FileHandle helps in this case? Because a fileHandle always point to the same file descriptor for it's operation.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 31, 2023

Is a filehandle not created on every request then?

But you are right. It is kind of racy if the file is changing. Thats why I would suggest this for non changing filesystems. Like when you do wildcard: false in fastify-static. Maybe this has to be done in fastify-static?

@climba03003
Copy link
Member

climba03003 commented Jan 31, 2023

Is a filehandle not created on every request then?

I expect it to be created on every request. Caching FileHandle or improper close would open in memory-leak.
And I would expect FileHandle actually hold the File it open to prevent race-condition.
FYI, the original send package support node@>= 0.8.0 and that's why it is not possible to use FileHandle. But, we can.

Maybe this has to be done in fastify-static?

Yes, for me. Caching is better to be done on higher level.

@mcollina
Copy link
Member

mcollina commented Feb 1, 2023

I'm not sure what's the right level to put caching. I think we might want to do in this module, but that would require a significant amount of API changes.

@serzero2007
Copy link

We can add stats argument to options to override fs.stats function in the new API here #15
By overriding stats function @Uzlopak will have ability to do whatever he wants with its calls. The same can be done for fs.createReadStream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants