Page MenuHomePhabricator

Minimise the code QuickSurveys loads when there are no surveys running
Open, LowPublic5 Story Points

Description

Background

Currently, QuickSurveys adds the ext.quicksurveys.init RL module to the output if the page exists, regardless of whether there are any surveys defined and that are enabled. This causes two issues, one minor and one major in the case of the Wikipedias:

  1. There's is a performance penalty as the module (and its ext.quicksurveys.lib dependency) is loaded for every mainspace page that exists
  2. If you have responses cached at the edge and disable the QuickSurveys extension, then RL will trigger an error for every mainspace pageview (for pages that exist) as the ext.quicksurveys.init module no longer exists but is still referenced

Performance

ext.quicksurveys.lib is 2.4kb and ext.quicksurveys.init 641 bytes.

Testing Environment for QA

None. However, see T209882#4868432 for an example of 2 that occurred in the wild.

Acceptance Criteria

  • When there are no surveys defined, then the ext.quicksurveys.init RL module is not added to the output (or the code it loads is minimised to a conditional client side check)
  • When there are no surveys enabled, then the ext.quicksurveys.init RL module is not added to the output (or the code it loads is minimised to a conditional client side check)
  • This behaviour is documented
    • Including a note about not immediately disabling the extension after running a survey

Notes

@Krinkle's suggested approach: T213459#4871107

Event Timeline

phuedx created this task.Jan 10 2019, 5:28 PM
Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptJan 10 2019, 5:28 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jdlrobson triaged this task as High priority.EditedJan 10 2019, 5:43 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: Readers-Web-Backlog.
Jdlrobson moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: ovasileva.

Even though this doesn't relate to our current goals, I'd like to propose we work on this asap @ovasileva as right now we're shipping a significant amount of unnecessary JS to our mobile users and there is a risk right now that if new surveys are enabled, we will hit client side error spikes again.

phuedx updated the task description. (Show Details)EditedJan 10 2019, 5:54 PM

I removed the duplicate information from the description but should clarify the following:

Right now, the extension has to be disabled to remove a survey and the associated performance penalty. We also ship additional JS unnecessarily to wikis which have QuickSurveys enabled because of T159738.

The extension doesn't need to be disabled to remove a survey. Surveys can be marked as disabled and their configuration won't be sent to the client. However, if there are no enabled surveys, then the ext.quicksurveys.init RL module is still added to the output. Changing this behaviour is covered in the second acceptance criterion.

T159738: Limit default styles loaded by OOUI (85kb after gzipping penalty to run a QuickSurveys) by droping use of oojs-ui-widgets only occurs when the user sees a survey. That being said, we should still fix it.

Thanks for the clarification! So not quite as bad as I first thought :)

Jdlrobson added a comment.EditedJan 10 2019, 6:26 PM

When there are no surveys defined, then the ext.quicksurveys.init RL module is not added to the output

So the downside of not adding ext.quicksurveys.init to the page is that enabled surveys will not show instantly (due to cached HTML!) so we should probably retain loading this unconditionally. However this code should be simplified to check the config value and load the rest of the code including ext.quicksurveys.lib conditionally.

pseudo code:

ext.quick.surveys.init would be simplified to:

if ( mw.config.get( 'wgEnabledQuickSurveys' ) ) {
  mw.loader.using( 'ext.quicksurveys.lib' )
}
ovasileva set the point value for this task to 5.Jan 10 2019, 6:29 PM
Jdlrobson renamed this task from Don't load the QuickSurveys init module shouldn't be loaded when there are no surveys running to Minimise the code QuickSurveys loads when there are no surveys running.Jan 10 2019, 7:24 PM
Jdlrobson added a project: Performance-Team.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Krinkle.

Thanks for looking at this. I have a question and two recommendations.

Question: Can we quantify and measure the current impact we think this has on users?

Recommendation for proposal as written:

  • I support the idea of making ext.quicksurveys.init no longer hard-depend on `ext.quicksurveys.lib. Separating the init from the rest makes sense.
  • Today, all three modules (..init, ..lib, ..views) are always loaded together, so they really should've been a single bundle rather than three separate modules. I'd recommend merge the ext.quicksurveys.lib and ext.quicksurveys.views bundles. They're never needed separately and would help towards T202154 goal and script size.
  • Update the server-side hook that enqueues the init module to also independently enqueue ext.quicksurveys.lib alongside ext.quicksurveys.init when surveys are enabled. This ensures that in the vast majority of cases, surveys will not have a worse latency than they have today. And it will avoid a significant regression to loadEventEnd that would otherwise happen for those users. I believe that given latency for lazy-load can be upwards from 3 seconds, it would also negatively impact survey impressions and diversity of the answers if we don't preload in the common case. This means, when surveys just got activated, during the first 24 hours some users lazy-load and some preload. And during the 24h after a survey ends, a few still have it preloaded as the cache churns back. This seems quite acceptable in my opinion, given how this is still relatively small impact overall. But, I think we could be more ambitious here and not even pay this cost. See alternate proposal :)

Alternate proposal:

As Jon's comment at T213459#4870585 shows, really even the remaining init logic could be moved to lib. The only thing we need to is to check the config. And as shown above, this could be checked server-side as well. And if you do both, the client check will be a no-op most cases (rides along the existing promise). That's fine, but we can do better.

The startup module doesn't have to be static. It provides a lot of power, and a lot of the major wins came from embracing that power. This is what we ended up doing with CentralNotice in 2015, worked out really well, and was actually quite easy in retrospect.

It works like this:

  • Merge the two view and lib modules (as before).
  • Enqueue ..init on all pages unconditionally (as before).
  • Vary the definition of the init module in a FileModule subclass by site config. This means, the module is defined as empty and dependency-free when there are no surveys; And empty with lib as its dependency (basically, an alias) when there are surveys.

Outcome: When surveys are enabled, all pages (including cached) instantly start loading lib without lazy-load or delay. When surveys are disabled, all pages (including cached) stop loading lib, and not even the three lines of code for init.

Implementation sketch:

extension.json
{
 "modules": {
    "ext.quicksurveys.lib": ..,
    "ext.quicksurveys.init": { "class": "MediaWiki\\QuickSurveys\\Module" }
  }
}
Module.php
class Module extends \ResourceLoaderFileModule {
  getDependencies( $context ) {
    return $context->getConfig()->get('EnabledSurveys') ? [ 'ext.quicksurveys.lib' ] : [];
  }
}

@Krinkle your suggestions sound great to me.

Very neat, @Krinkle! That approach'll require a fair bit of explanation in the QuickSurveys documentation for future developers (covered by AC 3)

phuedx updated the task description. (Show Details)Jan 11 2019, 4:43 PM
Jdlrobson lowered the priority of this task from High to Normal.Jan 14 2019, 7:12 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Upcoming to Triaged but Future on the Readers-Web-Backlog board.

Since we're only loading an additional 2.7kb dropping this to normal - we are not pulling in oojs-ui as I first expected.

Krinkle moved this task from Inbox to Radar on the Performance-Team board.Jan 14 2019, 8:54 PM
Krinkle edited projects, added Performance-Team (Radar); removed Performance-Team.
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.
Krinkle moved this task from Watching to Perf issue on the Performance-Team (Radar) board.
Jdlrobson moved this task from Backlog to Bugs on the QuickSurveys board.Jan 15 2019, 5:53 PM
Jdlrobson lowered the priority of this task from Normal to Low.Wed, Jul 31, 6:56 PM