squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

Config: only determine screen width if/when needed (performance improvement) #3831

Closed jrfnl closed 11 months ago

jrfnl commented 1 year ago

Description

Config: only determine screen width if/when needed

I'm not sure about *nix, but on Windows, the call to shell_exec('stty ...') is slow.

As things were, the call to shell_exec('stty ...') would be:

This is inefficient for the following reasons:

  1. The output from shell_exec('stty ...') does not change between calls (well, providing the user doesn't resize their terminal in the microseconds between calls)
  2. We don't actually need to know the value 'auto' translates to, until the reportWidth will be used.

Taking the above into account, making the same call up to four times is not desirable.

This commit moves the translation from 'auto' to an actual terminal width from the __set() method to the __get() method and overwrites the reportWidth value from 'auto' with the actual terminal width value, if available, and with the default value if the terminal width could not be determined. This means that subsequent calls to __get() for the reportWidth will automatically use the previously determined value instead of trying to determine value again.

This removes the inefficiency and should make PHPCS runs a little bit faster (at the very least on Windows).

The only time multiple calls to shell_exec('stty...') could now need to be made, would be if the reportWidth would be changed (back to 'auto') between the first retrieval and a subsequent retrieval of the reportWidth value. As things are, this will never happen in a normal PHPCS run, though could possibly happen in a test situation.

AbstractMethodUnitTest: take advantage of the change in reportWidth handling

For the tests using the AbstractMethodUnitTest class, the reportWidth and most other config settings are irrelevant.

This commit changes some of the set up/tear down for the test to make the use of the Config class more efficient.

This should make the tests using the AbstractMethodUnitTest class as their base significantly faster (at the very least on Windows).

While not benchmarked properly, I have done some comparisons with the test runs on my local machine on Windows.

Suggested changelog entry

Related issues/external references

Follow up on #3761 for which tests were added in #3820.

Types of changes

jrfnl commented 1 year ago

FYI: I've made a tiny tweak to the second commit to make the change compatible with PHP 8.3.

jrfnl commented 11 months ago

Closing as replaced by https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/61

jrfnl commented 10 months ago

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).