The support requests for users with "PHP Fatal error: Cannot redeclare wfProfileIn()" continue in #mediawiki.
My idea on how to fix the issue previously was to include in 1.18.1 (whenever we release it) an includes/ProfilerStub.php with something like: <?php wfDeprecated( 'includes/ProfilerStub.php' );
So that it would override any dead includes/ProfilerStub.php from an old version of MediaWiki and the bad require_once people still have would just become a no-op and things would continue working.
I've been starting to think that in 1.19 we should drop StartProfiler.php altogether.
StartProfiler.php simply exists to initiate the profiler. It's basically a LocalSettings.php that runs before LocalSettings.php
Back in 1.17 we loaded things in the order: StartProfiler, Defines, AutoLoader, DefaultSettings, LocalSettings
However the order we load things now is: Init, AutoLoader, Profiler, Defines, StartProfiler, DefaultSettings, LocalSettings
So there is absolutely no extra value to StartProfiler now since it loads directly before our settings and can no longer profile things before the settings.
To top it off we have comments like these in our code: # Include site settings. $IP may be changed (hopefully before the AutoLoader is invoked)
If $IP is changed, it's naturally going to be changed in LocalSettings.php We're 'hoping' the AutoLoader won't be invoked until AFTER $IP is modified. But loading StartProfiler BEFORE it and recommending patterns that might invoke the AutoLoader.
Considering the support requests we get related to bad StartProfiler.php files, the bad documentation on how to setup StartProfiler.php, and the fact it doesn't add any advantage over LocalSettings.php anymore. I'm thinking we should stop including StartProfiler.php and just make anyone who actually wants to use the profiler move their profiler configuration into LocalSettings.php with the rest of their config. We could include a debug line if we want: if ( is_readable( "$IP/StartProfiler.php" ) ) { wfDebug( 'You have a outdated StartProfiler.php in your install which is no longer in use. We recommend you delete it and move any profiler config you wish to keep into your LocalSettings.php' ); }
On 26/12/11 09:20, Daniel Friesen wrote:
The support requests for users with "PHP Fatal error: Cannot redeclare wfProfileIn()" continue in #mediawiki.
My idea on how to fix the issue previously was to include in 1.18.1 (whenever we release it) an includes/ProfilerStub.php with something like: <?php wfDeprecated( 'includes/ProfilerStub.php' );
So that it would override any dead includes/ProfilerStub.php from an old version of MediaWiki and the bad require_once people still have would just become a no-op and things would continue working.
Sounds good.
I've been starting to think that in 1.19 we should drop StartProfiler.php altogether.
StartProfiler.php simply exists to initiate the profiler. It's basically a LocalSettings.php that runs before LocalSettings.php
Back in 1.17 we loaded things in the order: StartProfiler, Defines, AutoLoader, DefaultSettings, LocalSettings
However the order we load things now is: Init, AutoLoader, Profiler, Defines, StartProfiler, DefaultSettings, LocalSettings
So there is absolutely no extra value to StartProfiler now since it loads directly before our settings and can no longer profile things before the settings.
As long as it's treated like an extension, with a minimum of boilerplate PHP code in LocalSettings.php, I think that should be OK.
-- Tim Starling
On Tue, 27 Dec 2011 18:06:41 -0800, Tim Starling tstarling@wikimedia.org wrote:
On 26/12/11 09:20, Daniel Friesen wrote:
The support requests for users with "PHP Fatal error: Cannot redeclare wfProfileIn()" continue in #mediawiki.
My idea on how to fix the issue previously was to include in 1.18.1 (whenever we release it) an includes/ProfilerStub.php with something like: <?php wfDeprecated( 'includes/ProfilerStub.php' );
So that it would override any dead includes/ProfilerStub.php from an old version of MediaWiki and the bad require_once people still have would just become a no-op and things would continue working.
Sounds good.
I've been starting to think that in 1.19 we should drop StartProfiler.php altogether.
StartProfiler.php simply exists to initiate the profiler. It's basically a LocalSettings.php that runs before LocalSettings.php
Back in 1.17 we loaded things in the order: StartProfiler, Defines, AutoLoader, DefaultSettings, LocalSettings
However the order we load things now is: Init, AutoLoader, Profiler, Defines, StartProfiler, DefaultSettings, LocalSettings
So there is absolutely no extra value to StartProfiler now since it loads directly before our settings and can no longer profile things before the settings.
As long as it's treated like an extension, with a minimum of boilerplate PHP code in LocalSettings.php, I think that should be OK.
-- Tim Starling
The typical thing to go into StartProfiler.php for someone who wants to use it is: $wgProfiler['class'] = 'Profiler';
;) ie: It's pure config, it's not even php code.
Well, unless you're using StartProfiler.example's method of setting up a sampling profiler: if ( !mt_rand( 0, 100 ) ) { $wgProfiler['class'] = 'Profiler'; } else { $wgProfiler['class'] = 'ProfilerStub'; }
We could try to simplify those kind of common cases.
On 28/12/11 13:53, Daniel Friesen wrote:
The typical thing to go into StartProfiler.php for someone who wants to use it is: $wgProfiler['class'] = 'Profiler';
;) ie: It's pure config, it's not even php code.
That's because the actual starting of the profiler happens after StartProfiler.php finishes but before LocalSettings.php starts. When you said there was no extra value to StartProfiler.php, I thought you were imagining some system where the profiler would be started at the top of LocalSettings.php
Running LocalSettings.php itself is often slow, especially the extension setup files. So it's important that the profiler is running at this time.
-- Tim Starling
On Tue, 27 Dec 2011 19:02:29 -0800, Tim Starling tstarling@wikimedia.org wrote:
On 28/12/11 13:53, Daniel Friesen wrote:
The typical thing to go into StartProfiler.php for someone who wants to use it is: $wgProfiler['class'] = 'Profiler';
;) ie: It's pure config, it's not even php code.
That's because the actual starting of the profiler happens after StartProfiler.php finishes but before LocalSettings.php starts. When you said there was no extra value to StartProfiler.php, I thought you were imagining some system where the profiler would be started at the top of LocalSettings.php
Running LocalSettings.php itself is often slow, especially the extension setup files. So it's important that the profiler is running at this time.
-- Tim Starling
I'm imagining the very little that goes on in StartProfiler.php (which is basically config) will go into LocalSettings.php, probably early on.
All that goes into StartProfiler.php is configuration of the $wgProfiler variable.
We load $IP/includes/profiler/Profiler.php first, right after the AutoLoader. Following that we set `$wgProfiler = array();` and include StartProfiler.php if present. Following that we load DefaultSettings.php and then LocalSettings.php
DefaultSettings.php doesn't invoke the profiler. So Profiler::$__instance is never set until the first wfProfileIn call made AFTER LocalSettings.php loads. Thinking about it requiring extensions probably don't even typically invoke the profiler anywhere within LocalSettings since anything like that is probably deferred into a function.
So, we're not actually running the profiler within LocalSettings.php or anywhere before it.
((And if we really wanted to do that, I'd almost argue for a setup where wfProfilerIn will queue up a deferred list of profiling entries and then when LocalSettings.php finishes we make a call after the inclusion that commits them. So that we don't have to worry about the profiler even being created till after the profiler config is setup))
On 28/12/11 14:52, Daniel Friesen wrote:
So, we're not actually running the profiler within LocalSettings.php or anywhere before it.
((And if we really wanted to do that, I'd almost argue for a setup where wfProfilerIn will queue up a deferred list of profiling entries and then when LocalSettings.php finishes we make a call after the inclusion that commits them. So that we don't have to worry about the profiler even being created till after the profiler config is setup))
Yes we really want to do it. I already said it's important. So you should decide whether you're going to really argue for that or just almost argue.
-- Tim Starling
On Tue, 27 Dec 2011 23:36:53 -0800, Domas Mituzas midom.lists@gmail.com wrote:
We could try to simplify those kind of common cases.
Yet there are cases that are not so common - "start profiler for this page", "for this ip", "for this wiki", "for this query string", etc. Where would that belong?
Domas
Good point. Rather than simply simplifying that kind of case we could add to the $wgProfiler array a callback functionality that can return a boolean on whether to start the profiler.
wikitech-l@lists.wikimedia.org