Page MenuHomePhabricator

Preference retrieval should not require so much parsing
Closed, ResolvedPublic

Description

Currently just fetching user options on Commons is slow due to a slow template in the gadget descriptions section. This actually causes deadlocks in ApiOptions when the watchlist token has to be created, the slow parsing happens, and then the actual preferences update happens. This is all pointless, since the API doesn't need those checkbox labels anyway...

One can profile this using eval.php on commons using:

Profiler::setInstance( new ProfilerSimpleText( array() ) ); Profiler::instance()->setTemplated( true );
$preferences = array();
var_dump( GadgetHooks::getPreferences( $wgUser, $preferences ) );
wfLogProfilingData();


Version: 1.23.0
Severity: normal

Details

Reference
bz56633
ProjectBranchLines +/-Subject
mediawiki/coremaster+45 -3
mediawiki/extensions/BetaFeaturesmaster+6 -2
mediawiki/coremaster+42 -2
mediawiki/coremaster+65 -9
mediawiki/extensions/Echomaster+11 -7
mediawiki/extensions/Gadgetsmaster+2 -3
mediawiki/extensions/BetaFeaturesmaster+3 -4
mediawiki/coremaster+30 -23
mediawiki/coremaster+6 -6
mediawiki/coremaster+1 -1
mediawiki/corewmf/1.37.0-wmf.7+1 -1
mediawiki/extensions/Gadgetswmf/1.37.0-wmf.7+22 -16
mediawiki/corewmf/1.37.0-wmf.7+12 -4
mediawiki/coremaster+12 -4
mediawiki/extensions/Gadgetsmaster+22 -16
mediawiki/coremaster+39 -43
mediawiki/extensions/Gadgetsmaster+20 -15
mediawiki/coremaster+1 -1
Show related patches Customize query in gerrit

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:41 AM
bzimport set Reference to bz56633.
bzimport added a subscriber: Unknown Object (MLST).

I haven't verified with stack traces, but looking at the code this is still a problem.

The main cause is that the registration for preferences is coupled with the provision of interface rendering information (labels etc.).

This should be fixable by at least allowing the hook to provide a message key or message object, instead of the fully parsed value. This would delay rendering of the messages, or, in case of API responses that just get or set preferences, not perform parsing at all.

From what I can see, Preferences in general already supports the use of a label-message key in favour of a literal label key. However, there are two problems:

  1. The Gadgets extension wrap the message between a <span dir=auto>. As such, it is forced to parse the message first.
  2. The Gadgets preferences use checkboxes for their preferences, which the MediaWiki Preferences system only supports through the use of an associative array, where the key is the label. As such, there is no place here to express a message key or message object.

This is the last remaining item in our Radar that doesn't have a team associated with it. Tagging Contributors-Team, given User preferences (previously unowned) has now been claimed by that team on mw.org.

Change 681204 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] Add default-message option key to HTMLInfoField

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

Change 681213 had a related patch set uploaded (by Krinkle; author: Umherirrender):

[mediawiki/extensions/BetaFeatures@master] Use new default-message option key of HTMLInfoField

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

Change 681217 had a related patch set uploaded (by Krinkle; author: Umherirrender):

[mediawiki/core@master] Handle multiselect param 'options-messages' in DefaultPreferencesFactory

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

Change 681214 had a related patch set uploaded (by Krinkle; author: Umherirrender):

[mediawiki/extensions/Gadgets@master] Use new default-message option key of HTMLInfoField

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

Change 681218 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/Gadgets@master] Reduce message parse in GadgetHooks::getPreferences

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

Change 681217 merged by jenkins-bot:

[mediawiki/core@master] Handle multiselect param 'options-messages' in DefaultPreferencesFactory

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

Change 681218 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Reduce message parse in GadgetHooks::getPreferences

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

Change 681218 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Reduce message parse in GadgetHooks::getPreferences

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

Reverted in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Gadgets/+/681733.

Change 686780 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] preferences: Swap from 'help' key to 'help-message' key on htmlform

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

Change 686993 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] Add messages options to HTMLCheckMatrix form field

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

Change 686994 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/Echo@master] Use new messages options on HTMLCheckMatrix form field

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

Change 687011 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] preferences: Move complex creation of infos for usergroups into closure

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

Change 687011 merged by jenkins-bot:

[mediawiki/core@master] preferences: Move complex creation of infos for usergroups into closure

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

Change 697075 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] Allow html form field option 'options-messages' to get parsed

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

Change 697076 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/Gadgets@master] Reduce message parse in GadgetHooks::getPreferences (second time)

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

Change 697075 merged by jenkins-bot:

[mediawiki/core@master] Allow html form field option 'options-messages' to get parsed

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

Change 697076 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Reduce message parse in GadgetHooks::getPreferences (second time)

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

Change 697816 had a related patch set uploaded (by Ladsgroup; author: Umherirrender):

[mediawiki/extensions/Gadgets@wmf/1.37.0-wmf.7] Reduce message parse in GadgetHooks::getPreferences (second time)

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

Change 697817 had a related patch set uploaded (by Ladsgroup; author: Umherirrender):

[mediawiki/core@wmf/1.37.0-wmf.7] Allow html form field option 'options-messages' to get parsed

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

Change 697817 merged by jenkins-bot:

[mediawiki/core@wmf/1.37.0-wmf.7] Allow html form field option 'options-messages' to get parsed

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

Mentioned in SAL (#wikimedia-operations) [2021-06-02T23:18:46Z] <ladsgroup@deploy1002> Synchronized php-1.37.0-wmf.7/includes: Backport: [[gerrit:697817|Allow html form field option 'options-messages' to get parsed (T58633)]] (duration: 01m 01s)

Change 697816 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@wmf/1.37.0-wmf.7] Reduce message parse in GadgetHooks::getPreferences (second time)

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

Change 697887 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/core@master] user: Accept options-messages for multiselect user options

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

Change 697818 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/core@wmf/1.37.0-wmf.7] user: Accept options-messages for multiselect user options

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

Change 697818 merged by jenkins-bot:

[mediawiki/core@wmf/1.37.0-wmf.7] user: Accept options-messages for multiselect user options

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

Mentioned in SAL (#wikimedia-operations) [2021-06-03T00:36:35Z] <ladsgroup@deploy1002> Synchronized php-1.37.0-wmf.7/includes/user/UserOptionsManager.php: Backport: [[gerrit:697818|user: Accept options-messages for multiselect user options (T58633 T278650)]] (duration: 00m 57s)

Mentioned in SAL (#wikimedia-operations) [2021-06-03T00:40:00Z] <ladsgroup@deploy1002> Synchronized php-1.37.0-wmf.7/extensions/Gadgets: Backport: [[gerrit:697816|Reduce message parse in GadgetHooks::getPreferences (second time) (T58633 T278650)]], Try II (duration: 00m 57s)

Change 697887 merged by jenkins-bot:

[mediawiki/core@master] user: Accept options-messages for multiselect user options

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

Change 686780 merged by jenkins-bot:

[mediawiki/core@master] preferences: Swap from 'help' key to 'help-message' key on htmlform

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

Change 681204 abandoned by Umherirrender:

[mediawiki/core@master] Add default-message option key to HTMLInfoField

Reason:

Not working for all existing usecases

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

Change 681213 abandoned by Umherirrender:

[mediawiki/extensions/BetaFeatures@master] Use new default-message option key of HTMLInfoField

Reason:

depending patch set is gone

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

Change 681214 abandoned by Umherirrender:

[mediawiki/extensions/Gadgets@master] Use new default-message option key of HTMLInfoField

Reason:

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

Change 686994 abandoned by Umherirrender:

[mediawiki/extensions/Echo@master] Use new messages options on HTMLCheckMatrix form field

Reason:

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

Change 686993 abandoned by Umherirrender:

[mediawiki/core@master] Add messages options to HTMLCheckMatrix form field

Reason:

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

Change 758502 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] Create HTMLFormValidator and use it for DefaultPreferencesFactory and ApiOptions

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

Change 758920 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] Get simplified form descriptor for validations of user preferences

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

Update on some perf stats.

Flame graphs that measure API requests in production:
https://performance.wikimedia.org/arclamp/svgs/daily/2022-01-30.excimer.api.svgz?s=ApiOptions&x=497.5&y=965

From 30,000 samples from ApiOptions requests over 24h, about half were spent in ApiOptions::getPreferences, and about half in UserOptionsManager::getOptionKinds. Both of which spend virtually all their time in DefaultPreferencesFactory::getFormDescriptor.

Change 758920 merged by jenkins-bot:

[mediawiki/core@master] preferences: Use a faster and simpler form descriptor when validating

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

Change 766206 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] Re-apply \"preferences: Use a faster and simpler form descriptor when validating\"

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

Change 769459 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/BetaFeatures@master] Hooks: Avoid message parsing during ApiOptions from getPreferences()

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

Change 769459 merged by jenkins-bot:

[mediawiki/extensions/BetaFeatures@master] Hooks: Avoid message parsing during ApiOptions from getPreferences()

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

Change 766206 merged by jenkins-bot:

[mediawiki/core@master] Re-apply "preferences: Use a faster and simpler form descriptor when validating"

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

All patches are merged or abandoned. Is this perhaps fixed?

I'll confirm in prod next Thursday based on flame graphs to see if anything else still shows up in ApiOptions for example.