fastcgi: Optimize FastCGI transport #4978
Conversation
|
Oooh, I can't wait to review this!! |
|
Out of curiosity, I checked stdlib fcgi server implementation, and found original implementation is mostly a copy of the fcgi.go. For example, writeRecord, writePairs, and record reading. Searching through go issues, I found nobody mentioned anything about performance, I guess not many people use it anyway. |
|
@WeidiDeng I'd be interested in a profile to compare memory usage or a CPU benchmark to compare performance. I don't have a PHP backend installed though. Would you or someone like to do some performance testing? |
|
Latest commit changes are drastic, but in my testing environment there are no problems. Here is the benchmark using nextcloud, separated by with or without this patch, and in 2 use cases, large file upload and website browsing. |
|
@WeidiDeng Thanks for the profiles! I'm looking at them now. Interesting, so far the patch version allocates about 13% less heap memory (if I'm reading these right). Have you noticed any significant difference in req/sec? |
|
Upper one is with this patch, lower one is build from master without this patch. Using go 1.19. header.lua is setting request cookies as to simulate user browser nextcloud directories. Around 10% improvement. |
|
@WeidiDeng Am I reading the results wrong? The lower one is 3 req/sec higher than the upper one. (But they're so similar anyways.) |
|
I write a simple file server using golang fastcgi.
Previous test result is because the test bottleneck is mysql server, so there is no significant difference. In longer test, howeve req/sec difference is smaller.
|
|
@mholt updated benchmark using php_info() as output. First nginx in docker as baseline.
Then caddy.
|
|
@WeidiDeng That's awesome!! Way more useful information here I think. The profile clearly shows about 10% less memory being allocated, and only ~5s spent on GC compared to 36s without the patch. Req/sec appear to be on-par with or higher than nginx. Very good! Definitely interested in having more people test this and then getting this merged in 🎉 |
|
I've tested it with our main use, MantisBT issue tracker, and it does work flawlessly. |
|
Thanks for testing @tgulacsi ! That's great to know. I will try to review this soon. |
5f7ad2f
to
4b05b2a
Compare
There was a problem hiding this comment.
@WeidiDeng This is really great, and I'm looking forward to merging this!
There's nothing obviously wrong that I can see. I'm not a PHP guy so I'm relying on those of you with PHP apps to test it out. (This will go into a beta release first.) But it LGTM and I'd be ready to merge it any time.
Have you run this with the race detector (go run -race or go build -race)?
Thanks so much for the exciting contribution!
Also reduce some duplication
|
If we could figure out how to set the logger when the clientCloser is created, we don't have to make it allocate. A cheap way is to pass I wonder if a better way would be to create a |
|
Here's the delta/difference between the profiles you posted above for Look at the green boxes (heap, then CPU):
And for your file server profile: Heap, then CPU:
Nice job 👏 (Lots of good info about using pprof tooling here: https://github.com/google/pprof/blob/main/doc/README.md#comparing-profiles) |
Should reduce allocations
|
@WeidiDeng I pushed one more commit that refactors client, it renames FCGIClient -> client, and should have slightly fewer allocations (like, 2 fewer -- we don't allocate the client or the clientCloser I think, since they're no longer pointers). The tests pass on my machine, but it'd be good to have some more real-world PHP testing. Also run with the race detector ( I'm about ready to merge this if you are. 🙂 |
There was a problem hiding this comment.
LGTM. @WeidiDeng anything else you want to do before we merge this?
|
@mholt I'm ready for it to be merged. About race detector, that one I haven't done. But that lock is unnecessary already, since reverse proxy can't write to the body and the only time it's written is during sending request which isn't concurrent. |
|
@WeidiDeng Which lock are you referring to? (I'll merge this in a few minutes then!) |
FCGIClient sync.Mutex only used in writeRecord |
|
@WeidiDeng Looks like it was removed in this PR -- if there's no races, sounds good to me! Thank you! 😄 Please contribute again. (I will catch up to your other PRs shortly, I'm behind.) |
Hello, first of all I love Caddy! I hope this helps. THE PROBLEM: In current "83b2697" caddy version is not working the option "capture_stderr" of the directive "php_fastcgi". HOW TO TEST THE PROBLEM: If you want to test it, just create a php file with syntax error of any type and pass it over php_fastcgi. WHEN STARTED THE PROBLEM: The problem started in the transition between the "f2a7e7c" commit, and the current "83b2697". WHY STARTED THE PROBLEM: On 2022 Sept 2, it was launched the commit "83b2697" which changed some files on fastcgi.go. These changes was titled: fastcgi: Optimize FastCGI transport (caddyserver#4978). It was changed a lot of internal functions and they were replaced with a more distributed logic into separate files (which is good). On those changes was removed a conditional that evaluates "t.CaptureStderr" (line 164, f2a7e7c). This conditional set the correct logger to be passed to the client (fcgiBackend in f2a7e7c) instance. So the logger was always set to "noopLogger" (a zap.NewNop()) and no matters you set capture_stderr, it was never registered into log. HOW TECHNICALLY OCCURRED THE PROBLEM: Those changes on the "t.CaptureStderr" (line 164, f2a7e7c) behaviour, probably were done because it was assumed that the evaluation of "c.stderr" (line 244, "83b2697") on client.go (another change introduced on "83b2697") would have the same behaviour, and it is right in some way. I mean, if "c.stderr" is set, it will work as expected. The problem is that nowhere it was passed the value evaluated on "t.CaptureStderr" to the client instance created, and so, it was never evaluated on "c.stderr" (line 244, "83b2697"). THE SOLUTION: I must to say that, although it has taken me quite some time to find the solution, it is actually quite simple. For fixing it I just pass the value in the creation of the client instance, ash show in this pull request. I hope this little enhancement could help a bit to us.
Hello, first of all I love Caddy! I hope this helps. THE PROBLEM: In current "83b2697" caddy version is not working the option "capture_stderr" of the directive "php_fastcgi". HOW TO TEST THE PROBLEM: If you want to test it, just create a php file with syntax error of any type and pass it over php_fastcgi. WHEN STARTED THE PROBLEM: The problem started in the transition between the "f2a7e7c" commit, and the current "83b2697". WHY STARTED THE PROBLEM: On 2022 Sept 2, it was launched the commit "83b2697" which changed some files on fastcgi.go. These changes was titled: fastcgi: Optimize FastCGI transport (caddyserver#4978). It was changed a lot of internal functions and they were replaced with a more distributed logic into separate files (which is good). On those changes was removed a conditional that evaluates "t.CaptureStderr" (line 164, f2a7e7c). This conditional set the correct logger to be passed to the client (fcgiBackend in f2a7e7c) instance. So the logger was always set to "noopLogger" (a zap.NewNop()) and no matters you set capture_stderr, it was never registered into log. HOW TECHNICALLY OCCURRED THE PROBLEM: Those changes on the "t.CaptureStderr" (line 164, f2a7e7c) behaviour, probably were done because it was assumed that the evaluation of "c.stderr" (line 244, "83b2697") on client.go (another change introduced on "83b2697") would have the same behaviour, and it is right in some way. I mean, if "c.stderr" is set, it will work as expected. The problem is that nowhere it was passed the value evaluated on "t.CaptureStderr" to the client instance created, and so, it was never evaluated on "c.stderr" (line 244, "83b2697"). THE SOLUTION: I must to say that, although it has taken me quite some time to find the solution, it is actually quite simple. For fixing it I just pass the value in the creation of the client instance, ash show in this pull request. I hope this little enhancement could help a bit to us.
Trying to fix 3803.
I tried it on a small php site, no problem so far. Extensive testing is required.