Improve file-diffing performance #3705

staabm · 2023-04-29T06:57:17Z

different approach on fixing rectorphp/rector#7899

after this PR we..

generate less diff strings -> which is faster
transport less data between processes in parallel-mode -> which is faster

packages/Parallel/Command/WorkerCommandLineFactory.php

staabm · 2023-04-29T10:22:00Z

I don't get why only e2e tests are failling

@TomasVotruba @samsonasik any idea?

samsonasik · 2023-04-29T10:28:04Z

it probably related with due to --dry-run and --no-ansi usage?

rector-src/e2e/e2eTestRunner.php

Line 21 in 0451834

    
           $e2eCommand = 'php '. $rectorBin .' process --dry-run --no-ansi -a '. $autoloadFile . ' --clear-cache';

samsonasik · 2023-04-29T10:51:17Z

@staabm you can try e2e test locally by run:

cd e2e/applied-rule-change-docblock 
composer install
php ../e2eTestRunner.php

packages/ChangesReporting/Output/ConsoleOutputFormatter.php

staabm · 2023-04-29T11:46:43Z

the only missing thing is the "first-line-number" in the file diffs, see e.g. https://github.com/rectorphp/rector-src/actions/runs/4838521883/jobs/8623019152?pr=3705

before this PR we were able to always tell about this line number, because we calculated each file-diff in both modes - and file numbers are only available in the "default-differ".

I guess if we want this perf improvement, we need to give up on this "first-line" number indication in the diff in parallel mode.

wdyt?

samsonasik · 2023-04-29T11:49:50Z

The line number in diff is a feature to ease jump to nearest changed code.

That's strange, I will look if I can do something

samsonasik · 2023-04-29T11:50:48Z

Oh, it seems you fix it?

staabm · 2023-04-29T11:50:53Z

That's strange, I will look if I can do something

I think I found the glitch. lets see

staabm · 2023-04-29T11:51:30Z

ohh lol. we now have the first line numbers, but also some more line numbers which we probably don't want? :)

staabm · 2023-04-29T15:23:50Z

packages/ChangesReporting/Output/WorkerOutputFormatter.php

+use Rector\Core\ValueObject\ProcessResult;
+use Rector\Parallel\ValueObject\Bridge;
+
+final class WorkerOutputFormatter implements OutputFormatterInterface


I have introduced this new formatter, so I can easily differentiate between a user requesting json format and a user requesting a different format, but rector is internally using json format while delegating to worker processes.

that was necessary for this PR to implement the pre-existing behaviour

staabm · 2023-04-29T16:48:01Z

@TomasVotruba do you think adding more detailed diff line numbers in diffs is a good thing and I should change test-expectations?

https://github.com/rectorphp/rector-src/actions/runs/4838587500/jobs/8623121975?pr=3705

samsonasik · 2023-04-29T16:53:07Z

More detailed diff is ok for me 👍

samsonasik · 2023-04-29T16:58:17Z

Hm..., not sure about DX perspective..., that probably cause ambiguous meaning when see the line change...

samsonasik · 2023-04-29T17:01:48Z

src/Console/Formatter/ConsoleDiffer.php

@@ -17,7 +17,7 @@ public function __construct(
    ) {
        // @see https://github.com/sebastianbergmann/diff#strictunifieddiffoutputbuilder
        // @see https://github.com/sebastianbergmann/diff/compare/4.0.4...5.0.0#diff-251edf56a6344c03fa264a4926b06c2cee43c25f66192d5f39ebee912b7442dc for upgrade
-        $unifiedDiffOutputBuilder = new UnifiedDiffOutputBuilder();
+        $unifiedDiffOutputBuilder = new UnifiedDiffOutputBuilder( "--- Original\n+++ New\n", true);


@staabm this may probably related of how original + new diff on Console formatter, see

rector-src/src/Console/Formatter/ColorConsoleDiffFormatter.php

Lines 66 to 71 in d7a738c

if ($escapedDiffLine === '--- Original') {

unset($escapedDiffLines[$key]);

}

if ($escapedDiffLine === '+++ New') {

unset($escapedDiffLines[$key]);

this value is the same as the one this class used by default before this PR

staabm · 2023-04-30T12:37:57Z

phpstan job requires #3716 to succeed

staabm · 2023-04-30T12:52:00Z

on a different workload the PR improves considerably (in comparison to e3c1d08)

even after all those PRs (this one included), diffing still dominates my profiles 🤷‍♂️.
from the remaining 4min 24secs diffing takes 1 minute.

samsonasik · 2023-04-30T16:25:01Z

Worker command line test seems error, probably related of changed format from json to worker https://github.com/rectorphp/rector-src/actions/runs/4844443238/jobs/8632724411#step:5:40

staabm · 2023-05-01T06:33:00Z

Worker command line test seems error

fixed in e933a6d

I have also adjusted the e2e test expectations in d9adf30

added more detailed line numbers

samsonasik · 2023-05-01T06:48:06Z

e2e/plain-views/expected-output.diff

@@ -4,7 +4,7 @@
 1) src/short_open_tag_with_import_name.php:0

    ---------- begin diff ----------
-@@ @@
+@@ -1,10 +1,12 @@
 <?php
 +


This somehow change the original expectation

rector-src/e2e/plain-views/expected-output.diff

Line 10 in 5c1be93

+use PhpParser\Node\Stmt\Expression;

First stmt detection maybe changed, you may need to debug on

rector-src/packages/PostRector/Rector/NameImportingPostRector.php

Lines 58 to 61 in 5c1be93

$currentStmt = current($file->getNewStmts());

if ($currentStmt instanceof FileWithoutNamespace && current($currentStmt->stmts) instanceof InlineHTML) {

return null;

}

staabm · 2023-05-01T07:18:31Z

I need to take a few more measure, but I think I was able to fix the underlying problem in sebastianbergmann/diff, which might make this PR here obsolete

samsonasik · 2023-05-01T07:58:51Z

@staabm thank you, I think patch to sebastianbergmann/diff is the right approach 👍 , we can just need to wait for next sebastianbergmann/diff release

staabm · 2023-05-01T10:27:38Z

This PR is no longer necessary.

I wrote a blog post about it https://staabm.github.io/2023/05/01/diff-speeding.html

staabm commented Apr 29, 2023

View reviewed changes

packages/Parallel/Command/WorkerCommandLineFactory.php Outdated Show resolved Hide resolved

staabm force-pushed the diff branch 2 times, most recently from ff0f5e2 to 3bd89ce Compare April 29, 2023 10:09

staabm commented Apr 29, 2023

View reviewed changes

packages/ChangesReporting/Output/ConsoleOutputFormatter.php Show resolved Hide resolved

staabm force-pushed the diff branch from 3680f42 to aa940ca Compare April 29, 2023 11:12

staabm marked this pull request as ready for review April 29, 2023 15:21

staabm requested a review from TomasVotruba as a code owner April 29, 2023 15:21

staabm commented Apr 29, 2023

View reviewed changes

samsonasik reviewed Apr 29, 2023

View reviewed changes

staabm force-pushed the diff branch 2 times, most recently from 7189e31 to 7f9b152 Compare April 30, 2023 12:26

staabm force-pushed the diff branch from 3c8c4eb to de9b1cb Compare April 30, 2023 12:54

staabm force-pushed the diff branch 2 times, most recently from b770e87 to d9adf30 Compare May 1, 2023 06:30

Improve file-diffing performance

3e5917c

staabm added 15 commits May 1, 2023 08:34

allow ansi

b201200

escape ansi chars

63014fc

support ansi

375762f

fix

6ae659e

introduce dedicated worker output format

01bb3a2

revert no-ansi.. no longer required

37f2a74

simplify

02774d0

fix missing line numbers in ConsoleDiffer

294cf41

Restore tests/Issues/PhpTagsAddedToBlade/PhpTagsAddedToBladeTest.php

973a991

ecs fix

60fafc2

fix

4b54a83

fix

8fb0513

fix

2f944ca

fix worker tests

4c03b88

adjust e2e expectations

c374bb1

added more detailed line numbers

staabm force-pushed the diff branch from d9adf30 to c374bb1 Compare May 1, 2023 06:34

samsonasik reviewed May 1, 2023

View reviewed changes

staabm closed this May 1, 2023

staabm deleted the diff branch May 1, 2023 10:28

staabm restored the diff branch May 1, 2023 18:51

staabm deleted the diff branch May 9, 2023 12:30

samsonasik mentioned this pull request Nov 23, 2024

[Differ] Remove DefaultDiffer, use ConsoleDiffer instead to avoid double diffing process #6482

Closed

	if ($escapedDiffLine === '--- Original') {
	unset($escapedDiffLines[$key]);
	}

	if ($escapedDiffLine === '+++ New') {
	unset($escapedDiffLines[$key]);

	$currentStmt = current($file->getNewStmts());
	if ($currentStmt instanceof FileWithoutNamespace && current($currentStmt->stmts) instanceof InlineHTML) {
	return null;
	}

Uh oh!

Improve file-diffing performance #3705

Improve file-diffing performance #3705

Uh oh!

Conversation

staabm commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

staabm commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samsonasik commented Apr 29, 2023

Uh oh!

samsonasik commented Apr 29, 2023

Uh oh!

Uh oh!

staabm commented Apr 29, 2023

Uh oh!

samsonasik commented Apr 29, 2023

Uh oh!

samsonasik commented Apr 29, 2023

Uh oh!

staabm commented Apr 29, 2023

Uh oh!

staabm commented Apr 29, 2023

Uh oh!

staabm Apr 29, 2023

Choose a reason for hiding this comment

Uh oh!

staabm commented Apr 29, 2023

Uh oh!

samsonasik commented Apr 29, 2023

Uh oh!

samsonasik commented Apr 29, 2023

Uh oh!

samsonasik Apr 29, 2023

Choose a reason for hiding this comment

Uh oh!

staabm Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm commented Apr 30, 2023

Uh oh!

staabm commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samsonasik commented Apr 30, 2023

Uh oh!

staabm commented May 1, 2023

Uh oh!

samsonasik May 1, 2023

Choose a reason for hiding this comment

Uh oh!

samsonasik May 1, 2023

Choose a reason for hiding this comment

Uh oh!

staabm commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samsonasik commented May 1, 2023

Uh oh!

staabm commented May 1, 2023

Uh oh!

Uh oh!

staabm commented Apr 29, 2023 •

edited

Loading

staabm commented Apr 29, 2023 •

edited

Loading

staabm Apr 29, 2023 •

edited

Loading

staabm commented Apr 30, 2023 •

edited

Loading

staabm commented May 1, 2023 •

edited

Loading