Page MenuHomePhabricator

Spike: How to implement performant campaign-associated mixins
Closed, ResolvedPublic1 Estimate Story Points

Related Objects

StatusSubtypeAssignedTask
DuplicateNone
OpenNone
DeclinedNone
Resolved atgo
ResolvedAndyRussG
ResolvedAndyRussG
ResolvedAndyRussG
ResolvedAndyRussG
ResolvedAndyRussG
Resolvedawight
ResolvedNone
ResolvedEjegg
ResolvedNone
Resolved ellery
Resolvedgreg
Resolvedawight
ResolvedAndyRussG
ResolvedAndyRussG
ResolvedAndyRussG
ResolvedAndyRussG
ResolvedNone
Resolvedawight
ResolvedAndyRussG

Event Timeline

atgo created this task.Feb 26 2015, 7:46 PM
atgo raised the priority of this task from to Medium.
atgo updated the task description. (Show Details)
atgo set Security to None.
atgo moved this task from In Analysis to Dev Ready on the Fundraising Sprint Enya board.
atgo added subscribers: atgo, Aklapper, AndyRussG.
atgo edited a custom field.

Here's the approach that we've settled on:

  • Campaign-associated Mixins should be RL modules
  • Instead of carrying JS, choiceData will just have the names of any campaign-associated mixin modules needed and the parameters to be used
  • bannerLoader will call mw.loader.load for the modules needed. The first time a user is targeted by a campaign that needs any given module, this will add a request to the process, but on subsequent page views, the module should be retrieved from the browser's localStorage. So, no more extra requests after the first one.

Non-blocking issues and considerations:

  • Some refactoring of the JS currently in-banner will be needed.
  • We'll try to consolidate bits of JS that are currently used together. It doesn't make sense to have very small modules or to spam the RL module registry.
  • We should only mixinize bits of JS that aren't expected to change too frequently, and make sure that the Mixins' parameters can handle most forseeable tuning needs. Changes in the actual JS will require a deploy.

The other option that was considered was: add commonly used functions to bannerController.lib and include actual small JS snippets in choiceData. However, it looks like the above approach will be more performant overall.

@Ejegg and @Krinkle have looked over this plan (though they shouldn't get any blame if the plane runs ahsore).

AndyRussG closed this task as Resolved.Mar 11 2015, 2:06 AM
AndyRussG moved this task from Doing to Done on the Fundraising Sprint Flaming Lips board.
awight added a subscriber: awight.Mar 11 2015, 2:39 AM

That sounds great! One small question though, since the RL modules are aggressively cached as you mention, why bother with the extra complexity of passing the module name and then loading explicitly, rather than just loading the module on the first pageview along with the others, if one of the campaigns targeting the viewer requires it?

Should we have this in "pending review"?

That sounds great! One small question though, since the RL modules are aggressively cached as you mention, why bother with the extra complexity of passing the module name and then loading explicitly, rather than just loading the module on the first pageview along with the others, if one of the campaigns targeting the viewer requires it?

Hmmm! WRT loading right away if any campaign choices require it: that would be great—if we can do it! Whether or not it's possible I think will depend on the delicacies of RL module caching and dependency resolution. Potentially we could make CNBannerChoiceDataResourceLoaderModule add dependencies based on the choices available... maybe... :)

I'd be mainly interested in that route if it could save us the extra request the first time a mixin module is needed. WRT the complexity of passing the module name, I'm not too worried, since we have to pass the name of the mixin required, in any case. True, the dynamic load may bring in some complexity for coordinating all the pieces of JS to run in the right order, but I think that'd be OK if it's the best option performance-wise.

Should we have this in "pending review"?

Yuup! Unclosing and backwardsing to PR.

AndyRussG reopened this task as Open.Mar 11 2015, 2:23 PM
AndyRussG moved this task from Done to Review on the Fundraising Sprint Flaming Lips board.

It looks like we could override getDependencies() in CNBannerChoiceDataResourceLoaderModule. That's called (eventually) via ResourceLoaderStartUpModule::getScript(). So in getDependencies() we could check the banner choices and add required mixin modules as dependencies. So long as caching of the startup module is long enough that we don't get very frequent calls, but not longer than current caching for CNBannerChoiceDataResourceLoaderModule, I think we can do it. :) (Just trying to grok the varnish setup again for that...)

Right, I hadn't thought this through, now I see why you're planning to load the extra RL modules manually. Still, the getDependencies thing, if it works, might pull the extra modules in slightly earlier (second or third initial RL rounds) than if we're putting this logic in the banner controller.

It's probably worthwhile to investigate, simply because the first and second impression viewers are such a delectable target :)

OK! I don't see anything in the varnish config that would make the startup RL module caching different from any others. Let's try this first!

AndyRussG closed this task as Resolved.Mar 11 2015, 8:53 PM
AndyRussG moved this task from Review to Done on the Fundraising Sprint Flaming Lips board.

Change 198267 had a related patch set uploaded (by AndyRussG):
WIP Campaign-associated mixins: schema update

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

mmodell removed a subscriber: awight.Jun 22 2017, 9:52 PM