Skip to content

Refactor: remove C thread pool#841

Closed
withinboredom wants to merge 7 commits into
mainfrom
refactor/main-thread
Closed

Refactor: remove C thread pool#841
withinboredom wants to merge 7 commits into
mainfrom
refactor/main-thread

Conversation

@withinboredom
Copy link
Copy Markdown
Member

@withinboredom withinboredom commented Jun 2, 2024

This PR builds on #837 and moves thread management from C to Go. There appears to be no measurable performance impact, but it makes it easier to reason about thread management. It could also allow dynamically starting/stopping PHP threads based on requests (scale to zero, even) in the future, which could be extremely useful on the edge.

Note that I had to remove -wAll from CFLAGS due to a warning from go's generated code. See golang/go#6883

@withinboredom withinboredom added enhancement New feature or request go Pull requests that update Go code labels Jun 2, 2024
@withinboredom withinboredom requested a review from dunglas June 2, 2024 10:12
@withinboredom withinboredom self-assigned this Jun 2, 2024
@dunglas
Copy link
Copy Markdown
Member

dunglas commented Jun 2, 2024

I fear that we'll have problems with signals (used for PHP timeouts) if we let Go starting the threads instead of using pthread directly. I don't remember the details, so we'll have to double check, it's maybe possible but I'm not sure.

https://pkg.go.dev/os/signal#hdr-Go_programs_that_use_cgo_or_SWIG

@dunglas
Copy link
Copy Markdown
Member

dunglas commented Jun 2, 2024

To be more clear: Go doesn't allow to change the signal mask of a thread started by it (it's possible for thread started by C however), and PHP needs a custom signal mask for timeouts handling.

That being said, moving threads creation C-side should be straightforward.

For the record, @arnaud-lb, @bukka and I looked at how we could have signal-less timeout support for PHP. Arnaud even crafted a prototype.
If we manage to do that, then we could totally get rid of threads and create a TSRM implementation tailored for goroutine. It's on the roadmap.

@withinboredom
Copy link
Copy Markdown
Member Author

withinboredom commented Jun 2, 2024

I specifically tested timeouts and didn't see any issues.

I also just finished testing fibers and it somehow works as well... I threw ~50k requests and didn't see a crash.

The reason I think this works is that we never return full control back to Go, but I do agree this needs more real-world testing (the go routines take over the thread and have go kill the thread when they are done).

@dunglas
Copy link
Copy Markdown
Member

dunglas commented Jun 2, 2024

It's explicitly stated "The non-Go code should not change the signal mask on any threads created by the Go runtime. If the non-Go code starts new threads of its own, it may set the signal mask as it pleases." in the Go docs.

Maybe should we at least open an issue on the Go bug track to check if it is safe (at least for now) or not.

I'm 100% sure that PHP changes the sigmask to set the timeout handlers.

@withinboredom
Copy link
Copy Markdown
Member Author

In theory, we never return back from C code except to perform a select in a loop; the thread is terminated upon the function completion. For the most part, Go loses control of the thread (read on for what that means, exactly).

The real problem is that eventually, Go will want to grow the stack and it will crash because it thinks it owns the thread.

I don't believe that means it's impossible or won't eventually work, but I think (for now) we can consider this "won't do"; however, I'm kinda nerding out on getting it working, so I'll continue down this path for a bit to get to know the deeper workings of this weird relationship called cgo.

@dunglas
Copy link
Copy Markdown
Member

dunglas commented Jun 2, 2024

Interesting! Anyway, starting the thread from C instead of from Go seems straightforward.

@withinboredom
Copy link
Copy Markdown
Member Author

The biggest thing to come out of this adventure has been a PoC to remove the Go -> C -> Go callbacks through a circular buffer/shared memory approach (very much not channels, more like a BufferedReader that can read arbitrary structured messages; though I'd eventually like to have the interface be channels). It's still a pretty long way from being done (the current PoC can only handle a single request at a time right now). Still, I'll create a library for it, as it's probably generally valuable for anyone wanting to pass messages back to a different go thread asynchronously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request go Pull requests that update Go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants