Skip to content

Conversation

@AJenbo
Copy link
Contributor

@AJenbo AJenbo commented Mar 21, 2023

I have seen several developers getting confused by this message thinking that --xdebug will result in better performance.

Since it can be read as:

"--xdebug" is not used. This may slow down performance

(missing the meaning of the period and surrounding text)

Having xdebug and phpstan installed is common for developers :)

$errorOutput->getStyle()->note('You are running with "--xdebug" enabled, but the Xdebug PHP extension is not active. The process will not halt at breakpoints.');
} elseif (!$allowXdebug && XdebugHandler::isXdebugActive()) {
$errorOutput->getStyle()->note('The Xdebug PHP extension is active, but "--xdebug" is not used. This may slow down performance and the process will not halt at breakpoints.');
$errorOutput->getStyle()->note('The Xdebug PHP extension is active, but "--xdebug" is not used. Using "--xdebug" may slow down performance, but will allow you to debug PHPStan using breakpoints.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the point of this message is, that php processes may slow down because xdebug beeing loaded.

phpstan doesn't get slowser because of --xdebug

Copy link
Contributor Author

@AJenbo AJenbo Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't xdebug handler prevent that by disabling the extension?

My understanding of --xdebug is that it prevents xdebug handler from doing so.

The current phrasing makes it sound like --xdebug will improve performance which I don't think is the case either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When xdebug is loaded things get a lot slower - e.g. when running make tests.

If you don't run with --xdebug you get a slow process without any benefits - hence the warning.

Copy link
Contributor Author

@AJenbo AJenbo Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of XdebugHandler is to restart the process without Xdebug exactly to avoid this slow down, even if it is installed. By testing for XdebugHandler::isXdebugActive() before $xdebug->check(); this message is being shown when it's not relevant.

Here are some numbers from a fairly large project.
php phpstan: 117 sec
php+xdebug phpstan: 110 sec
php+xdebug phpstan --xdebug: 264 sec

Maybe you had PHPSTAN_ALLOW_XDEBUG=1 set in your enviroment when testing this so that Xdebug was left active? This would cause Xdebug to still be loaded and result in the behavior you describe (https://github.com/composer/xdebug-handler#basic-usage).

Edit: see #2299 for a fix for that issue.

Copy link
Contributor Author

@AJenbo AJenbo Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the confusing nature of the message has been pointed out by others as well: #1878 (comment), issue/WI-69996

How about this phrasing instead?

The Xdebug PHP extension is active, but "--xdebug" is not used. Consider disabling Xdebug when not debugging PHPStan, to avoid performance issues.

I took out and the process will not halt at breakpoints to keep the message relatively short as users are unlikely to have set any breakpoints if they where unaware that Xdebug is loaded.

If neither of these versions seem suitable, could you please help me rephrase it? It sounds like the message might be related to how make tests works internally, something I'm not familiar with.
The warning is causing confusion for end-users in its current form. I have noticed that some people are setting up their CI to run slower due to misunderstanding the message and thinking that using --xdebug will actually improve performance when Xdebug is active. Your help with resolving this issue would be greatly appreciated. Thank you

@ondrejmirtes
Copy link
Member

Please rebase since the code moved.

@ondrejmirtes
Copy link
Member

I did one more thing regarding this: 1d4ede1

@ondrejmirtes
Copy link
Member

I closed this PR because I don't think we need to improve this anymore.

@AJenbo AJenbo deleted the patch-1 branch April 1, 2023 13:41
@AJenbo
Copy link
Contributor Author

AJenbo commented Apr 1, 2023

Agreed, the new message shown since 1d4ede1 makes it much more clear which options is the right one for ones use case.

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

Labels

None yet

3 participants