Page MenuHomePhabricator

Use DI for Profiler class instance in MediaWiki core code
Closed, ResolvedPublic

Description

Profiler::instance() contains some global state by using $wgProfiler

Also WikiMap::getCurrentWikiDbDomain() (maybe use LoadBalancer instead)
Also RequestContext and LoggerFactory

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Using DI for this makes sense to me. Thanks. I would recommend against making it a service, though, since this is not meant to be tied to a specific wiki or other other swappable state.

Rather, it should be global to the PHP process, independent of potentially multiple service containers (e.g. once we start to support multiple/foreign wiki contexts, which I see as one of the primary purposes for service wiring to exist). As such, I think Profiler::instance() remains the appropiate entry point to this singleton, but can indeed be injected into serviced classes (or used directly, in non-service classes and entry point code).

I think it's important that as we move toward DI in this class, that we make construction remains cheap. This is naturally the case in the legacy/global code as things are lazily discovered and initialised. I have seen this done incorrectly before in other classes, so I want to make this is taken into account by first fixing any expensive constructors of dependencies. Especially here since this may very well be the earliest and most constructed singleton we have that has dependencies on lots of other major singletons.

For wgProfiler and db name, it might make sense to take those directly as setting values without involving any indirection other needless complexity. They are not traditional configuration variables and would be well within the subset of config variables required for bootstrapping (like db host/pwd) and thus not loadable from an external source by extensions or otherwise benefit from Config. For testing the same constructor can be used directly as we do with service classes.

Krinkle renamed this task from Research to create service for Profiler::instance() to Use DI for Profiler class instance in MediaWiki core code .Jan 26 2021, 5:03 PM
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.
Krinkle claimed this task.
Krinkle removed a project: Performance-Team (Radar).
Krinkle added a project: Performance-Team.

Fixed in:

profiler: Inject $wgProfiler from Setup.php to Profiler
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/809043