Eplanning: new prioritization metric for adunit sizes

[chino117]

Type of change:

Feature

Description of change:

New prioritization metric for adunit sizes

contact email of the adapter’s maintainer:

chino117@hotmail.com

[SyntaxNode]

I recommend moving the sorter to its own file due to its non-trivial logic. I'd also recommend coded unit tests (not using the json framework) for this sorting behavior specifically.

[SyntaxNode]

Sorting algorithms are always fun to read. Could you please describe the algorithm to assist with the review?

I'm a bit concerned with the performance of building the size1 and size2 strings on each compare. Perhaps that burden can be reduced with memoization.. although before I perhaps prematurely worry about performance, how many formats do you typically see in requests?

[chino117]

#1648 (comment) :
The code can be simplified, it's true, I did it like this for a more descriptive logic, I change it if you want to as you suggest.

#1648 (comment) :
The priorityOrderForMobileSizesAsc and priorityOrderForDesktopSizesAsc arrays are sorted in ascending order of priority, so the last item is the highest priority.
If the size is in the array, the indexOf function gives the index of its position within the array, if it is not defined, it returns -1.
What you want to achieve with the ordering is that the one with the highest priority is in the first position of the sizesSortedByPriority array.
So:
-if both sizes are defined, we take the one with the highest index as the smallest
-in case that one of the two sizes is not defined, the one that is defined is taken as smaller
-in case none is defined, the order is maintained

#1648 (comment) :
As for the formats, they are usually 3 to 4

[guscarreon]

Overall looks pretty good. Left a couple of comments

[guscarreon]

I just want to double-check on the sorting logic. The first element in priorityOrderForMobileSizesAsc has the highest priority and should be placed last in the sorted list, right? Just for the sake of understanding the changes better I wrote the test cases shown below. Is the behavior they unveil correct? Is this the intended logic? (I find test case #4 particularly interesting)

// Some test I wrote just to help me understand the sorting logic better
func TestLessByPriorityMobile(t *testing.T) {
	testCases := []struct {
		desc          string
		inFormatList  []openrtb.Format
		expectedOrder []openrtb.Format
	}{
		//priorityOrderForMobileSizesAsc = []string{"1x1", "300x50", "320x50", "300x250"}
		{
			desc: "1) No WxH is listed in priorityOrderForMobileSizesAsc list doesn't change",
			inFormatList: []openrtb.Format{
				{W: 640, H: 480},
				{W: 320, H: 240},
			},
			expectedOrder: []openrtb.Format{
				{W: 640, H: 480},
				{W: 320, H: 240},
			},
		},
		{
			desc: "2) One entry listed in priorityOrderForMobileSizesAsc, that entry will be swapped to the last place",
			inFormatList: []openrtb.Format{
				{W: 1, H: 1},
				{W: 1080, H: 780},
				{W: 640, H: 480},
			},
			expectedOrder: []openrtb.Format{
				{W: 1080, H: 780},
				{W: 640, H: 480},
				{W: 1, H: 1},
			},
		},
		{
			desc: "3) Two entries found in priorityOrderForMobileSizesAsc and will be swapped to appear in order",
			inFormatList: []openrtb.Format{
				{W: 300, H: 250},
				{W: 1, H: 1},
			},
			expectedOrder: []openrtb.Format{
				{W: 300, H: 250},
				{W: 1, H: 1},
			},
		},
		{
			desc: "4) List exactly like priorityOrderForMobileSizesAsc. Entries will be reversed.",
			inFormatList: []openrtb.Format{
				{W: 1, H: 1},
				{W: 300, H: 50},
				{W: 320, H: 50},
				{W: 300, H: 250},
			},
			expectedOrder: []openrtb.Format{
				{W: 300, H: 250},
				{W: 320, H: 50},
				{W: 300, H: 50},
				{W: 1, H: 1},
			},
		},
	}
	for i, test := range testCases {

		actualOrder := test.inFormatList
		sort.Sort(byPriorityMobile(actualOrder))

		assert.Equal(t, test.expectedOrder, actualOrder, "Test %d not ok", i+1)
	}
}
adapters/eplanning/eplanning_test.go

The main reason behind wanting to double-check this is because in line 311 shown below it seems to me that the very first non-zero element (that can be the one with lowest priority) would be selected:

310         for _, format := range sizesSortedByPriority {
311             if format.W != 0 && format.H != 0 {
312                 return format.W, format.H
313             }
314         }
315     }
316
317     return 0, 0
318 }
adapters/eplanning/eplanning.go

[guscarreon]

If at the end, only one format is going to be included in the ImpExt, do we need to sort the entire list? Assuming the format list comes with N entries, sorting comes at a cost of O(NlogN). But, if we just compare the format list and select a match from the priorityOrderFor<DEVICE>Sizes list, we could, at the cost of a little memory that Prebid Server could spare, do this in O(N+K) time instead. This seems like a good optimization opportunity given that the sorting algorithm you are currently using swaps to the end only the priorityOrder matches (see test case number two of the TestLessByPriorityMobile(t *testing.T) I wrote).

Maybe we could do something similar to:

298   func getSizeFromImp(imp *openrtb.Imp, isMobile bool) (uint64, uint64) {
299       if imp.Banner.W != nil && imp.Banner.H != nil {
300           return *imp.Banner.W, *imp.Banner.H
301       }
302  
303       if imp.Banner.Format != nil {
304 -         sizesSortedByPriority := imp.Banner.Format
305 -         if isMobile {
306 -             sort.Sort(byPriorityMobile(sizesSortedByPriority))
307 -         } else {
308 -             sort.Sort(byPriorityDesktop(sizesSortedByPriority))
309 -         }
310 -         for _, format := range sizesSortedByPriority {
311 -             if format.W != 0 && format.H != 0 {
312 -                 return format.W, format.H
313 -             }
314 -         }
    +         // Space O(n)
    +         hashedFormats := make(map[string]int,0)
    +
    +         // Traverse format array in O(n) time
    +         for i, format := range imp.Banner.Format {
    +             if format.W != 0 && format.H != 0 {
    +                 hashedFormats[fmt.Sprintf("%dx%d", format.W, format.H)] = i
    +             }
    +         }
    +         // Traverse prioritized formats in O(k) time
    +         for i:=len(priorityOrderFor__MobileOrDesktop__SizesAsc)-1; i>=0; i-- {
    +             //   ^--- Start with the highest priority in priorityOrderFor__MobileOrDesktop__SizesAsc
    +
    +             if formatIndex, wasFound := hashedFormats[priorityOrderFor__MobileOrDesktop__SizesAsc]; wasFound {
    +                 return imp.Banner.Format[formatIndex].W, imp.Banner.Format[formatIndex].H
    +             }
    +         }
315       }
316  
317       return 0, 0
318   }
adapters/eplanning/eplanning.go

[chino117]

#1648 (comment)
No, the logic is the last element of priorityOrderForMobileSizesAsc has the highest priority and must be placed first in the ordered list.

#1648 (comment)
I liked your suggestion and I have taken your code and made some modifications to it so that it complies with the logic that my implementation had.

[guscarreon]

LGTM. Thank you for addressing our comments.

[SyntaxNode]

Looks great. Thank you for working with us on the feedback. Just one small optimization comment.

[chino117]

Yes, no problem

[SyntaxNode]

Yes, no problem

Fantastic! Thank you.

[guscarreon]

LGTM