-
Notifications
You must be signed in to change notification settings - Fork 399
feat: frankenphp_info #1771
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
base: main
Are you sure you want to change the base?
feat: frankenphp_info #1771
Conversation
| */ | ||
| function apache_response_headers(): array|bool {} | ||
|
|
||
| function frankenphp_info(): array {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have the array shape already so it can be part of the stub?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, but good point! It might also be nice to add the FrankenPHP version, but I'm currently not sure if it's even available from inside of the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the PHPArray function, it might also make sense to instead allow direct convertion of either a slice or map to an array (unless that's something you already considered).
func SliceToPHPArray(slice []interface{})
func MapToPHPArray(map map[string]interface{})There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea on the paper. I think that we discussed this with @dunglas and the final word is that we'd like to avoid ending with a clunky API with to many functions. I'm not exactly sure if this was the conclusion of this discussion or another, but this is something that Kevin may confirm I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would save a few functions though, since on the go side you just manipulate the slice or map.
Instead of:
type PHPKeyType int
type PHPKey struct
type Array struct
func (arr *Array) SetInt
func (arr *Array) SetString
func (arr *Array) Append
func (arr *Array) getNextIntKey
func (arr *Array) Len
func (arr *Array) Atyou'd just need something like:
type Array = map[string]interface{}
func PHPArray(a Array) unsafe.Pointer
func GoArray(arr unsafe.Pointer) Array
type PackedArray = []interface{}
func PHPPackedArray(a PackedArray) unsafe.Pointer
func GoPackedArray(arr unsafe.Pointer) PackedArrayThe unfortunate thing here is that the PHP Array is a union of the 'packed' and 'unpacked' hash table. Like this you leave it up to the user to determine what they need.
Would also solve the problem that currently duplicate keys are possible in the Array struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also the problem that PHP hashmaps are ordered. But maybe this is no big deal as we cannot pass arguments by reference in our case. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it potentially looses its order in the unpacked case, but would be cleaner and more performant otherwise. Also I could just do something like this 😄
return PHPArray(&Array{
"frankenphp_version" : C.GoString(C.frankenphp_get_version().server_version),
"thread_name": thread.name(),
"thread_index": int(threadIndex),
"is_worker_thread": thread.handler.(*workerThread) != nil,
"num_threads": mainThread.numThreads,
"max_threads": mainThread.maxThreads,
})There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, if keeping the order is completely necessary, the correct data type on the go side would be an ordered map for the 'unpacked' array (sadly no built in ordered map by go)
The issues with the current implementation are mainly that duplicate keys are allowed on the go side and keys can be a mix of ints and strings. But exactly mirroring PHP Arrays probably cannot be done anyways without it getting too messy. So either mapping an array to a slice (if you need order) or a map (if you need association) seems like it makes most sense IMO.
Not sure though what others think
It should! If you encounter any blocker in this regard, I'll be happy to have a look |
|
Currently also seeing memory leaks with |
|
That's weird, PHPArray uses emalloc(), which should take care of freeing memory automatically |
|
I think the issue is that the Line 232 in 8175ae7
Line 256 in 8175ae7
Also I think there are dedicated zvals for But you are right, if allocated with emalloc, it will be arena-deallocated, so these would only leak until a worker restarts (I assume). |
|
Also the array keys in Line 183 in 8175ae7
When coming from go it would be better to use C.zend_hash_str_add(zendArray, toUnsafeChar(k.Str), (C.size_t)(len(k.Str)), zval) |
|
Hmm I might also be wrong about what specifically leaks though, might have to look into it further |
|
After looking over it again, it boils down to calling The correct way would be for PHP to handle allocation, so the GC has access to the values and can clean them up. An example would be just calling Lines 269 to 270 in 8175ae7
|
|
I'll have a look on Monday. Thank you for investigating! |
|
I'll wait with this PR until then 👍, no rush, I don't think many people have tried the extension builder yet. You might end up needing wrapper functions for all of these macros, since they allocate with the GC in mind: |
This PR is inspired by some discussions like #1743. It's currently often not trivial to determine if a PHP request is running in worker mode or to generally access info about FrankenPHP from the PHP side.
This PR adds a
frankenphp_info()function to retrieve debug info as an array from the PHP side. Still not sure about all the information that could be relevant.The extension generator definitely helped building this function 👍 (assuming
PHPArraycan be safely used already)