The general idea with skins is that computed information is computed once, and stored as key-value pairs. Then re-used from there.

It starts at SkinTemplate::prepareQuickTemplate, which is called exactly once, and later methods re-use that injected/computed information.

I pointed out several times when SkinMustache and getTemplateData were getting introduced that it was not matching this logic structure, and favouring getting called lazily, using global state during the lazy init, and not caching data by default.

I had assumed this tech debt was resolved at some point, but looks like it's still basically the same. Although reducing it to "just two" is better than I feared, although having to carefully pass data many functions deep is not very ergonomic (or extendable) in the long run.

SkinMustache::getTemplateData should only be called once. I personally think it would make sense to formalize this by throwing a RuntimeException when it gets called more than once, as this bug keeps sneaking back into the code and we should likely guard against it.

@Krinkle my understanding aligns with yours about how this should work, so if it's not working that, I consider that a bug - please raise one. I think what might be confusing things here is @pmiazga is referring to SkinComponentMenu::getTemplateData() not SkinMustache::getTemplateData. Currently SkinMustache::getTemplateData calls out to various components and aggregates the data from each of them. The expectation is that SkinComponentMenu::getTemplateData() gets called for every menu once.

SkinTemplate::getPortletsTemplateData is called from two places:
SkinTemplate::getTemplateData()
Skin::getCategories()

Good catch. The issue described here is T331360 (which was also the thing I reached out to you about previously relating to the code mob - not sure if this would be a suitable venue to discuss the problem). On long term we should deprecate use of Skin::getCategories inside Vector22. We have been moving away from generating the same blobs of HTML for all skins towards a data based approach. The problem we have here is when building SkinVector22 we need to be careful not to make breaking changes to other skins and Skin::getCategories is used by other skins e.g Timeless.

In case you are interested, this touches on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/894732 which I mentioned to you previously would be a good task to go over during one of the Codemobs (allows us to make breaking changes to the data we ship to templates).

On short term it seems like we should cache this. If I submit a patch would you be able to review?

::getTemplateData was called twice and we already fixed that. That previous bug fixed - now this code is not as prominent (previously it was 4 times and bigger "All instances" time).

This bug was about calling SkinTemplate::getPortletsTemplateData which is called from SkinTemplate::getTemplateData and SkinTemplate:getCategoryLink - that both are called from SkinMustache::getTemplateData.
You can check it here - https://performance.wikimedia.org/excimer/profile/17c43b402fb432bb - but right now it's less than 1% whereas previously it was 2-3% if request time due to 4 calls.

SkinTemplate::getPortletsTemplateData calls three things:

• SkinTemplate::buildContentNavigationUrlsInternal
• Skin:buildSidebar
• Skin::getPortletData

Both SkinTemplate::buildContentNavigationUrlsInternal and Skin::buildSiderbar cache the method results in private properties, Skin::getPortletData creates the SkinComponentMenu every time is called.

@Jdlrobson - one more prominent issue might be that due to creating SkinComponentMenu twice with same parameters, we call Skin::getAfterPortlet() twice with the same parameters, which will trigger hooks for the same thing twice. Not a biggie, but probably not expected behaviour.

First call: https://github.com/wikimedia/mediawiki/blob/474925f009bce349941fc3e6c5e5baeca785df02/includes/skins/SkinTemplate.php#L181
Second call: https://github.com/wikimedia/mediawiki/blob/474925f009bce349941fc3e6c5e5baeca785df02/includes/skins/SkinTemplate.php#L592

While we work on this code, things get more complex and we start calling functions that build things multiple time, which probably wasn't the initial plan. I wonder if is it worth create a Container for the SkinTemplateData, as right now plenty of methods within Skin classes do things like

	private function someMethod() {
		if ( $this->cachedValue ) {
			return $this->cachedValue;
		}
		// process ....
                $this->cachedValue = $result;
                return $result;
        }

and this probably makes things confusing on what can/should be cached.

Change 979419 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Skin: Optimize code path for category generation

https://gerrit.wikimedia.org/r/979419

Change 979419 merged by jenkins-bot:

[mediawiki/core@master] Skin: Optimize code path for category generation

https://gerrit.wikimedia.org/r/979419