-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Multiple JIT test improvements #12406
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
Ping @iluuu1994 :)
@danog Can you apply this just to master? The changes haven't landed in older branches. I've only backported the minimum to fix nightly (because nightly uses the workflow.yml from master, but the action files from the branches).
I've only backported the minimum to fix nightly (because nightly uses the workflow.yml from master, but the action files from the branches).
Actually that's precisely why I can't target this on master, the patch.php file needs to be present on all branches for nightlies to work (plus, this improves JIT testing everywhere).
I missed how much this changes. Can you split the fixes for jit_max_... and the .github/patch.php file? I'm not sure if checking for opcache limits should be done this way, and I don't think it really matters for most things. Actually, setting jit_max_root_traces and shm being full is what exposed the bug here: #12366. Anyway, that part is more controversial so I'd like to hear what others think, but we can fix jit_max_... in the meantime.
37f4aab to
35c0183
Compare
@danog Btw I just merged the changes for jit_max_....
Done @iluuu1994!
I'm aware that #12366 was actually caused by the shm filling up, but at the same time I'm a bit confused as to how that could have happened, as when running the nightly CI tasks on my fork, the patch.php file actually warned that JIT was completely disabled due to excessive size of the JIT config (maybe it was just the normal opcache cache being filled up?).
Either way, opened #12425 with the improved jit_max settings and the patch.php JIT checker script (which I added just as a sanity check for the tests, to make sure none of them fill up the opcache memory, preventing some JIT traces from being compiled: there is value in testing the edge case of filling up the opcache memory, as proved by #12366, but my intent was more to improve JIT coverage :)
@danog Btw I just merged the changes for jit_max_....
Whoops missed that, thank you!
Ping @iluuu1994
Ping @iluuu1994 again? :)
@iluuu1994 @dstogov Should be good to merge!
I've also added a parallelization script for the old community tasks, which makes builds at least 20 minutes faster (when running on a 2-core GHA worker, sometimes GHA will provide a 4-core worker, which approximately halves overall execution times, but manually specifying a parallelism of 4 to the new nightly.php script might yield in further improvements even on 2-core GHA workers):
Example:
- Official 8.3 community build: 1h50m https://github.com/php/php-src/actions/runs/6844502224/job/18608466671
- Parallelized 8.3 community build (2-core GHA worker): 1h24m https://github.com/danog/php-src/actions/runs/6849232945/job/18621036979 (considering only tasks present in the old community build, Psalm+PHPStan+phpseclib add around 50 minutes to that, so not too bad IMO)
Another benefit of the nightly.php script is that nightly tests can now easily be run on other CIs (like for example the ARM one)
.github/workflows/nightly.yml
Outdated
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.
It's better to keep the default values.
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.
Personally, I would prefer to split this PR into two.
- Change ini setting
- Add new apps (this should be merged only after the first passes nightly tests)
I'm not sure if switching from yml tasks description to nightly.php is good or bad, but I can't follow the changes...
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.
Switching to the new nightly.php script allows to parallelize the phpunit tests of all the various libraries (this yields considerable speed improvements especially with CI workers with a lot of CPU cores); plus using a simple PHP script instead of a platform-dependent job declaration allows easily running the nightly tests locally and/or on different CIs (like for example, it might be worth running at least the nightly.php script on the ARM CI as well to test arm JIT)
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.
Honestly, I'm not a big fan of this for two reasons:
- It adds more complexity than we really need.
- More importantly, it merges the output of all tested frameworks into a single step in GA. Some steps like Symfony already emit a lot of output that is hard to scan. Merging all steps is going to make this much worse.
I appreciate your effort, but I wonder whether we actually need this build to be faster?
Your PRs also still do way too much. 🙂 Simultaneously moved and refactored code is very hard to review.
.github/jit_check.php
Outdated
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 these? The script ends here 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.
This should bypass an issue where the garbage collector is not invoked in forks for some reason (particularly in Psalm).
de059eb to
90de8b4
Compare
tests_psalm_phpstan_phpseclib
I think this is generally a good idea. Please rebase this on master so we can see the latest failures.
I agree with Ilija that all output smashes together is not very desirable.
I also wonder which of the workarounds of https://github.com/php/php-src/pull/12406/files#diff-b9bd430d413419fa77c4542b3359e8f5ad83d9128cf10bd784fb073ca088db68R127-R134 we still need, these may be fixed bugs (in Symfony for example and in php-src)
Also: what is the reason of adding a deepbind flag to configure? It seems unrelated.
Bumped and cleaned up a bit, will be opening issues for failures.
Regarding mixed output, while I could of course implement separation per library, that will only increase runtime (or at least needlessly delay it, if separating just printing the output without actually deparallelizing execution), so I'd like to leave it interleaved, the full reproducer command is available anyway.
Uh oh!
There was an error while loading. Please reload this page.
Had to reduce the trace count because allocations fails, it turns out JIT was actually disabled after all (??, maybe because my GHA workers have less RAM the the ones used by this org...).