Page MenuHomePhabricator

Improve code quality for Mobile Preferences toggle switches
Open, Needs TriagePublic

Description

As our team is becoming familiar with all of the available code libraries to implement client side features, we're finding learning opportunities, and T317117 is one of those. In this task, we're going to optimize a feature based on our evolving understanding of how to contain the jquery emanating from OOUI. We will improve performance and legibility in the process.

Event Timeline

Change 857747 had a related patch set uploaded (by Jsn.sherman; author: Jsn.sherman):

[mediawiki/core@master] mobile preferences: optimize toggle switches

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

Test wiki created on Patch demo by JSherman (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/96f76bc491/w

Test wiki created on Patch demo by JSherman (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/313dbcd50d/w

I've created a gerrit change request with several patchset that take individual optimization steps. I've noted the local runtime that I logged for the function and updated it in the comments. The first patch set just names the function to make it easier to profile (for a baseline), then the following patch sets progressively optimize the code and update the measured runtime for the function in the comment.
I'm creating patchdemos for the baseline and final patch sets so I we can all check it out live on our machines. There will be run-to-run variability, and I did multiple runs locally when I was testing this, but I'm including some one off checks from the patchdemos here.

To do some basic performance profiling:

baseline (~62ms):

  • login to the baseline patchdemo https://patchdemo.wmflabs.org/wikis/96f76bc491
  • go to preferences and select the mobile view
  • in chrome/edge/chromium open dev tools and select the performance tab
  • click the "start profiling and reload page" icon near the top left of the performance panel
  • when profiling is complete, click into the the summary and hit ctrl + f
  • enter insertToggles into the search box
  • note the aggregated time
    image.png (560×864 px, 59 KB)

optimised (~13ms):

  • login to the optimized patchdemo https://patchdemo.wmflabs.org/wikis/313dbcd50d
  • go to preferences and select the mobile view
  • in chrome/edge/chromium open dev tools and select the performance tab
  • click the "start profiling and reload page" icon near the top left of the performance panel
  • when profiling is complete, click into the the summary and hit ctrl + f
  • enter insertToggles into the search box
  • note the aggregated time
    image.png (560×864 px, 63 KB)

Change 857747 merged by jenkins-bot:

[mediawiki/core@master] mobile preferences: optimize toggle switches

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

Sam Walton noticed a bug for T317117 in beta which patch 857747 inadvertently resolves, so I merged it for expediency. We still intend to spend more time looking at our code quality here and will also need to remove the educational comments. Patches related to that will be filed against this task.

Thanks Jason, all of your work on this has really made a big difference

Change 863017 had a related patch set uploaded (by Jsn.sherman; author: Jsn.sherman):

[mediawiki/core@master] mobile preferences: trim insertToggles() comments

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

Change 863017 merged by jenkins-bot:

[mediawiki/core@master] mobile preferences: trim insertToggles() comments

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