Skip to content
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

Remove cache slot from ZEND_VERIFY_RETURN_TYPE and arg RECV opcodes #18258

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Apr 5, 2025

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,45

JIT Version Mean Time (ms) Std Dev (ms) Min Time (ms) Max Time (ms) Speedup
Disabled Original PHP 490.7 ±1.4 488.3 493.3 1.00×
Disabled Patched PHP 486.4 ±1.3 484.6 488.8 1.01×
Tracing Original PHP 618.9 ±1.8 617.2 623.1 1.00×
Tracing Patched PHP 614.7 ±3.4 610.6 622.1 1.01×

WordPress Benchmark (index.php)

Ran with -T 5,20

JIT Version Mean Time (ms) Std Dev (ms) Min Time (ms) Max Time (ms) Speedup
Disabled Original PHP 637.8 ±2.7 633.0 641.8 1.00×
Disabled Patched PHP 633.1 ±3.0 630.3 639.5 1.01×
Tracing Original PHP 763.3 ±4.0 757.8 770.8 1.00×
Tracing Patched PHP 758.2 ±3.7 754.4 766.2 1.01×

So 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.

@nielsdos nielsdos changed the title Remove cache slot from ZEND_VERIFY_TYPE and arg RECV opcodes Remove cache slot from ZEND_VERIFY_RETURN_TYPE and arg RECV opcodes Apr 6, 2025
@iluuu1994
Copy link
Member

This is essentially redundant since adding the interned string CE cache, right?

@nielsdos
Copy link
Member Author

nielsdos commented Apr 6, 2025

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.

@iluuu1994
Copy link
Member

iluuu1994 commented Apr 6, 2025

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.

@nielsdos
Copy link
Member Author

nielsdos commented Apr 6, 2025

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?

@nielsdos nielsdos marked this pull request as ready for review April 6, 2025 11:53
@nielsdos nielsdos requested a review from dstogov as a code owner April 6, 2025 11:53
@nielsdos nielsdos requested review from arnaud-lb and iluuu1994 April 6, 2025 11:53
@iluuu1994
Copy link
Member

#18189 (comment) mentions it. It would likely be a small optimization.

@nielsdos
Copy link
Member Author

nielsdos commented Apr 6, 2025

#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)

Copy link
Member

@dstogov dstogov left a 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.

Copy link
Member

@arnaud-lb arnaud-lb left a 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.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants