Page MenuHomePhabricator

Performance analysis of ULS "Compact Interlanguage links" feature
Closed, ResolvedPublic

Description

I noticed this feature taking a fair amount of time to initialize when enabling the Beta feature.

Here's a summary of a quick performance analysis I did using Chrome Dev Tools' Timeline on a fast desktop machine.

In total CompactInterlanguageList.init() takes 40-50ms (on a fast desktop machine).

  • hideOriginal() / showOriginal():
    • Forced and blocking style recalculation that causes a needless flash between hiding original and displaying the compact list.
      • Move call to hideOriginal() to before render() to combine paints.
    • hide(): Lots of needless overhead inside jQuery and Sizzle. See also https://github.com/jquery/jquery.com/issues/88 and http://www.learningjquery.com/2010/05/now-you-see-me-showhide-performance. VisualEditor also learned a lesson from this and improved performance a lot. jQuery hide() is a problematic kitchen sink due to an endless stream of use cases it needs to account for. If you know your code, always use an explicit inline style instead.
      • Use plain css() instead.
    • show(): Creates inline 'display: list-item;' which is the default.
      • Use plain css() instead.
  • getInterlanguageList():
    • Lots of overhead from Sizzle.attr() and jQuery.access().
      • Use plain getAttribute() instead.
  • compact()->filterByPreviousLanguages():
    • mw.uls.getPreviousLanguages() is deoptimized due to try/catch.
    • [replaced with localstorage] mw.uls.getPreviousLanguages() is slow due to $.cookie() access.
  • compact()->filterByCommonLanguages() [upstream lib: uls]
    • mw.uls.getFrequentLanguageList() is slow due to use of getPreviousLanguages().
    • [replaced with localstorage] mw.uls.getPreviousLanguages() calls $.cookie() – again!
    • [replaced with localstorage] mw.uls.getPreviousLanguages() also calls $.removeCookie()
      • ⇒ Add caching so this second call to $.cookie() isn't done.
      • ⇒ Defer the removeCookie() to some future time, doesn't seem important.
  • compact()
    • This function seems to be broken. It calls various filters that take a list as argument, but it passes no argument so it always goes through a costly fetch only to end up returning an empty list.
  • addTrigger() [upstream lib: jquery.i18n]
    • Uses browser's HTML parser. Should probably be plain text nstead. It appears $.i18n, contrary to mw.msg, doesn't have a text() mode where only transformation is done but no html creation/escaping. It'd be a lot better to expose plain text directly so there is no need to escape it and then natively parse as HTML. You can inject text into the DOM directly as a text node.
  • listen()
    • Takes over 20ms synchronously.
    • [offset now called only after click] Forced style calculation, layout, and paint because of call to $trigger.offset().
      • ⇒ Maybe figure a way to display this more naturally with the stylesheet relatively instead of requiring inline styles and absolute offsets.
  • listen()->filterByCommonLanguages()
    • Called again, when it was already called by compact()
    • Re-triggers mw.uls.getFrequentLanguageList(), getPreviousLanguages() and $.cookie() read.
  • listen()->uls() [upstream lib: uls]
    • Takes over 20ms synchronously (on a high-end desktop machine!).
    • Does a whole bunch of things, relating to:
      • ULS.listen()
      • $.fn.lcd() / LanguageCategoryDisplay
      • RegionSelector.init()
        • LanguageCategoryDisplay.append() which does many repeated calls to clearTimeout/setTimeout.
      • ULS.success()
        • Includes an avoidable 'Recalculate Style' from show(), css(), detach(), remove(), append(), and more.

Event Timeline

Krinkle created this task.Dec 23 2015, 8:45 PM
Krinkle updated the task description. (Show Details)
Krinkle raised the priority of this task from to Needs Triage.
Krinkle added a subscriber: Krinkle.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptDec 23 2015, 8:45 PM

Change 260795 had a related patch set uploaded (by Krinkle):
Various performance fixes for CompactInterlanguageList

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

Krinkle updated the task description. (Show Details)Dec 23 2015, 10:17 PM
Krinkle set Security to None.

Thanks a lot for this analysis. Do you think fixing all of these would make the performance acceptable, or does that need more detailed analysis/fixes?

Krinkle added a comment.EditedDec 28 2015, 11:20 PM

Thanks a lot for this analysis. Do you think fixing all of these would make the performance acceptable, or does that need more detailed analysis/fixes?

It's hard to tell for sure what the runtime footprint will look like, but I think fixing the issues on this list would make it acceptable - yes.

Note that my patch only covers a few from the list. More from the list needs to be done for it to be acceptable. Especially the inefficient use of cookies and forced style calculations in ULS.

Change 260795 merged by jenkins-bot:
Various performance fixes for CompactInterlanguageList

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

Nemo_bis triaged this task as Normal priority.Jan 5 2016, 11:48 AM
Nikerabbit updated the task description. (Show Details)Apr 20 2016, 2:08 PM

Updated the description to what was already done by @Krinkle. We might have touched a few more on recent refactorings, but all of these should be fixed and verified.

Krinkle updated the task description. (Show Details)Apr 21 2016, 11:20 PM
Nikerabbit updated the task description. (Show Details)Apr 22 2016, 6:51 AM
Nikerabbit updated the task description. (Show Details)Apr 22 2016, 6:54 AM
Nikerabbit updated the task description. (Show Details)Apr 22 2016, 6:56 AM

For some reason both https://gerrit.wikimedia.org/r/#/c/284869/ and https://gerrit.wikimedia.org/r/#/c/284868/ were not linked here!

I also remember doing some testing what where the main causes left, but don't remember them now.

So after latest fixes, on regular page view (i.e. not clicking the trigger), it now takes around 17 ms. This splits to 6ms mw.loader.using for ext.uls.mediawiki and 8ms to CompactInterlanguageList.render, where 6ms is taken by mw.msg.

mw.msg is unavoidable and optimizing it is imho out of scope for this task. We could make it faster by not showing the exact number to avoid plural parsing, but that seems counter-productive to me. Lazy-loading ext.uls.mediawiki is debatable.

More optimizations can be done for the code that gets executed when the trigger is clicked and menu is constructed (a chunk of code was executed on page lead early but I delayed it until a click), but that seems of a lesser priority to me and can be tracked in a separate task.

I think we are done here, the code in our direct control takes just a couple of ms now. Before closing this task I recommend someone else to verify this, and also to file a new bug for the issues Krinkle pointed out but which were not fixed here.

Change 287569 had a related patch set uploaded (by Santhosh):
Compact links: Avoid duplicate call of filterByCommonLanguages

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

Change 287569 merged by jenkins-bot:
Compact links: Avoid duplicate call of filterByCommonLanguages

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

Agreed. The change from jquery-i18n to mw-msg made a big difference already (https://gerrit.wikimedia.org/r/#/c/284868/2/resources/js/ext.uls.compactlinks.js).

I'll do quick analysis later this week to see the current status, but I expect nothing major to come up. This all looks great. Thanks!

Arrbee closed this task as Resolved.May 17 2016, 7:01 AM
Arrbee moved this task from QA to Done on the Language-Q4-2016-Sprint 2 board.