Multiple JIT test improvements #12406
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). |
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 |
danog
force-pushed
the
tests_psalm_phpstan_phpseclib
branch
from
37f4aab
to
35c0183
Compare
|
@danog Btw I just merged the changes for |
|
Done @iluuu1994! 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 :) |
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:
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
| echo opcache.jit_blacklist_root_trace=255 >> /etc/php.d/opcache.ini | ||
| echo opcache.jit_blacklist_side_trace=255 >> /etc/php.d/opcache.ini |
There was a problem hiding this comment.
It's better to keep the default values.
There was a problem hiding this comment.
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.
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.
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
| unset($status); | ||
| gc_collect_cycles(); |
There was a problem hiding this comment.
Do we need these? The script ends here anyway.
There was a problem hiding this comment.
This should bypass an issue where the garbage collector is not invoked in forks for some reason (particularly in Psalm).
danog
force-pushed
the
tests_psalm_phpstan_phpseclib
branch
from
de059eb
to
90de8b4
Compare
Girgias
removed request for
adoy,
derickr,
arnaud-lb,
kocsismate,
nielsdos,
Girgias and
SakiTakamachi
danog
changed the title
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...).