Page MenuHomePhabricator

Performance review of client-side-only version of Graph extension
Open, Stalled, MediumPublic

Description

Description

MediaWiki-extensions-Graph currently uses the Graphoid service to render graphs on the backend. In order to retire that service, we are planning to switch on the existing front-end-only graph rendering that already exists in the extension. This work has not yet started, as the contractor tasked with it has not yet been hired - we anticipate it will be a short project, and ready for review on beta sometime in February, at the latest.

Preview environment

N/A currently. Listed in TODOs below.

Which code to review

Generally: MediaWiki-extensions-Graph

Specifically: Whatever changes we make to the code for maintenance purposes, bugfixes, etc. - TBD at the moment.

Performance assessment

Please initiate the performance assessment by answering the below:

  • What work has been done to ensure the best possible performance of the feature?
    • TBD
  • What are likely to be the weak areas (e.g. bottlenecks) of the code in terms of performance?
  • Are there potential optimisations that haven't been performed yet?
    • TBD
  • Please list which performance measurements are in place for the feature and/or what you've measured ad-hoc so far. If you are unsure what to measure, ask the Performance Team for advice: performance-team@wikimedia.org.
    • TBD

TODO (for contractor):

  • Test configuration change
  • Make changes to Graph extension for maintenance/bugfixes as needed
  • List changes here for examination
  • Deploy config change and any new code to beta
  • Create a test page with at least 1 graph
  • Ping this ticket and edit this description to link to the test page
  • Briefly audit performance, answer above questions.

P.S., sorry for all of the TBD/TODO items. Given the requirement for lead-time and our current lack of progress, we wanted to give you all a heads up so you can schedule a cursory review of the code in time for our rough deadline (middle of Q3).

Event Timeline

Restricted Application added subscribers: Liuxinyu970226, Aklapper. · View Herald TranscriptDec 12 2019, 9:53 PM
Krinkle renamed this task from Performance review of front-end-only version of Graph extension to Performance review of client-side-only version of Graph extension.Dec 16 2019, 8:58 PM
Gilles assigned this task to Peter.Dec 16 2019, 9:01 PM
Gilles moved this task from Inbox to Backlog: Future Goals on the Performance-Team board.
Gilles claimed this task.May 4 2020, 11:37 AM
Gilles triaged this task as Medium priority.
Gilles added a subscriber: Peter.

@MarkTraceur what's the current status of this product? Can I try it somewhere on Beta?

Gilles changed the task status from Open to Stalled.May 4 2020, 11:44 AM
Jseddon added a subscriber: Jseddon.Jun 9 2020, 5:46 PM

@Jseddon Let me know where we can test this

Gilles changed the task status from Stalled to Open.Jun 25 2020, 8:46 AM

From a conversation with Seddon:

currently the change is deployed to test.wp wiki and mediawikiwiki
test2 wiki still is using graphoid

Not a performance bug, but FYI there is a vertical alignment issue when the browser window is not very wide and there is an image inside the infobox. The spinner is in the right spot, but then the graph appears below the infobox instead of next to it:

Here is my first pass of review. I might have missed small things, but there's already a lot to address.

There is no visual fallback when JS is disabled. I understand that the idea is to stop have server-side rendering for this, but there isn't even any text explaining that there's supposed to be a graph there and it couldn't be rendered due to lack of JS (or said JS being blocked). This information needs to be conveyed, particularly for users who may consider enabling JS or unblocking it to view the graph. I see that there's a recent commit by @Jseddon doing something like this that just not be live yet on testwiki.

The progress spinner appears even if it's only going to be visible for a split second. The current performance perception best practice is to only have spinner/progress bars display for processes that take a significant amount of time. Otherwise, the spinner brings attention to a process that may actually be fast, giving the false impression that it's going to take a while. Basically, don't draw attention to something that might be fast. The recommended fix is simple: just wait for 1 second (or maybe even more) before displaying the spinner. In a lot of cases, the graph will appear in less time than that short wait and the user won't have to view the spinner at all. It will look just like images loading on the page. In fact, if the goal is to make it look like an image (like it used to be), doing without the spinner altogether would make it consistent in visual loading behaviour.

The spinner is both embedded in CSS and referred to as a URL (/w/extensions/Graph/includes/ajax-loader.gif?b3af8). Just keep the URL one, which seems to be the one actually used. Remove the embedded copy from the CSS. Images embedded in CSS are now discouraged anyway.

There doesn't seem to be any instrumentation of the performance from the end-user's perspective. This can be a good way to find bugs. Eg. graphs that take a long time to render, or don't render at all. Simply use a mw.track call looking at the difference in monotonic time between when the DOM appears and when the graph has actually been rendered. You can then turn that into a Grafana dashboard very easily. And even set up alerts, which would let you detect major breakages that may happen when you update the extension later, for example.

The JS request oddly includes jQuery jQuery should already be present on any mediawiki pageview. Why does the extension load jquery again?

The JS is huge. 228kb compressed, 852kb to parse uncompressed. Without jquery it's still 184kb compressed/698kb uncompressed. Since the bulk of that is 3rd-party libraries, it might be useful to get a wide selection of graphs and do some usage check on the JS codebase. Just to get an idea of whether we're really using large portions of those libraries or not. It might also be worth checking if the newer versions of those libraries are leaner or more modular. The version of vega used (2.6.3) is 4 years old, current version is 5.13.0. The version of D3 used (3.5.17) is 2 years old, current version is 5.16.0.

Loading the JS and the data isn't parallelised. This is especially critical considering the sheer size of the JS. I see that there's a TODO about this in the code, but that this code is unused at the moment. It seems like vega2 is included with the page directly instead of using ext.graph.loader

The graph JSON isn't cached. The API call that gets it doesn't have any caching parameter. It should be trivial to add those with a maxage parameter. Maybe don't add caching if the user is logged-in so that editors who modify the graph data see their updates immediately, but for unlogged visitors, the graph data should be cached for at the very least a few minutes.

Not a performance bug, but FYI there is a vertical alignment issue when the browser window is not very wide and there is an image inside the infobox. The spinner is in the right spot, but then the graph appears below the infobox instead of next to it:

Yeah I noticed this really unappealing behaiviour when creating the fallback option. The graph dimensions provided by a user is only an approximation of the final generated graph size.

Gilles changed the task status from Open to Stalled.Sep 11 2020, 7:43 AM

Marking this as stalled for my own tracking of performance reviews that are currently actionable for us. While the first pass I've done might not be complete, there is a lot to work on already, and at least file tasks for. Please move this back to open when the various issues discovered have at least been filed as tasks and triaged. Thanks!