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

Needs performance guard CI process #343

Closed
sirbrillig opened this issue Dec 2, 2024 · 4 comments · Fixed by #345
Closed

Needs performance guard CI process #343

sirbrillig opened this issue Dec 2, 2024 · 4 comments · Fixed by #345

Comments

@sirbrillig
Copy link
Owner

We need to add a CI task that guards against major performance regressions like #340

@jrfnl
Copy link
Collaborator

jrfnl commented Dec 2, 2024

Not sure if it would help, but PHPCS itself offers a Performance report --report=Performance and you could possibly parse the outcome of that to check against a threshold value ?
The report is most interesting when combined with a range of sniffs to see which ones in a particular ruleset are performance hogs, but I suppose it could also be used for this.

As for improving performance, PHPCSUtils can help with that as any repeated calls to utility functions with the same arguments will get cached results within a single runtime (for most functions), which can often save a lot of processing.

@jrfnl
Copy link
Collaborator

jrfnl commented Dec 2, 2024

Note: the Performance report is only available in PHPCS 3.8.0+.

@sirbrillig
Copy link
Owner Author

sirbrillig commented Dec 2, 2024

Ah, that's cool!

Yeah, I'd like to just make a job that runs a timing check and makes sure that the total time is not greater than X. In this case, the performance before #334 was about 0.528548 seconds as measured on my machine by --report=Performance and after that PR it was 66.230763 seconds (using this file as a benchmark). I think we could safely say that if the total is greater than around 0.6 seconds, CI should fail. I'll see if I can build something using that.

@jrfnl
Copy link
Collaborator

jrfnl commented Dec 2, 2024

Maybe run grep over the output of the report ?

I'm thinking a variation of these steps (but then with phpcs & the Performance report, store the time output to $GITHUB_OUTPUT and compare that output then against a fixed setting in a next step):

- name: Grab PHPUnit version
id: phpunit_version
run: echo "VERSION=$(vendor/bin/phpunit --version | grep --only-matching --max-count=1 --extended-regexp '\b[0-9]+\.[0-9]+')" >> $GITHUB_OUTPUT
- name: "DEBUG: Show grabbed version"
run: echo ${{ steps.phpunit_version.outputs.VERSION }}
- name: Determine PHPUnit config file to use
id: phpunit_config
run: |
if [ "${{ startsWith( steps.phpunit_version.outputs.VERSION, '9.' ) && steps.phpunit_version.outputs.VERSION >= '9.3' }}" == "true" ]; then
echo 'FILE=phpunitlte9.xml.dist' >> $GITHUB_OUTPUT
echo 'EXTRA_ARGS=--coverage-cache ./build/phpunit-cache' >> $GITHUB_OUTPUT
elif [ "${{ startsWith( steps.phpunit_version.outputs.VERSION, '10.' ) || startsWith( steps.phpunit_version.outputs.VERSION, '11.' ) }}" == "true" ]; then
echo 'FILE=phpunit.xml.dist' >> $GITHUB_OUTPUT
echo 'EXTRA_ARGS=' >> $GITHUB_OUTPUT
else
echo 'FILE=phpunitlte9.xml.dist' >> $GITHUB_OUTPUT
echo 'EXTRA_ARGS=' >> $GITHUB_OUTPUT
fi

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

Successfully merging a pull request may close this issue.

2 participants