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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(files): remove spawning a thread for each file handling, as it does more harm than actually blocking #3531

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joelwurtz
Copy link
Contributor

@joelwurtz joelwurtz commented Dec 13, 2024

PR Type

Fix/Feat (not always sure with those kind of patch)

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

I was looking at a recent comment that indicates than actix files was having bad perf, after some investigation it seems that using the sync api make it way better, than having to spawn a thread to make it async.

In my local environment i want from 11k req/s to 600k req/s (so roughly a x54 increase of performance)

It may be a problem if you only handle big files with poor fs i/io speed (so all actix web threads will be blocked).

Maybe we could still spawn this thread if the size of the file is superior to some limit ?

@robjtede
Copy link
Member

Having some threshold on the spawn might be a good idea. I can see this current patch being quite risky for high concurrency situations.

@joelwurtz
Copy link
Contributor Author

Maybe, we can provide a default strategy that tell that half of worker can do sync file serving (using a sémaphore) and allow people to change this strategy if they want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-files project: actix-files B-semver-patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants