[amp-auto-ads] Multiple amp-auto-ads on the same page - adConstraints are not kept

[Zer0Divis0r]

I am trying to understand if multiple amp-auto-ads from different networks would work correctly if implemented simultaneously on the same page. Currently I am facing the issue that adConstraints are not really kept. I think that each auto-ads instance keeps distance from ads that were previously present on the page, but they ignore ads created by each other.

Here is example link, where both AdSense and FirstImpression.io are implemented, both networks have constraint of at least one viewport height distance between ads, but ads still appear next to each other.

https://www.favecrafts.com/Knit-Baby-Blankets/Easy-Free-Knitting-Patterns-and-Help/amp#fi_reveal=113853 (preferably to look in mobile emulator)

Here how it looks like were the problem exists, ads are too close to each other:

Is this a config issue, or multiple amp-auto-ads are expeted to work like that?

Additional info:

AdConstriants from AdSense:

"adConstraints": {
  "initialMinSpacing":"1vp",
  "subsequentMinSpacing": [
    {"adCount":3,"spacing":"2vp"},
    {"adCount":6,"spacing":"3vp"}
  ],
  "maxAdCount":8
}

AdConstraints from FirstImpression.io:

"adConstraints": {
 "initialMinSpacing":"609px",
 "subsequentMinSpacing": [{"adCount":10,"spacing":"200px"}],
 "maxAdCount":100
}

@tlong2

[tlong2]

I think there is an implicit assumption in the code that there will only ever be one amp-auto-ads trying to place ads within the page.

When the code runs to process an amp-auto-ads tag, it creates an AdTracker which keeps track of where ads are on the page: https://github.com/ampproject/amphtml/blob/master/extensions/amp-auto-ads/0.1/amp-auto-ads.js#L139 This is initially constructed by find all amp-ads on the page, and is then incrementally updated with any additional ads that amp-auto-ads places: https://github.com/ampproject/amphtml/blob/master/extensions/amp-auto-ads/0.1/ad-strategy.js#L116

I think the issue is that each instance of amp-auto-ads will maintain its own instance of AdTracker. So they will never know about the ads that the other instance has placed. A couple of ways of solving this would be:

1. Maintain some kind of global AdTracker that all instances of amp-auto-ads share
2. Rather than incrementally adding new ads to the AdTracker, just make it do a full lookup of all amp-ads every time it's queried

[Zer0Divis0r]

Thank you for your prompt reply. The industry realities show that publishers add AdSense's auto-ads because it is the easiest, but it's lack of control over ad strategy makes them seek for a solution that offers a more tailored ad placements set. Removing AdSense is not an option, becasue they want a supplimentary solution, aside from Google's.

So, do I undderstand correctly, that to implement solution # 2 all that has to be done:

• add a method to AdTracker to refresh the ads in the tracker. The method should accept ampdoc as an argument.
• add a call to that function just before nextPlacement.placeAd(). The only complication here that I see is how to pass the ampdoc to AdStrategy so that it can pass it to the method to refresh ads.

Do I miss anything?

Thanks.

[tlong2]

Thinking about it more... I guess that would work some of the time, but there will probably be some issues with race conditions, since all this stuff is happening asynchronously and returning promises. One amp-auto-ads might place an ad while the other amp-auto-ad's AdTracker is iterating through the list of ads it knows about. I.e., the AdTracker can become stale between the call to the new function you mention and the AdTracker actually being used.

I think the only way to avoid the race condition issue would be to implement some kind of lock on placing ads that all amp-auto-ad instances respect. It might just be easiest to do this at the amp-auto-ads entry point, so one amp-auto-ads tag runs its strategy and blocks all others, then once it's finished, the next amp-auto-ads tag can run its strategy.

Maybe not relevant if we just go down the route of making amp-auto-ads run sequentially, but in response to "The only complication here that I see is how to pass the ampdoc to AdStrategy"... Rather than passing ampdoc to AdStrategy and then passing it into that new function, it might be easier to just construct AdTracker with ampdoc. That should be easy enough since ampdoc is available when AdTracker is created: https://github.com/ampproject/amphtml/blob/master/extensions/amp-auto-ads/0.1/amp-auto-ads.js#L139

[Zer0Divis0r]

The function getExistingAds() is actually synchronous, it does not return a Promise. How about adding a call to it just before isTooNearAnAd() here - https://github.com/ampproject/amphtml/blob/90c63b843c432840346562e8978ccf3a06e7eac7/extensions/amp-auto-ads/0.1/placement.js#L197 ?

[tlong2]

But the functions within AdTracker related to actually measuring whether or not the proposed position is too near to an ad are asynchronous. isTooNearAnAd() calls isWithinMinDistanceOfAd_() which asynchronously iterates over the the list of ads. So the change you propose would result in an algorithm that looks something like:

• Update list of ads by calling getExistingAds()
• Call isTooNearAnAd() -- Calls isWithinMinDistanceOfAd_ --- Calls getDistanceFromAd_ for 1st ad [JS thread yields] Processes distance from 1st ad --- Calls getDistanceFromAd_ for 2nd ad [JS thread yields] Processes distance from 2nd ad --- Calls getDistanceFromAd_ for 3rd ad [JS thread yields] Processes distance from 3rd ad ...

During any one of those [JS thread yields] the other instance of amp-auto-ads on the page could insert an ad meaning that the list of existing ads becomes stale. Given the amount of yielding going on, and given that both instances of amp-auto-ads will be trying to run at the same time, I think the chance of race conditions resulting in buggy behavior are reasonably high.

I think the simplest solution that will work is to just make sure that only one instance of amp-auto-ads runs at a time. Or alternatively protect the Placement.placeAd() function with a global mutex (i.e. a single mutex that applies to all instances of the class). That would prevent any instance amp-auto-ads from inserting an ad while another instance is in the processing of trying to insert one.

[Zer0Divis0r]

Making a single mutex in AmpAutoAds.placeAds_() will be enough to ensure that only one instance runs at the same time, no? Is there any lib already in the project that can be used for that?

[tlong2]

Yes, I think that should work. I'm not aware of any existing lib in the project for doing mutexs. But I'm also not that familiar with the majority of the amphtml codebase. @lannka might know of an appropriate library.

[Zer0Divis0r]

@lannka , can you please comment on this? Is there any lib in the probect to do a mutex? Thanks.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.