dunglas / frankenphp

🧟 The modern PHP app server
https://frankenphp.dev
MIT License
6.96k stars 244 forks source link

perf: Remove all Cgo handles #1073

Closed AlliBalliBaba closed 1 month ago

AlliBalliBaba commented 1 month ago

This PR aims to improve handle performance as discussed in #934

Instead of optimizing handles, they are completely removed and replaced by passing the index of the current thread This makes it so we can store a slice of phpThreads on the go side and have more potential control over thread creation/destruction/metrics.

The performance impacts are similar to #934.

A quick worker benchmark: wrk -t 4 -c 150 -d 60 on WSL bookworm-Docker 20 CPU cores 40 threads

without this PR: ~57000 req/s with this PR: ~65000 req/s

Note that this benchmark was done with php_import_environment_variables disabled (I plan to optimize that it in a separate PR).

This PR comes with some significant refactoring (especially in worker.go) so I hope it doesn't conflict too much with other potential PRs.

AlliBalliBaba commented 1 month ago

I realized that you can also create go flamegraphs with go tool pprof. I'm creating them like this in the dev.Dockerfile:

# installation
mkdir /usr/local/src/flamegraph && \
cd /usr/local/src/flamegraph && \
git clone https://github.com/brendangregg/FlameGraph.git && \
cd /usr/local/src/flamegraph/FlameGraph

# create a 20s flamegraph.svg in project dir
go tool pprof -raw -output=cpu.txt 'http://localhost:2019/debug/pprof/profile?seconds=20' && \
./stackcollapse-go.pl cpu.txt | ./flamegraph.pl > /go/src/app/flame.svg

Flamegraph after this PR: flame-no-handles (M12 comes from import_environment_variables and scales linearly with the amount of environment variables)

AlliBalliBaba commented 1 month ago

@withinboredom I also noticed that with the current settings of minWorkerErrorBackoff=10ms the failing-worker.php test will sometimes crash due to consuming too much memory ('core dump', also in the main branch).

Changing this to 100ms fixes the issue. Still it's weird that it would start consuming so much memory, maybe it's spawning too many goroutines?

dunglas commented 1 month ago

By the way, I would like to contact you privately @AlliBalliBaba, I couldn't find a way to contact you, would you mind sending a mail or a DM on any social network?

dunglas commented 1 month ago

@AlliBalliBaba I simplified the code, rebased and renamed a function to match our CS in 756675f (#1073). Let's merge if it works for you.

AlliBalliBaba commented 1 month ago

Sounds good to me 👍

dunglas commented 1 month ago

Thank you @AlliBalliBaba!!

AlliBalliBaba commented 1 month ago

@dunglas fyi I sent you mail on kevin@dunglas.fr