Page MenuHomePhabricator

Question: Is Skin::preloadExistence still relevant?
Closed, ResolvedPublicBUG REPORT

Description

Skins carry the method Skin::preloadExistence. The method is a little cryptic with very little documentation. It claims to preload the existence of 3 commonly requested pages in a single query.

To support it it uses the public Skin::footerLink and the private Skin::footerLinkTitle which are used inside Skin::getSiteFooterLinks

It also provides a hook which is seldom used:
https://codesearch.wmcloud.org/search/?q=SkinPreloadExistence&i=nope&files=&excludeFiles=&repos=

I think this code better belongs inside OutputPage skin preloadExistence has nothing to do with rendering or describing a skin.

Before I investigate refactoring this code, I wanted to check in with performance team if:

  1. the code is still relevant
  2. If the code should be expanded in any way e.g. to support other URLs.

Event Timeline

The method, and hook, exist for performance reasons.

So long as we have footer links in a skin, and that these links use the link renderer (with features such as red/blue link classes etc.) then they should be preloaded to avoid numerous databases queries that would otherwise go one-by-one for each link target to query page metadata during the critical path of rendering page views (even on mostly-cached pageviews).

When I last refactored this code, I considered proposing to remove this feature. Instead, I first optimised it so that it would no longer be a perf concern, and then forgot about it.

Removing it would mean the footer links behave (very slightly) differently. In particular, it means that they no longer get a red color if the target page doesn't exist for any reason. The downside of this would be that on a brand new wiki, it would be less obvious to the community that some of their default project pages have yet to be created. Another scenario is if a localisation update or interface admin changes the message that decides where the link goes to, and if the new target doesn't exist. It'd be nice to easily spot that through a red link. How often does someone with knowledge of how to fix it or report it, click on footer links to see if they still work?

On the other hand, we don't provide this affordance to sidebar links (although perhaps we should?) Either way, the sidebar has its own batch loader and cache, so it wouldn't be a performance issue there even if we did support red links there. Perhaps this would be a useful thing to surface through a special page for admins, about minor site issues and configuration recommendations. That way it wouldn't be computed on every page view, but still reach interested parties. This could even be configured to notify a certain user group when new issues are discovered. That's a general area, including ideas like post-upgrade installer-type warnings and extension updates, that we've talked about for a long time but as of yet undeveloped.

Krinkle edited projects, added Performance-Team (Radar); removed Performance-Team.
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.
Krinkle subscribed.

@Krinkle sounds good. Seems like a no brainer to retain the functionality.

The method is protected but never extended. Is the a skin internal or do we expect skins to extend this?

Do you think it would be okay to make this method private and internal to the Skin class?
https://codesearch.wmcloud.org/search/?q=preloadExistence&i=nope&files=&excludeFiles=&repos=

So long as skins have links in a footer, and that skins can control or change these links, and that we format these with LinkRenderer, then skins will also need to inform the preloader about those links to avoid additional db query overhead.

However, this informing happens through onSkinPreloadExistence. The preloadExistence() method isn't a getter to know what to preload, it's the instruction to kick off the preload. Only core knows when to call this. There should be no reason for other classes to call this indeed.