-
Notifications
You must be signed in to change notification settings - Fork 423
perf: optimize types #2090
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?
perf: optimize types #2090
Conversation
# Conflicts: # types.c # types.go # types.h # types_test.go
# Conflicts: # types.c # types.go # types.h # types_test.go
|
@alexandre-daubois I kind of missed this in #1731, but returning the same zval here in integration tests is an invalid operation (because of GC):
The array actually needs an additional reference (via GC_ADDREF). Strangely, this was compensated by doing more heap allocations, which potentially hides values from the GC: Line 374 in 19d00a0
Looking at func PHPReturnValue(value any, return_value unsafe.Pointer)That would make memory leaks more unlikely. WDYT? func go_do_something(param *C.zval, returnValue *C.zval) {
result := // do something with param
// instead of allocating a new zval
// write directly into return_value
PhpReturnValue(unsafe.Pointer(returnValue), result)
} |
|
This branch now also adds a |
|
To be sure, is it just in case of an array or the leak could also be present in other cases? I never saw PHP report leaks because of return values and your comment only mentions arrays, that's why I'm not sure? If it is only about arrays, maybe we could have the best of both worlds for the DX which is keeping the classical return statement for scalars and maybe a new function to return arrays |
|
The leak happens in tests since PHPValue is used internally in one place. When using PHPValue in most cases the user also needs to
It's not only about arrays, I think The api just becomes: (typing this out on mobile) PHPfunction(somefunction) {
//... parse args
go_do_something(args, return_value);
}func go_do_something(arg *C.zval, returnValue *C.zval) {
transformedArg = // do something with arg...
// just write the go value directly into return_value
frankenphp.PHPReturnValue(unsafe.Pointer(returnValue), transformedArg)
}No need for RETURN_BOOL, RETURN_ARR, PHPMap, PHPString etc.. |
I had this branch lying around for quite a while waiting for #1894 to be merged. This PR makes type conversions up to 4x faster by minimizing Cgo overhead.