Arginfo: add and use known strings for attributes#19075
Arginfo: add and use known strings for attributes#19075DanielEScherzer merged 3 commits intophp:masterfrom
Conversation
Split out from the PropertyInfo class so that known strings can also be used for attributes in a follow-up commit.
|
This is the first part of my follow-up to #18780 dealing with reducing the work of registering attributes on constants (and other things) - here, if a string is known the known string is used for attribute name and values, and
Yes I agree. Originally posted by @nielsdos in #18780 (comment) |
| return $result; | ||
| } | ||
| $include = self::PHP_80_KNOWN; | ||
| switch ($minPhp) { |
There was a problem hiding this comment.
Because gen_stub supports running on PHP 7.4, I have a todo to update that eventually
|
Looks like this makes a degradation on Symfony's Valgrind instruction count. It'd be interesting to know why. |
Hmm, curious For 64e2832 which is the last commit before this PR, https://github.com/php/php-src/actions/runs/16128700585/job/45511592322 reports DetailsBut for this PR at 99ec395, https://github.com/php/php-src/actions/runs/16155780300/job/45597800753 reports DetailsSo basically (differences in bold)
So this isn't just for symfony, this patch appears to consistently increase the instruction count? I suspect the reason is that the addition of the new known permanent strings means that any searches for a permanent string now have a couple extra strings to check against - do we care? The memory usage should be lower even if the instruction count is higher |
|
This could just be startup overhead that is being influenced. |
|
Okay - do you consider the instruction count change a blocker? |
|
Not really, but it would still be good to confirm with php-cgi's |
I couldn't figure out how to do this, would you be willing to check? |
|
I'll check tomorrow |
|
Conceptually this is okay. This difference is 1.00008923x, or 100.008922996%, which is lower than CI reports and I think that this is just noise. |
No description provided.