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

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

Closed
wants to merge 2 commits into from

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented May 20, 2023

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:

  • Made when the initial default value for reportWidth is being set in restoreDefaults().
  • Potentially made a second time if the users CodeSniffer.conf file contained a report_width => 'auto' entry.
  • Potentially made a third time if any of the rulesets used for the run contained a <config name="report-width" value="auto"/> entry.
  • Potentially made a fourth time if --report-width=auto would be passed from the command-line.

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.

  • phpunit --filter Core (= the tests which use this base class + a few extra tests):
    Before: 2 minutes.
    After: 8 seconds.
  • The same effect can be seen when running phpunit without a --filter:
    Before: 7 minutes.
    After: 5 minutes.
  • And when I apply a similar change to the one made here to the base test class in PHPCSUtils (4621 tests):
    Before: 2.5 minutes.
    After: 1 second.

Suggested changelog entry

  • Runtime improvement for PHPCS CLI users. The improvement should be most noticeable for users on Windows.

Related issues/external references

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

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

jrfnl added 2 commits July 21, 2023 00:34
Follow up on 3761 for which tests were added in 3820.

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:
* Made when the initial default value for `reportWidth` is being set in `restoreDefaults()`.
* Potentially made a second time if the users `CodeSniffer.conf` file contained a `report_width => 'auto'` entry.
* Potentially made a third time if any of the rulesets used for the run contained a `<config name="report-width" value="auto"/>` entry.
* Potentially made a fourth time if `--report-width=auto` would be passed from the command-line.

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.
…andling

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.

* `phpunit --filter Core` (= the tests which use this base class + a few extra tests):
    Before: **2 minutes**.
    After: **8 seconds**.
* The same effect can be seen when running `phpunit` without a `--filter`:
    Before: **7 minutes**.
    After: **5 minutes**.
* And when I apply a similar change to the one made here to the base test class in PHPCSUtils (4621 tests):
    Before: **2.5 minutes**.
    After: **1 second**.
@jrfnl jrfnl force-pushed the feature/config-improve-performance branch from 1f11631 to 020fbfa Compare July 20, 2023 22:34
@jrfnl
Copy link
Contributor Author

jrfnl commented Jul 20, 2023

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

@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 2, 2023

Closing as replaced by PHPCSStandards/PHP_CodeSniffer#61

@jrfnl jrfnl closed this Dec 2, 2023
@jrfnl jrfnl deleted the feature/config-improve-performance branch December 2, 2023 02:23
@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 8, 2023

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).

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

Successfully merging this pull request may close these issues.

1 participant