-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Remove cache slot from ZEND_VERIFY_RETURN_TYPE and arg RECV opcodes #18258
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
This is essentially redundant since adding the interned string CE cache, right?
This is essentially redundant since adding the interned string CE cache, right?
Indeed. I'm currently benching and will update the OP when I have results.
077c18a to
3a674fe
Compare
Nice simplification. We've briefly mentioned the possibility of adding a class cache for RECV and VERIFY_RETURN_TYPES in another issue. This would require a single slot, where the CE of the last value successfully type checked would be stored. This is mostly useful for union and intersection types, improving consecutive calls with the same class type. If there are concerns of slowing down non-union/intersection type checks, we could create a custom specialized handler that is only used when a cache slot is actually allocated. This might be worth exploring as well.
Nice simplification. We've briefly mentioned the possibility of adding a class cache for RECV and VERIFY_RETURN_TYPES in another issue. This would require a single slot, where the CE of the last value successfully type checked would be stored. This is mostly useful for union and intersection types, improving consecutive calls with the same class type. If there are concerns of slowing down non-union/intersection type checks, we could create a custom specialized handler that is only used when a cache slot is actually allocated. This might be worth exploring as well.
That should be done separately from this PR.
Where can I find the discussion? Is it a situation that really occurs a lot in practice?
#18189 (comment) mentions it. It would likely be a small optimization.
#18189 (comment) mentions it. It would likely be a small optimization.
Oh right, I can take a look at that after this goes through. (The implementation and handling of cache slots would be different to this anyway)
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 can't do a deep review now, but since this removes more than adds, I support this.
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.
Nice!
This looks good 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.
LGTM as well
9a691fe to
be0132e
Compare
Uh oh!
There was an error while loading. Please reload this page.
Since #7336 the CE cache is always used even if Opcache is not loaded.
This means that the cache slot is almost fully redundant, except for self/parent/static. Removing this was hinted at in #7336 (review) but was never done. This patch removes this finally.
Valgrind in CI shows a I-count reduction of 0.13% without JIT and 0.08% with JIT for Symfony. 0.06% and 0.08% reduction for Wordpress respectively without and with JIT.
Real-time benchmarks on an i7-1185G7:
Symfony Benchmark (public/index.php)
Ran with
-T 10,45WordPress Benchmark (index.php)
Ran with
-T 5,20So I see very slight improvements and no regressions.
Note that tracing run has a higher baseline due to initial JIT overhead, but increasing the number of runs made the machine throttle which ruined the stability of the benchmark.