New Performance report to measure sniff run time performance
#3810
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
1 task
|
Rebased to get past merge conflicts. |
jrfnl
force-pushed
the
feature/3784-add-new-sniff-performance-report
branch
from
314656b
to
172ac6a
Compare
... 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 order 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 test.
jrfnl
force-pushed
the
feature/3784-add-new-sniff-performance-report
branch
from
172ac6a
to
109cddc
Compare
|
As per my comment in PR #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 |
5 tasks
3 tasks
5 tasks
|
Closing as replaced by PHPCSStandards/PHP_CodeSniffer#60 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR depends on PR #3804 to allow for automated testing of the new report.Either PR #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 #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
trackTimetoggle for whether or not to track listener timesAs 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 > 2condition.However, adding the Performance report would add a second criteria.
This commit adds a new (internal) Config setting
trackTime, which will be set totruewhenPHP_CODESNIFFER_VERBOSITY > 2.This commit paves the way for adding the second criteria in the next commit.
New
Performancereport to measure sniff run time performanceThe 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 end-to-end test (depends on PR #3804).
Includes mention of the report in the CLI help text.
The report output is as shown in the screenshots in #3784 (comment)