actix-web icon indicating copy to clipboard operation
actix-web copied to clipboard

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

Open joelwurtz opened this issue 1 year ago • 2 comments

PR Type

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

PR Checklist

  • [x] Tests for the changes have been added / updated.
  • [x] Documentation comments have been added / updated.
  • [x] A changelog entry has been made for the appropriate packages.
  • [x] Format code with the latest stable rustfmt.
  • [x] (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 ?

joelwurtz avatar Dec 13 '24 14:12 joelwurtz

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

robjtede avatar Dec 29 '24 15:12 robjtede

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.

joelwurtz avatar Dec 30 '24 11:12 joelwurtz