-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Use the same zend_arg_info struct for internal and user functions #19022
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
Conversation
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.
Thanks for having a look at this, I'm overall in favour of unifying those, as it makes it easier to reason about.
See some of my comments, which are mostly nits. :)
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 need to do this with a printf? Can't we just append a char and then the zend_string?
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.
You are right, but I want to avoid changes that are not directly related
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.
ACK, let's keep this for a follow-up then :)
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.
I think this can be improved to prevent a strlen computation within the smart_str_appends.
Co-authored-by: Gina Peter Banyard <girgias@php.net>
You can probably get rid of ZEND_ACC_USER_ARG_INFO
Right :) I wanted to do it, but it's not trivial because the flag is also used to signal how arg default values should be fetched. I may try to remove it later.
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.
As a follow-up, the get_default_via_ast() function should be changed to accept a zend_string to get rid of an unnecessary strlen() computation :)
This does seem to have a negative impact on the Valgrind instruction counts though. Probably because of the slightly higher memory requirement if you have the zend_string header too.
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.
Is this comment still correct?
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.
Yes this is still correct. This is also related to why we can not remove ZEND_ACC_USER_ARG_INFO immediately.
@nielsdos I believe this is purely startup/shutdown overhead. When ignoring startup/shutdown, I see absolutely zero degradation under valgrind.
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.
I don't object.
This wasn't made before to avoid the waste of memory.
Now names are going to be duplicated from .text to heap and shm.
This is not a big problem, but it's better to measure this in some way.
I've measured zero regression on wall time or valgrind icount on the symfony benchmark. I will measure the memory impact once I get back from vacations.
NB: I've also tried declaring zend strings in static storage, but there are a number of complications that make the current solution more practicable. The most annoying one is that we can't initialize a zend string directly due to the variable size member.
NB: I've also tried declaring zend strings in static storage, but there are a number of complications that make the current solution more practicable. The most annoying one is that we can't initialize a zend string directly due to the variable size member.
I know. I tried this years ago and gave up :)
With a realistic set of extensions: Core,ctype,date,dom,FFI,fileinfo,filter,gmp,hash,iconv,intl,json,lexbor,libxml,mbstring,mysqli,mysqlnd,openssl,pcre,PDO,pdo_mysql,pdo_sqlite,Phar,posix,random,Reflection,session,SimpleXML,sockets,SPL,sqlite3,standard,tokenizer,uri,xml,xmlreader,xmlwriter,Zend OPcache,zend_test,zlib I measured the memory usage and interned strings usage when running the following script: readfile("/proc/self/status"); var_dump(opcache_get_status()["interned_strings_usage"]);
Opcache interned strings:
The relative increase of VmData is not negligible, but the absolute difference (352 KiB) is not really high when considering this is likely to be shared between processes. And this is a "fixed" overhead: This does not grow proportionally to the program size or its own memory usage. Given there is zero time or instruction overhead in the symfony benchmark, this seems acceptable to me. |
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.
Should be ZEND_API if used from extensions (e.g. ext/pdo/pdo_dbh.c).
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.
Nit: Use pemalloc for system OOM safety.
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.
I don't understand this comment. It's referring to DNF but is handling only union types.
Edit: I see this comes from a moved code block. Would still be nice to clarify. /cc @Girgias
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.
Nit: I realize this was renamed to match the argument, but this is still used only for internal function arg infos. So maybe the existing name still fits?
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.
I like the unification, and it looks reasonable to me. Should probably be delayed for 8.6.
Uh oh!
There was an error while loading. Please reload this page.
Internal functions use
char*strings to represent arg names and default values. This differs from user functions, which use zend strings.Here I unify this by using the same struct,
zend_arg_infoin both function types.This simplifies accesses to arg infos. Also, in the PFAs RFC this avoids converting internal arg infos at runtime when applying an internal function.