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

New Performance report to measure sniff run time performance #60

Merged
merged 4 commits into from Dec 5, 2023

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 10, 2023

Description

Recreation of upstream PR squizlabs/PHP_CodeSniffer#3810:

This PR depends on PR squizlabs/PHP_CodeSniffer#3804 to allow for automated testing of the new report.
Either PR squizlabs/PHP_CodeSniffer#3804 should be merged first and this PR rebased to remove that commit after the merge or the EndToEnd test included in this PR should be removed from the last commit and (after this PR has been merged), the test can be added to PR squizlabs/PHP_CodeSniffer#3804.

☝🏻 The above is the only reason to open this PR in draft. The actual change in the PR is ready for review.


Util\Timing: split printRunTime() method

... into logical parts to allow the Performance report access to the total run time information (at the point of report creation) as well.

File: add getListenerTimes() method

... to allow for access to the recorded listener times from within a report class.

Config: add trackTime toggle for whether or not to track listener times

As recording the time taken by each sniff has a performance impact in and of itself on a CS run, only record the time taken by each sniff when needed. [*]

Originally, this was already done conditionally based on the PHP_CODESNIFFER_VERBOSITY > 2 condition.
However, adding the Performance report would add a second criteria.

This commit adds a new (internal) Config setting trackTime, which will be set to true when PHP_CODESNIFFER_VERBOSITY > 2.

This commit paves the way for adding the second criteria in the next commit.

[*] I've done some unscientific benchmarks for this by running PHPCS multiple times, with and without tracking the listener times, over a 300+ file codebase.
Without tracking listener times, the run time was always around 39 seconds with 56Mb memory use.
With tracking listener times, the run time was always around 54 seconds with 64Mb memory use.

This, to me, shows a significant enough difference and sufficient reason to put this toggle in place to only track time when needed.

New Performance report to measure sniff run time performance

The report will print the sniff name, cumulative listener run time and % of the total sniff run time for each sniff triggered during a run.

The report is ordered by cumulative listener run time in descending order.

Additionally, it will highlight sniffs which have a cumulative listener run time more than twice the average run time per sniff in orange and sniffs with a cumulative listener run time of more than three times the average run time per sniff in red.

At the bottom of the report it will also compare the total sniff relative run time with the total run time.

Fixes squizlabs/PHP_CodeSniffer#3784

Includes end-to-end test (depends on PR squizlabs/PHP_CodeSniffer#3804).
Includes mention of the report in the CLI help text.

The report output is as shown in the screenshots in squizlabs/PHP_CodeSniffer#3784 (comment)


More information from the original PR:

As per my comment in PR squizlabs/PHP_CodeSniffer#3804, I've removed the End2End test for the report from this PR to unblock it and am marking the PR as ready for review

Suggested changelog entry

  • New Performance report, mostly useful for sniff maintainers, to allow for finding "slow" sniffs.
    • To run this report, add --report=Performance for a normal PHPCS run on the command line.

Related issues/external references

Fixes #28

@jrfnl
Copy link
Member Author

jrfnl commented Nov 12, 2023

🎗️ Reminder: when this PR gets merged, the wiki will need to be updated, both the Usage page as well as the Reports page!

... into logical parts to allow the Performance report access to the total run time information (at the point of report creation) as well.
... to allow for access to the recorded listener times from within a report class.
…imes

As recording the time taken by each sniff has a performance impact in and of itself on a CS run, only record the time taken by each sniff when needed. [*]

Originally, this was already done conditionally based on the `PHP_CODESNIFFER_VERBOSITY > 2` condition.
However, adding the Performance report would add a second criteria.

This commit adds a new (internal) Config setting `trackTime`, which will be set to `true` when `PHP_CODESNIFFER_VERBOSITY > 2`.

This commit paves the way for adding the second criteria in the next commit.

---

[*] I've done some unscientific benchmarks for this by running PHPCS multiple times, with and without tracking the listener times, over a 300+ file codebase.
Without tracking listener times, the run time was always around 39 seconds with 56Mb memory use.
With tracking listener times, the run time was always around 54 seconds with 64Mb memory use.

This, to me, shows a significant enough difference and sufficient reason to put this toggle in place to only track time when needed.
The report will print the sniff name, cumulative listener run time and % of the total sniff run time for each sniff triggered during a run.

The report is ordered by cumulative listener run time in descending order.

Additionally, it will highlight sniffs which have a cumulative listener run time more than twice the average run time per sniff in orange and sniffs with a cumulative listener run time of more than three times the average run time per sniff in red.

At the bottom of the report it will also compare the total sniff relative run time with the total run time.

Fixes 3784

Includes mention of the report in the CLI help text.
@jrfnl jrfnl force-pushed the feature/3784-add-new-sniff-performance-report branch 2 times, most recently from 65916bd to 70a3d56 Compare December 5, 2023 22:51
@jrfnl jrfnl merged commit 38c2caf into master Dec 5, 2023
63 checks passed
@jrfnl jrfnl deleted the feature/3784-add-new-sniff-performance-report branch December 5, 2023 22:55
@jrfnl
Copy link
Member Author

jrfnl commented Dec 5, 2023

The wiki has been updated to include information about the new report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment