zend_execute: Mark `zend_get_executed_*()` as `__attribute__((pure))` by TimWolla · Pull Request #18998 · php/php-src · GitHub | Latest TMZ Celebrity News & Gossip | Watch TMZ Live
Skip to content

zend_execute: Mark zend_get_executed_*() as __attribute__((pure)) #18998

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

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

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Jul 1, 2025

These functions do not modify the state of the program and depend on thread-safe global variables only.


This is a possible precursor to #18995.

@TimWolla
Copy link
Member Author

TimWolla commented Jul 1, 2025

CI Benchmark indicates a slight slowdown. Can anyone with a “reliable CPU” do another non-valgrind check to see if this actually resulted in regressions? My CPU is notoriously bad at benchmarking.

@TimWolla TimWolla requested review from iluuu1994 and nielsdos July 1, 2025 13:24
@iluuu1994
Copy link
Member

iluuu1994 commented Jul 1, 2025

❯ b 50 --mode=cycles 'sapi/cli/php-old -n benchmark/repos/symfony-demo-2.2.3/public/index.php' 'sapi/cli/php-new -n benchmark/repos/symfony-demo-2.2.3/public/index.php'
Old:       1.694 G ± 991.360 k (0.059%) 
New:       1.696 G ± 1.041 M (0.061%)
Diff:      2.264 M (0.134%)
T-test:    5.679
P-value:   0.001

First test looks to be slower.

Edit: Second run approx. confirms.

❯ b 50 --mode=cycles 'sapi/cli/php-old -n benchmark/repos/symfony-demo-2.2.3/public/index.php' 'sapi/cli/php-new -n benchmark/repos/symfony-demo-2.2.3/public/index.php'
Old:       1.697 G ± 1.114 M (0.066%)   
New:       1.698 G ± 957.368 k (0.056%)
Diff:      1.337 M (0.079%)
T-test:    3.282
P-value:   0.008

@TimWolla
Copy link
Member Author

TimWolla commented Jul 1, 2025

image

@iluuu1994
Copy link
Member

Can relate

@TimWolla TimWolla force-pushed the zend-get-executed-scope branch from b26e8a6 to 2fb856e Compare July 2, 2025 07:01
@nielsdos
Copy link
Member

nielsdos commented Jul 2, 2025

Coming from your other PR, I see now why you submitted this.
I don't know why this worsens performance, but indeed the benchmark seems to indicate now a deviation higher than noise.
It could be many different reasons, from alignment of functions/loops to bad compiler spill/rematerialization decisions...

TimWolla added 2 commits July 3, 2025 16:37
These functions do not modify the state of the program and depend on
thread-safe global variables only.
@TimWolla TimWolla force-pushed the zend-get-executed-scope branch from 2fb856e to edf10a4 Compare July 3, 2025 14:38
@iluuu1994
Copy link
Member

Given that this PR seems to lead to a unclear performance degradation, I'd prefer not to merge it. We can investigate why it happens, and potentially create a bug report if we can identify a compiler-related culprit. It may also be alignment-related, as Niels mentioned. I have thought about a way to align the function offsets of two binaries when I encountered this issue myself, given that -falign-functions and friends didn't help. One way might be to build the binaries, inspect the elf-file and calculate the necessary nops to insert, and then use llvm-bolt's --pad-funcs-before flag. Not sure whether that will work. Suggestions are welcome.

@iluuu1994
Copy link
Member

Also, for completion's sake: I tried to apply pure and const with an automated script last year:
master...iluuu1994:php-src:script-fix-const-pure-attributes

The entire PR showed a big degradation, although I don't remember by how much nor how I measured it. It would ofc be great if this unlocked some optimization opportunities. In that case, it might be better to go the automated route, rather than applying it bit-by-bit by hand.

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.

3 participants

TMZ Celebrity News – Breaking Stories, Videos & Gossip

Looking for the latest TMZ celebrity news? You've come to the right place. From shocking Hollywood scandals to exclusive videos, TMZ delivers it all in real time.

Whether it’s a red carpet slip-up, a viral paparazzi moment, or a legal drama involving your favorite stars, TMZ news is always first to break the story. Stay in the loop with daily updates, insider tips, and jaw-dropping photos.

🎥 Watch TMZ Live

TMZ Live brings you daily celebrity news and interviews straight from the TMZ newsroom. Don’t miss a beat—watch now and see what’s trending in Hollywood.