Page MenuHomePhabricator

CentralNotice shouldn't load JS mixins for campaigns targeting other Geos
Open, LowPublic

Description

There are probably other targeting factors that can be applied before loading the mixins, but geolocation seems like the biggest one that would let us avoid loading unneeded modules for a lot of readers.

Right now it seems like if a campaign that requires special mixins is running somewhere, all users of that wiki load that CN mixin. Since the information about mixins seems to come from choiceData, where Geo targeting is also defined, it seems like the decision to load the mixin or not could depend on the geotargeting, to avoid loading mixin modules that won't be needed by the current user.

Right now, browsing enwiki from France, I see that there are two campaigns requesting the largeBannerLimit mixin:

Both of which are targeting the Netherlands, where I'm not.

Yet, I'm loading the largeBannerLimit mixin:

I don't know if mixins can affect the targeting logic. If they don't, they should only be loaded after all targeting logic has run, to make sure that extra JS modules are only loaded for users that need them.

Event Timeline

Gilles created this task.May 2 2018, 7:42 AM

Thanks!! :) This is correct, however it's blocked by 1) lack of geolocation information from the load.php entry point and 2) inability to split the RL cache on country. If those limitations are solved, it'd be pretty simple to do this.

We have indeed discussed this a few times... My understanding has been that it's better to focus on other medium-term routes to improving CentralNotice performance (like Web Workers).

CentralNotice needs to choose and load a banner early on in the pageview, so, I don't think we can load these modules asynchronously.

I don't know if mixins can affect the targeting logic. If they don't, they should only be loaded after all targeting logic has run

They don't affect general targeting logic per se , but they do help determine whether a banner is shown to a given user, and which banner may be shown, so they must run before a banner can be loaded. To make these determinations, they use data that's only available on the client (in LocalStorage).

Mmm just one further note, if we could split the cache on country and get that data into ResourceLoaderContext, we could also send a lot less choices to each client, which would also be a win performancewise.

See CNChoiceDataResourceLoaderModule::getDependencies() and ChoiceDataProvider::getChoices().

I'm afraid splitting the cache by country isn't feasible any time soon.

On topic of CentralNotice, the internal review has spawned T192623 which is going to eliminate 1 of the serial roundtrips required for CentralNotice banner display (as well as for everything else!).

Regarding the mixin dependencies, I'm surprised that the mixins themselves would be used by the CentralNotice client in its decision making process. If I recall correctly, in 2015 we worked together to augment BannerChoices to support dynamic dependencies for the purpose of not needing to lazy-load them after a banner was chosen. Basically, before that, the flow looked as follows:

  1. startup: request base modules
  2. base modules: request page modules
  3. page modules (incl. CentralNotice client and banner choices): decide on banner, then request the banner's RL dependencies, and once they arrive, make the final request for the banner HTML.
  4. chosen banner's dependencies/mixin modules.
  5. banner HTML (from API or page, not RL).

When we added the dynamic getDependencies() to ResourceLoader for CN (7e0d3369c, T98924; 2a46d85a2a), it resulted in req 4 going away in favour of BannerChoices preloading all mixins for the given choices. That brought us to the current situation of 4 serial roundtrips instead of 5 (not counting the HTML).

It sounds like the the client logic for deciding banners does itself depend on the individual campaign/banners's dependencies. If that is the case, then never mind. But if that isn't the case, then I was thinking we could perhaps come up with a way to load modules and banner HTML in the same request. I actually have some ideas for how to make that happen.

That would reduce overhead of JS download/parsing/pre-execution (for this task) without changing the number of roundtrips.

Gilles added a comment.EditedMay 4 2018, 7:56 AM

Why wouldn't splitting the cache by country be feasible? Even on a cold browser cache, Varnish gets the GeoIP cookie on the very first load.php request, since it's the first pageload that sets the cookie.

And in a worst case scenario where the cookie isn't there, which should be rare, we can serve all the choices like we do now.

AndyRussG added a comment.EditedMay 30 2018, 3:17 AM

On topic of CentralNotice, the internal review has spawned T192623 which is going to eliminate 1 of the serial roundtrips required for CentralNotice banner display (as well as for everything else!).

Cool!

Regarding the mixin dependencies, I'm surprised that the mixins themselves would be used by the CentralNotice client in its decision making process. If I recall correctly, in 2015 we worked together to augment BannerChoices to support dynamic dependencies for the purpose of not needing to lazy-load them after a banner was chosen.

Mmmm... the current campaign mixin system was originally created with the dynamic dependencies... Mostly equivalent functionality that existed before that was achieved with in-banner JS. However, that meant that often the banner itself would be fetched and injected, but not actually displayed to the user. It also placed limits on the now-standard large-banner-followed-by-small-banner FR practice.

(There was also another, older CentralNotice feature, also called "mixins", or "banner mixins", that had some of the same goals, though it was pretty different...)

I was thinking we could perhaps come up with a way to load modules and banner HTML in the same request. I actually have some ideas for how to make that happen.

That would reduce overhead of JS download/parsing/pre-execution (for this task) without changing the number of roundtrips.

Interesting! Potentially, we could have some of the mixins loaded together with the banner. Of the six existing mixins, five use the pre-banner hook to run code before the banner is chosen and loaded (i.e., shortly after a campaign is chosen), and, of those, two might be re-factored to be able to load with the banner. Three mixins (largeBannerLimit, impressionDiet and bannerSequence) do impact on whether a banner is shown, and if so, which one.

So, potentially, we could add a system whereby only certain mixins load as dependencies of ChoiceData, and others are bundled with banner content, as needed.

A simpler, quick win on this point might also be to split ChoiceData on platform (desktop vs. mobile). Many campaigns only target one or the other, and the cache is already split on host, I think...

Thanks much!!! :)

Vvjjkkii renamed this task from CentralNotice shouldn't load JS mixins for campaigns targeting other Geos to itdaaaaaaa.Jul 1 2018, 1:13 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from itdaaaaaaa to CentralNotice shouldn't load JS mixins for campaigns targeting other Geos.Jul 1 2018, 9:26 AM
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.
CommunityTechBot raised the priority of this task from High to Needs Triage.Jul 3 2018, 1:57 AM

TODO: quantify actual impact

Gilles triaged this task as Low priority.Jun 6 2019, 10:03 AM