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

Lock free handles #934

Closed
wants to merge 7 commits into from
Closed

Lock free handles #934

wants to merge 7 commits into from

Conversation

withinboredom
Copy link
Collaborator

@withinboredom withinboredom commented Jul 23, 2024

This takes the Handles from the CL I submitted to Go and applies them here. The changes are not that big to the FrankenPHP parts; though I did also remove the "smart pointer" stuff. We just need to be careful to delete handles.

Here's the performance difference for worker mode, visualized by ChatGPT after feeding it 10 benchmarks each:

frankenphp benchmarks

This is approximately a 9-13% increase in RPS using a very simple PHP script.

I also wrote about the changes to Handles on my blog

@withinboredom
Copy link
Collaborator Author

Tests are failing for some reason, I'll investigate.

@withinboredom
Copy link
Collaborator Author

I went ahead and merged #933 into this PR to run a benchmark. It looks like this makes worker/cgi-mode have the same performance characteristics.

frankenphp.c Outdated Show resolved Hide resolved
frankenphp.c Outdated Show resolved Hide resolved
frankenphp.c Outdated Show resolved Hide resolved
frankenphp.go Outdated Show resolved Hide resolved
frankenphp.go Show resolved Hide resolved
concurrent_handle.go Outdated Show resolved Hide resolved
frankenphp.go Outdated Show resolved Hide resolved
worker.go Show resolved Hide resolved
@withinboredom withinboredom changed the base branch from main to refactor-ts July 24, 2024 18:00
@withinboredom
Copy link
Collaborator Author

withinboredom commented Jul 25, 2024

Don't merge/review (unless you want to). I submitted a CL to Go and getting good feedback. Already, the CL version is 20-40% faster than this version and doesn't require any changes to the Handles api.

https://go-review.googlesource.com/c/go/+/600875

Base automatically changed from refactor-ts to main July 26, 2024 07:22
@withinboredom withinboredom marked this pull request as draft July 26, 2024 08:15
@withinboredom withinboredom marked this pull request as ready for review August 12, 2024 15:02
handles.go Show resolved Hide resolved
frankenphp.go Show resolved Hide resolved
handles.go Outdated Show resolved Hide resolved
handles.go Outdated Show resolved Hide resolved
handles_test.go Outdated Show resolved Hide resolved
@AlliBalliBaba
Copy link
Collaborator

AlliBalliBaba commented Sep 21, 2024

@withinboredom I've been experimenting a bit with passing pthread_self() instead of creating handles and am seeing similar performance improvements to your branch (going from ~40.000 rps to ~50.000 rps on my 20 core machine).

The basic idea is to keep a sync map on the go side like this:

phpThreads: [
     threadId1: {mainRequest *http.Request, workerRequest *http.Request}
     threadId2: {mainRequest *http.Request, workerRequest *http.Request}
     threadId2: {mainRequest *http.Request, workerRequest *http.Request}
     ...
]

This allows us eliminate all handles and generally feels like a simpler approach. It might also make extracting metrics easier, since we keep a reference to all requests currently going through the threads.
WDYT?

@withinboredom
Copy link
Collaborator Author

Yes! I don't think you even need a sync.Map as you can just use a slice[nbWorkers] then address the slice. It would be much more efficient and 'lock-free' so long as each worker only modifies its own slice. Also, sync.Map is pretty slow due to dirty tracking, if a sync.Map is required, it is usually faster to just do your own locks and a regular map.

@AlliBalliBaba
Copy link
Collaborator

Interesting, I didn't know sync.Map was that inefficient. I'll try making a PR once the filewatcher branch has moved forward..

@withinboredom
Copy link
Collaborator Author

sync.Map isn't necessarily inefficient, it efficiently does what it advertises, but it only is useful in certain situations, and this isn't one of them (cgo Handles uses sync.Map under the hood). For Handles, sync.Map almost always takes the "slow path" through sync.Map and not the fast path (which only happens when there is contention on the same thread, which is almost never the case for FrankenPHP).

@dunglas
Copy link
Owner

dunglas commented Oct 18, 2024

Replaced by #1073.

@dunglas dunglas closed this Oct 18, 2024
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

Successfully merging this pull request may close these issues.

3 participants