Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WebServerBundle] Add support for Xdebug's Profiler#28298
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| return$this->hostname.':'.$this->port; | ||
| } | ||
| publicfunctionisXdebugUsed() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure adding this publilc method makes sense.
Let's useextension_loaded inline instead, don't you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we inline it, the developers could no longer disable this feature if wanted. Ok for you ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nobody is going to extend this class anyway.
fabpot commentedAug 30, 2018
I would add a flag instead. |
maidmaid commentedAug 30, 2018
Could you be more precise please ? |
fabpot commentedAug 30, 2018
Being able to use an option (sorry, doing too much of that other language that is not PHP) on commands: |
maidmaid commentedAug 30, 2018
IMO, this feature should be enabled by default (if xdebug extension is loaded of course), so if we really want make this feature controllable, |
e25fe30 to62852abComparenicolas-grekas commentedAug 31, 2018
I'm wondering: why would you need to do this? I mean: can't you change your php.ini instead and get the same result in a more flexible way (i.e. don't force the setting to everyone?) |
nicolas-grekas commentedAug 31, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Or can't we make |
maidmaid commentedAug 31, 2018
I thought that worked firstly, but it actually does not because we enable it only in the main process, not in the sub process running |
nicolas-grekas commentedAug 31, 2018
Yep I know, but wecan make it work. It would be better IMHO. We could even make it work for all xdebug ini settings if it makes sense, isn't it? |
maidmaid commentedAug 31, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
After checking again the settings described inhttps://xdebug.org/docs/all_settings, just enabling the trigger of the profiler seems relevant IMO. |
| } | ||
| $process =newProcess(array_merge(array($binary),$finder->findArguments(),array('-dvariables_order=EGPCS','-S',$config->getAddress(),$config->getRouter()))); | ||
| $xdebugArgs =\extension_loaded('xdebug') ?array('-dxdebug.profiler_enable_trigger=1') :array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should we check whether this setting is enabled in the current PHP instead of always adding them ? AFAIK, turning on this feature adds overhead to the execution of PHP, as Xdebug needs to instrument the code (even though it does not actually collect the profiling data).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's the reason why I turn on only the trigger of the profiler but not the profiler itself, whose the overhead is insignifiant.
nicolas-grekas commentedSep 1, 2018
I would really prefer this to be made to work with |
62852ab to8ef25ccComparemaidmaid commentedSep 1, 2018
@nicolas-grekas Could you review the last version I committed ? |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(with minor comment)
| $config =newWebServerConfig($documentRoot,$env,$input->getArgument('addressport'),$input->getOption('router')); | ||
| $io->success(sprintf('Server listening on http://%s',$config->getAddress())); | ||
| if (\extension_loaded('xdebug') &&'1' ===ini_get('xdebug.profiler_enable_trigger')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if (ini_get('xdebug.profiler_enable_trigger')) { would be enough to me (same below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
8ef25cc to34930ecCompare| $config =newWebServerConfig($documentRoot,$env,$input->getArgument('addressport'),$input->getOption('router')); | ||
| $io->success(sprintf('Server listening on http://%s',$config->getAddress())); | ||
| if ('1' ===ini_get('xdebug.profiler_enable_trigger')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The'1' === doesn't make really sense.
The really correct way to check a boolean ini setting is by using filter_var:filter_var(ini_get('xdebug.profiler_enable_trigger'), FILTER_VALIDATE_BOOLEAN)
Seehttp://php.net/manual/fr/function.filter-var.php#121263
I'd suggest to either make a loose comparison (thus my original proposal:if (ini_get('xdebug.profiler_enable_trigger')) {) or a strict one using filter_var, but not something in between.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ini_get return the normalized value instead of the one present in thephp.ini file, so the return value will be'1' ou'' here (orfalse if xdebug is not loaded).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh great! Let's useif (ini_get('xdebug.profiler_enable_trigger')) { then, to be consistent with the code base at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
34930ec to0f4c0a6Comparenicolas-grekas commentedSep 9, 2018
Thank you@maidmaid. |
…aidmaid)This PR was merged into the 4.2-dev branch.Discussion----------[WebServerBundle] Add support for Xdebug's Profiler| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | /| License | MIT| Doc PR | /> Xdebug's Profiler is a powerful tool that gives you the ability to analyze your PHP code and determine bottlenecks or generally see which parts of your code are slow and could use a speed boost.https://xdebug.org/docs/profilerWhen we run/start the web server, it would be useful to enable the trigger for the Xdebug's Profiler. That means we could easily trigger the creation of a Xdebug profile and analysing it [thanks to PhpStorm](https://www.jetbrains.com/help/phpstorm/analyzing-xdebug-profiling-data.html) which provides an **Execution Statistics** panel (examine the summary information about execution metrics of every called function) and a **Call Tree** panel (explore the execution paths of all called functions). You can see these two panels in action [here](https://youtu.be/_ua_O01IICg?t=1m22s) for a better understanding.Commits-------0f4c0a6 Add support for Xdebug Profiler
Uh oh!
There was an error while loading.Please reload this page.
https://xdebug.org/docs/profiler
When we run/start the web server, it would be useful to enable the trigger for the Xdebug's Profiler. That means we could easily trigger the creation of a Xdebug profile and analysing itthanks to PhpStorm which provides anExecution Statistics panel (examine the summary information about execution metrics of every called function) and aCall Tree panel (explore the execution paths of all called functions). You can see these two panels in actionhere for a better understanding.