From 8a87ec277858c4e72e8f654627c5363ec979771b Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 28 Aug 2019 01:41:42 +0100 Subject: [PATCH] profiler: Centralise output responsibility from ProfilerOutputText to Profiler Make it Profiler.php's responsibility to enforce this, based on the existing signal from ProfilerOutput::logsToOutput(). The ProfilerOutputText class should not have to double-check this a second time. Long-term, I'd like even this check in Profiler::logDataPageOutputOnly to be removed, because really the external caller of that should know whether it is safe to output stuff or not rather than stashing its own state inside Profiler::$allowOutput and then implicitly reading it back out again later on. But, that's for another time. Also: * Remove use of deprecated Profiler::setTemplated while at it. * Make 'visible' parameter explicit, as for other parameters. Change-Id: Iaa3fc4ea25a059b90235d769db60c04b8f152f05 --- includes/profiler/Profiler.php | 8 ++- includes/profiler/output/ProfilerOutput.php | 7 ++- .../profiler/output/ProfilerOutputText.php | 59 ++++++++++--------- includes/skins/SkinTemplate.php | 2 +- load.php | 2 +- maintenance/Maintenance.php | 2 +- 6 files changed, 44 insertions(+), 36 deletions(-) diff --git a/includes/profiler/Profiler.php b/includes/profiler/Profiler.php index 0fcc97dbb2..2eb558603e 100644 --- a/includes/profiler/Profiler.php +++ b/includes/profiler/Profiler.php @@ -249,6 +249,10 @@ abstract class Profiler { * @since 1.26 */ public function logDataPageOutputOnly() { + if ( !$this->allowOutput ) { + return; + } + $outputs = []; foreach ( $this->getOutputs() as $output ) { if ( $output->logsToOutput() ) { @@ -291,7 +295,7 @@ abstract class Profiler { * @param bool $t */ public function setTemplated( $t ) { - // wfDeprecated( __METHOD__, '1.34' ); + wfDeprecated( __METHOD__, '1.34' ); $this->allowOutput = ( $t === true ); } @@ -302,7 +306,7 @@ abstract class Profiler { * @return bool */ public function getTemplated() { - // wfDeprecated( __METHOD__, '1.34' ); + wfDeprecated( __METHOD__, '1.34' ); return $this->getAllowOutput(); } diff --git a/includes/profiler/output/ProfilerOutput.php b/includes/profiler/output/ProfilerOutput.php index fe27c046e7..bc14f4bf68 100644 --- a/includes/profiler/output/ProfilerOutput.php +++ b/includes/profiler/output/ProfilerOutput.php @@ -48,7 +48,7 @@ abstract class ProfilerOutput { } /** - * Does log() just send the data to the request/script output? + * May the log() try to write to standard output? * @return bool * @since 1.33 */ @@ -57,7 +57,10 @@ abstract class ProfilerOutput { } /** - * Log MediaWiki-style profiling data + * Log MediaWiki-style profiling data. + * + * For classes that enable logsToOutput(), this must not + * be called unless Profiler::setAllowOutput is enabled. * * @param array $stats Result of Profiler::getFunctionStats() */ diff --git a/includes/profiler/output/ProfilerOutputText.php b/includes/profiler/output/ProfilerOutputText.php index 95b5ff95bc..ee06b59ace 100644 --- a/includes/profiler/output/ProfilerOutputText.php +++ b/includes/profiler/output/ProfilerOutputText.php @@ -29,11 +29,15 @@ */ class ProfilerOutputText extends ProfilerOutput { /** @var float Min real time display threshold */ - protected $thresholdMs; + private $thresholdMs; + + /** @var bool Whether to use visible text or a comment (for HTML responses) */ + private $visible; function __construct( Profiler $collector, array $params ) { parent::__construct( $collector, $params ); $this->thresholdMs = $params['thresholdMs'] ?? 1.0; + $this->visible = $params['visible'] ?? false; } public function logsToOutput() { @@ -41,39 +45,36 @@ class ProfilerOutputText extends ProfilerOutput { } public function log( array $stats ) { - if ( $this->collector->getTemplated() ) { - $out = ''; + $out = ''; - // Filter out really tiny entries - $min = $this->thresholdMs; - $stats = array_filter( $stats, function ( $a ) use ( $min ) { - return $a['real'] > $min; - } ); - // Sort descending by time elapsed - usort( $stats, function ( $a, $b ) { - return $b['real'] <=> $a['real']; - } ); + // Filter out really tiny entries + $min = $this->thresholdMs; + $stats = array_filter( $stats, function ( $a ) use ( $min ) { + return $a['real'] > $min; + } ); + // Sort descending by time elapsed + usort( $stats, function ( $a, $b ) { + return $b['real'] <=> $a['real']; + } ); - array_walk( $stats, - function ( $item ) use ( &$out ) { - $out .= sprintf( "%6.2f%% %3.3f %6d - %s\n", - $item['%real'], $item['real'], $item['calls'], $item['name'] ); - } - ); + array_walk( $stats, + function ( $item ) use ( &$out ) { + $out .= sprintf( "%6.2f%% %3.3f %6d - %s\n", + $item['%real'], $item['real'], $item['calls'], $item['name'] ); + } + ); - $contentType = $this->collector->getContentType(); - if ( wfIsCLI() ) { + $contentType = $this->collector->getContentType(); + if ( wfIsCLI() ) { + print "\n"; + } elseif ( $contentType === 'text/html' ) { + if ( $this->visible ) { + print "
{$out}
"; + } else { print "\n"; - } elseif ( $contentType === 'text/html' ) { - $visible = $this->params['visible'] ?? false; - if ( $visible ) { - print "
{$out}
"; - } else { - print "\n"; - } - } elseif ( $contentType === 'text/javascript' || $contentType === 'text/css' ) { - print "\n/*\n{$out}*/\n"; } + } elseif ( $contentType === 'text/javascript' || $contentType === 'text/css' ) { + print "\n/*\n{$out}*/\n"; } } } diff --git a/includes/skins/SkinTemplate.php b/includes/skins/SkinTemplate.php index f348135427..aeca0162c1 100644 --- a/includes/skins/SkinTemplate.php +++ b/includes/skins/SkinTemplate.php @@ -208,7 +208,7 @@ class SkinTemplate extends Skin { * Initialize various variables and generate the template */ function outputPage() { - Profiler::instance()->setTemplated( true ); + Profiler::instance()->setAllowOutput(); $out = $this->getOutput(); $this->initPage( $out ); diff --git a/load.php b/load.php index 4d34e5ddca..6edb7ec954 100644 --- a/load.php +++ b/load.php @@ -45,7 +45,7 @@ $context = new ResourceLoaderContext( $resourceLoader, $wgRequest ); // Respond to ResourceLoader request $resourceLoader->respond( $context ); -Profiler::instance()->setTemplated( true ); +Profiler::instance()->setAllowOutput(); $mediawiki = new MediaWiki(); $mediawiki->doPostOutputShutdown( 'fast' ); diff --git a/maintenance/Maintenance.php b/maintenance/Maintenance.php index c3644ee610..130d1fbf71 100644 --- a/maintenance/Maintenance.php +++ b/maintenance/Maintenance.php @@ -831,7 +831,7 @@ abstract class Maintenance { + $wgProfiler + [ 'threshold' => $wgProfileLimit ] ); - $profiler->setTemplated( true ); + $profiler->setAllowOutput(); Profiler::replaceStubInstance( $profiler ); } -- 2.20.1