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

Introduce benchmark + performance regression checks #195

Closed
Ocramius opened this issue May 19, 2021 · 6 comments · Fixed by #223
Closed

Introduce benchmark + performance regression checks #195

Ocramius opened this issue May 19, 2021 · 6 comments · Fixed by #223
Labels
Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness Type: Enhancement Most issues will probably ask for additions or changes.
Milestone

Comments

@Ocramius
Copy link
Contributor

As Psl\ grows in size and adoption, its performance becomes one important evaluation point when considering it for downstream adoption.

This question came up in both #194 and Roave/BackwardCompatibilityCheck#306, and it would be a good idea to have reference/comparison.

The idea is to have following benchmarks:

  • comparison of Psl and internal functions or constructs it replaces (assuming 100% type coverage and no runtime errors occur)
  • comparison of Psl against itself at different releases

For that, it would be interesting to use https://github.com/phpbench/phpbench/tree/1.0.1

@dantleech do you know if there is a sensible way to run benchmarks and compare them in CI? That's something that is kinda missing within PHPBench' docs 🤔

@Ocramius Ocramius added the Type: Enhancement Most issues will probably ask for additions or changes. label May 19, 2021
@azjezz azjezz added this to the 1.8.0 milestone May 19, 2021
@azjezz
Copy link
Owner

azjezz commented May 19, 2021

some functions will always be slower than core ( e.g: Regex\matches will always perform worst than preg_match, due to additional error handling ), the question is how much difference is considered acceptable.

@Ocramius
Copy link
Contributor Author

Obviously, Psl will always be slower than core: it is required to keep the baseline under control though, as performance regressions often go unnoticed.

Also, knowing how much slower the library is allows for downstream consumers to take an informed decision as to when/where to use it.

@dantleech
Copy link

dantleech commented May 19, 2021

r.e. running in CI - I'm doing this with a bash script with assertions at a tolerance of 10% (but this is too optimistic, 15% might be better). It's still an experimental area (i also tried two separate actions to combine their results, but this doesn't work as the actions can run in completely different envs I guess).

@Ocramius
Copy link
Contributor Author

Sounds feasible - even with a very high tolerance, it's more about looking at it before releasing, I guess?

Intra-worker performance should be relatively stable

@dantleech
Copy link

dantleech commented May 19, 2021

it's more about looking at it before releasing, I guess?

with a 15% tolerance it should catch obvious regressions, I guess if you want to check more subtly then before release/at PR time.

Intra-worker performance should be relatively stable

as in 2 actions? at least for me (the one time i tried it) this resulted in less stable results

@Ocramius
Copy link
Contributor Author

as in 2 actions? at least for me (the one time i tried it) this resulted in less stable results

No no, same action and container: nothing guarantees that you will get two boxes running on a similar hardware otherwise.

@azjezz azjezz removed their assignment May 19, 2021
@azjezz azjezz added Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: Available No one has claimed responsibility for resolving this issue. labels May 19, 2021
@azjezz azjezz linked a pull request Sep 4, 2021 that will close this issue
1 task
@azjezz azjezz modified the milestones: 1.8.0, 1.9.0 Sep 8, 2021
@azjezz azjezz added Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness and removed Status: Available No one has claimed responsibility for resolving this issue. labels Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants