Use the new ZPP API in ext/ftp #3041
Conversation
| Z_PARAM_RESOURCE(z_ftp) | ||
| Z_PARAM_LONG(size) | ||
| Z_PARAM_OPTIONAL | ||
| Z_PARAM_ZVAL_DEREF_EX(zresponse, 0, 1) |
There was a problem hiding this comment.
The tests are failing here. What macro should we use?
|
The Fast ZPP API improves performance at the cost of larger binaries. I'm not convinced that the new API should be applied everywhere, but only where it makes sense (i.e. for frequently called functions which execute very fast), what probably isn't the case for (most) FTP functions since their performance bottleneck is elsewhere. See also https://wiki.php.net/rfc/fast_zpp#proposal. |
|
@cmb69 Thanks for explaining me, I didn't know that this comes from a RFC, and why we have both syntaxes in the core. But, it only speeds certainly kinds of functions, not all of them? Because the readability is so much easier with fast zzp 👍 |
|
No, the new API would speed up all functions, but if the function takes a long time per se (that is, without the parameter parsing) the gain is minimal (I'd say neglectible). For otherwise fast functions, the gain can be impressive, though (cf. 0bf11d1). |
|
@cmb69 So should I close this PR or wait for another reviewer? |
|
IMO the move to the fast ZPP API is not an improvement in this case, if not even a worsening. It would not affect the performance of the FTP operation, while affecting the binary size. Thanks. |
|
@carusogabriel thanks for the PR in first place, and for the prudent handling. Keep up the good work! Thanks. |
No description provided.