Fixer: improve performance of generateDiff() #355
Conversation
jrfnl
changed the title
jrfnl
force-pushed
the
feature/fixer-generate-diff-performance-improvement
branch
from
25d3c34 to
de1c3f2
Compare
|
I've updated the first commit with four extra tests (diff of blank lines/whitespace at start/end of file) |
jrfnl
force-pushed
the
feature/fixer-generate-diff-performance-improvement
branch
from
de1c3f2 to
905e157
Compare
I tested this on my Ubuntu machine using the command On a quick check, I believe the same happened in the CI runs. On the last few runs that I checked on the main branch, it took around 13 seconds to run the tests (https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/7977678082/job/21781108952#step:9:56 and https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/7975902826/job/21775216017#step:9:56). On this branch, it took around 15 seconds (https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/7986938143/job/21808386831?pr=355#step:9:56). Since the sample size is very small, it is very possible that this variation in the execution time is not directly related to the changes in this branch. |
|
Thanks for testing @rodrigoprimo ! Could it be the new tests I added (which specifically trigger this "slow" code) which cause the extra run time ? Could be checked by checking out the branch and (temporarily) reverting the changes from the first commit. |
|
@jrfnl, now that I ran the tests a few more times, I'm starting to think that the first data that I got is not very reliable. I'm seeing a significant variation in execution times between runs. A few times, the execution time using this branch is faster than using the main branch and sometimes it is not. I tried running all the tests without the first commit and didn't notice much of a difference (but again, the execution time varies a lot anyway). Then I used For comparison, here are the slowest tests for this run: |
|
@rodrigoprimo Thanks for doing that. The main thing I wanted to verify is that this doesn't cause a performance regression on non-Windows systems and if the timings are around the same time each time, with small differences, I don't think it does, as that's the normal behaviour. |
|
Just to give you some idea of the difference this makes on Windows (on a monster of a desktop) - when running just the GenerateDiff tests, I consistently see a difference of 2 seconds in the test run (~3 seconds without the patch), ~1 second with the patch): Timing without the patch (first commit only): Timing with the patch: |
|
I agree that there don't seem to be any performance regressions introduced by this PR. Thanks for the additional details regarding the difference this PR makes in the execution time when running on Windows. I got curious and decided to check the execution times of the new tests without the patch on Linux, and there is an improvement as well. Without the patch (just the first commit): With the patch: |
jrfnl
force-pushed
the
feature/fixer-generate-diff-performance-improvement
branch
from
905e157 to
2fdbcc4
Compare
|
Made one very tiny tweak still to the code to account better for the different return types of - if ($diff === null) {
+ if ($diff === false || $diff === '') { |
These tests test the functionality and document the behaviour of the `Fixer::generateDiff()` method, with a particular focus on (trailing) whitespace handling in the diff as this was noted in the method itself as a potential point of failure. Related to 146
Similar to 351 and 354, this is a change intended to make the test suite faster, in particular when running on Windows, as a quality of life improvement for contributing developers. However, this change has the added benefit that the performance improvement will also be noticeable for users running PHPCS with the `diff` report, which also uses the `Fixer::generateDiff()` method. On its own, this change makes running the test suite ~50% faster on Windows. In combination with the other two changes, the difference is smaller, but still ~10%.
jrfnl
force-pushed
the
feature/fixer-generate-diff-performance-improvement
branch
from
2fdbcc4 to
851bda2
Compare
|
Rebased without further changes. Merging once the build passes. |
jrfnl
deleted the
feature/fixer-generate-diff-performance-improvement
branch
Description
Fixer::generateDiff(): add tests
These tests test the functionality and document the behaviour of the
Fixer::generateDiff()method, with a particular focus on (trailing) whitespace handling in the diff as this was noted in the method itself as a potential point of failure.Related to #146
Fixer: improve performance of generateDiff()
Similar to #351 and #354, this is a change intended to make the test suite faster, in particular when running on Windows, as a quality of life improvement for contributing developers.
However, this change has the added benefit that the performance improvement will also be noticeable for users running PHPCS with the
diffreport, which also uses theFixer::generateDiff()method.On its own, this change makes running the test suite ~50% faster on Windows.
In combination with the other two changes, the difference is smaller, but still ~10%.
👉🏻 Request for testing:
Based on the test runs in CI, this change should make no significant different on *nix systems, but I'd love to have some people test it and post their with and without timing results.
Suggested changelog entry
Performance improvement for the "Diff" report. Should be most notable for Windows users.