php
perf: optimize types
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.
| Benchmark Name | ns/op main | ns/op this PR | Faster x times |
|---|---|---|---|
| BenchmarkBool-20 | 264.5 | 156.5 | 1.69 |
| BenchmarkInt-20 | 311.6 | 162.6 | 1.92 |
| BenchmarkFloat-20 | 367.1 | 174.4 | 2.11 |
| BenchmarkString-20 | 169.5 | 165.1 | ~1 |
| BenchmarkInternedString-20 | 162.4 | 160.3 | ~1 |
| BenchmarkEmptyMap-20 | 223.0 | 98.04 | 2.27 |
| BenchmarkMap5Entries-20 | 4229 | 1710 | 2.47 |
| BenchmarkAssociativeArray5Entries-20 | 4088 | 1684 | 2.43 |
| BenchmarkSlice5Entries-20 | 3379 | 853.2 | 3.96 |
| BenchmarkMap50Entries-20 | 38023 | 13524 | 2.81 |
@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): https://github.com/php/frankenphp/blob/19d00a08e2217167918aa7c32c056c1a70f22021/testdata/integration/callable.go#L35
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: https://github.com/php/frankenphp/blob/19d00a08e2217167918aa7c32c056c1a70f22021/types.go#L374
Looking at PHPValue(), I think it might make sense to have an alternate version that directly takes the zval as value and just pass the return value directly to go.
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 PHPReturnValue function that allows directly writing into a preallocated zval. This avoids memory leaks of the zval container, like the one currently present in external worker tests on main.
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 efree the actual container, which can be easily forgotten. PHP will report leaks if the debug version is installed like in the dev.Dockerfile.
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
It's not only about arrays, I think PHPReturnValue might be the only function we need. In the usual function flow there always is a preallocated zval and we just write directly into it.
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..