Use Concurrency To Download Vendor List #4005

zhongshixi · 2024-10-22T00:53:20Z

Problem
The current implementation of downloading vendor list in preload stage is executed sequentially for individual vendor file - it costs 5 to 10 seconds to download all ~300 vendor files on my local machine ( Apple M1 Pro, 32GB memory).

Solution

the solution is to optimize the preload procedure by applying concurrency which is configurable by the end user. The end user can tune the number depending on their own spec of the machine.
by default, the concurrency is disabled so it wont impact anything. It is end-user's responsibility to provide a number to tune the performance.
in my pbs.yaml config , i set the following value which i observed almost 5x to 10x performance improvement ( ~500 to 600 ms ), any number more than 20 for max_concurrency_init_fetch_specific_version , the performance increase is diminishing

gdpr:
  default_value: "0" # disable gdpr, explicitly specifying a default value is a requirement in prebid server config
  vendorlist_fetcher:
    max_concurrency_init_fetch_latest_version: 2
    max_concurrency_init_fetch_specific_version: 20
  timeouts_ms:
    init_vendorlist_fetches: 30000
    active_vendorlist_fetch: 30000

you are welcome to play around with the solution on your own setting and i highly recommend you see the number yourself.

…ncurrency behaviour; add sample configuratio in the sample

zhongshixi · 2024-10-22T00:54:13Z

gdpr/vendorlist-fetching_test.go

@@ -284,12 +292,6 @@ var vendorList2Expected = testExpected{
 	vendorPurposes:    map[int]bool{1: false, 2: true, 3: true},
 }

-var vendorListFallbackExpected = testExpected{


this code block is not used at all so i deleted it

Did you need to delete it to accomplish your goal in this PR ?

but why we need to keep it if the code is not used at all? there is no comment or description associated with the deleted test case.

i used a practice called grooming as you go - so if you happen to touch the related code piece as part of the solution, then you just do a little bit grooming to avoid large bulk of grooming/refactoring in the future

Ok. I see no problem with that.

sebhtml · 2024-10-22T12:30:16Z

config/config.go

@@ -240,10 +240,21 @@ type Privacy struct {
 	LMT  LMT
 }

+type VendorListFetcher struct {
+	// Max concurrent request for downloading the latest version of the vendor list from a specific major version.


I think that the word "request" should be spelled in the plural form.

sebhtml · 2024-10-22T12:30:23Z

config/config.go

+	// If it is 0 or negative, then means there is no limit for the max concurrency.
+	MaxConcurrencyInitFetchLatestVersion int `mapstructure:"max_concurrency_init_fetch_latest_version"`
+
+	// Max concurrent request for downloading every specific version of the vendor list from its first version to its latest version.


I think that the word "request" should be spelled in the plural form.

sebhtml · 2024-10-22T12:31:10Z

config/config.go

-	Enabled                 bool         `mapstructure:"enabled"`
-	HostVendorID            int          `mapstructure:"host_vendor_id"`
-	DefaultValue            string       `mapstructure:"default_value"`
+	Enabled      bool   `mapstructure:"enabled"`
+	HostVendorID int    `mapstructure:"host_vendor_id"`
+	DefaultValue string `mapstructure:"default_value"`


I think that the vertical alignment was intentional.

Why change the indentation style of those lines ?

@sebhtml do you mind elaborating more about vertical alignment was intentional ? Changing the indentation is automatically done by the go formatter ( i am using MS Visual Studio ) , which i believe it is easier to read.

but i am curious about other technical aspects related to it

The types are vertically aligned:

| v Timeouts GDPRTimeouts `mapstructure:"timeouts_ms"` NonStandardPublishers []string `mapstructure:"non_standard_publishers,flow"` NonStandardPublisherMap map[string]struct{} TCF2 TCF2 `mapstructure:"tcf2"` AMPException bool `mapstructure:"amp_exception"`

see

prebid-server/config/config.go

Line 258 in 0a5b937

Timeouts GDPRTimeouts `mapstructure:"timeouts_ms"`

I don't know if my comment is relevant though.

It may be go fmt's decision; they actually look wrong before.

@bsardo let me know your opinion

sebhtml · 2024-10-22T12:32:43Z

config/config.go

+	v.SetDefault("gdpr.vendorlist_fetcher.max_concurrency_init_fetch_latest_version", 1) // by default one download at a time
+	v.SetDefault("gdpr.vendorlist_fetcher.max_concurrency_init_fetch_latest_version", 1) // by default one download at a time


Why is this line repeated twice ?

good catch, will change it

sebhtml · 2024-10-22T12:44:50Z

gdpr/vendorlist-fetching.go

+	tsStart := time.Now() // For logging how long this takes
+	for _, v := range versions {
+		specVersion := v.specVersion
+		firstVersion := v.firstListVersion
+		wgLatestVersion.Go(func() error {
+			latestVersion := saveOne(ctx, client, urlMaker(specVersion, 0), saver)
+			for i := firstVersion; i < latestVersion; i++ {
+				currentVersion := i
+				wgSpecificVersion.Go(func() error {
+					saveOne(ctx, client, urlMaker(specVersion, currentVersion), saver)
+					return nil
+				})
+			}
+			return nil
+		})
+	}
+
+	wgLatestVersion.Wait()
+	wgSpecificVersion.Wait()


Hi !

I don't know if it matters.

The WaitGroup wgSpecificVersion will likely wait for more Goroutines than wgLatestVersion.

Globally, you start Goroutines with a limit of conf.MaxConcurrencyInitFetchLatestVersion + conf.MaxConcurrencyInitFetchSpecificVersion concurrent Go routines.

So why have 2 wait groups instead of just 1 wait group ?

@sebhtml , good thought, however the reason i did it is to have fine granular control of two different behaviours:

fetching the latest version

fetching each specific version.

plus provide sequential execution as default behaviour so the decision of tuning the controlling parameters is determined by the host.

by using only one wait group, there are two thoughts behind it

conf.MaxConcurrencyInitFetchLatestVersion and conf.MaxConcurrencyInitFetchSpecificVersion loses its meaning since one wait group wont be able to control two types of behaviour precisely.

lets say you use one wait group and the corresponding config is conf.MaxConcurrencyInitFetch , and then you set the value to 1 ( which is supposed to mean it is a sequential execution ) with the following implementation

var wg errorgroup.Group for _, v := range versions { specVersion := v.specVersion firstVersion := v.firstListVersion wg.Go(func() error { latestVersion := saveOne(ctx, client, urlMaker(specVersion, 0), saver) for i := firstVersion; i < latestVersion; i++ { currentVersion := i wg.Go(func() error { saveOne(ctx, client, urlMaker(specVersion, currentVersion), saver) return nil }) } return nil }) }

then you have deadlock because one go routine is waiting for another go routine while only 1 go routine is available

let me know if you have different idea

Oh I see. You have a good point. I therefore resolve this thread.

sebhtml · 2024-10-22T12:49:41Z

gdpr/vendorlist-fetching_test.go

+	saverMutex.Lock()
 	*s = append(*s, vi)
+	saverMutex.Unlock()


If *s = append(*s, vi) panics, I think that the saverMutex will never be unlocked.

I think that deferring can avoid that:

saverMutex.Lock() defer saverMutex.Unlock() *s = append(*s, vi)

sebhtml · 2024-10-22T12:53:55Z

sample/001_banner/app.yaml

+  vendorlist_fetcher:
+    max_concurrency_init_fetch_latest_version: 2
+    max_concurrency_init_fetch_specific_version: 20
+  timeouts_ms:
+    init_vendorlist_fetches: 30000
+    active_vendorlist_fetch: 30000


Are those new configuration keys optional ?

if init_vendorlist_fetches is 0 - then you literally set 0 sec as the timeout of preloading the vendor list, what you end up seeing the preload will have context deadline exceeded/timeout

active_vendorlist_fetch is more for fetching the vendor list on the run - same idea as above

Hi @zhongshixi

I am not asking what are those new configuration keys for.

I am asking whether or not the Prebid Server software works if you don't provide them.

@sebhtml
for vendor list max concurrent request

v.SetDefault("gdpr.vendorlist_fetcher.max_concurrency_init_fetch_latest_version", 1) // by default one fetch at a time v.SetDefault("gdpr.vendorlist_fetcher.max_concurrency_init_fetch_specific_version", 1) // by default one fetch at a time

we have default settings, which is literally the same as before ( sequential download )

for init_vendorlist_fetches and active_vendorlist_fetch, here is the default, if you do not set them to a reason value, you wont be able to preload the vendor list due to context deadline issue. The server can still spin up and run perfectly, but you do not have a prefetched vendor list

v.SetDefault("gdpr.timeouts_ms.init_vendorlist_fetches", 0) v.SetDefault("gdpr.timeouts_ms.active_vendorlist_fetch", 0)

let me know if i answered your question

So it's not backward-compatible.
I think that it should be backward-compatible.

@sebhtml i am not following

even without my change the issue context deadline issue still happen, because you can not really set 0 timeout , the default setting is using 0 as timeout, i talked to many people who host prebid server, they all configure the following value to more than 0 so we do not immediately timeout when the fetchiing request is fired

v.SetDefault("gdpr.timeouts_ms.init_vendorlist_fetches", 0) v.SetDefault("gdpr.timeouts_ms.active_vendorlist_fetch", 0)

the default logic before my PR change uses sequential execution, the default setting added in this PR also leads to sequential execution

v.SetDefault("gdpr.vendorlist_fetcher.max_concurrency_init_fetch_latest_version", 1) // by default one fetch at a time v.SetDefault("gdpr.vendorlist_fetcher.max_concurrency_init_fetch_specific_version", 1) // by default one fetch at a time

in summary, i do not think this PR incurs any compatibility issue. So i would like to ask if could you point out exactly which part of the code logic is not backwards compatible, maybe i am actually missing something

I don't know how to rephrase it. So I let it go.

zhongshixi · 2024-10-28T15:43:23Z

gdpr/vendorlist-fetching.go

+			for i := firstVersion; i < latestVersion; i++ {
+				currentVersion := i
+				wgSpecificVersion.Go(func() error {
+					saveOne(ctx, client, urlMaker(specVersion, currentVersion), saver)


to help reviewer understand the reason behind reason that i do not use channel or wait group+mutex is because the implementation of saveOne is already mutex safe due to its using sync.Map underneath, there is no need to add unnecessary new lock mechanism

https://pkg.go.dev/sync#Map

use concurrency to download vendor list; add config to control the co…

0c48c70

…ncurrency behaviour; add sample configuratio in the sample

zhongshixi commented Oct 22, 2024

View reviewed changes

zhongshixi added 2 commits October 21, 2024 20:54

remove extra space

e0b1be2

fix test;add more comments

0a5b937

sebhtml reviewed Oct 22, 2024

View reviewed changes

fix the code according to the review

7623704

sebhtml reviewed Oct 22, 2024

View reviewed changes

use defer to ulock the mutex if the locked code block has panic

e2baac5

bsardo self-assigned this Oct 22, 2024

zhongshixi commented Oct 28, 2024

View reviewed changes

		v.SetDefault("gdpr.vendorlist_fetcher.max_concurrency_init_fetch_latest_version", 1) // by default one download at a time
		v.SetDefault("gdpr.vendorlist_fetcher.max_concurrency_init_fetch_latest_version", 1) // by default one download at a time

Use Concurrency To Download Vendor List #4005

Are you sure you want to change the base?

Use Concurrency To Download Vendor List #4005

Conversation

zhongshixi commented Oct 22, 2024 • edited Loading

zhongshixi Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongshixi Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongshixi Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

sebhtml Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongshixi Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongshixi Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongshixi Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongshixi Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongshixi Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

zhongshixi commented Oct 22, 2024 •

edited

Loading

zhongshixi Oct 22, 2024 •

edited

Loading

zhongshixi Oct 22, 2024 •

edited

Loading

zhongshixi Oct 22, 2024 •

edited

Loading

sebhtml Oct 22, 2024 •

edited

Loading

zhongshixi Oct 22, 2024 •

edited

Loading

zhongshixi Oct 22, 2024 •

edited

Loading

zhongshixi Oct 22, 2024 •

edited

Loading

zhongshixi Oct 22, 2024 •

edited

Loading

zhongshixi Oct 28, 2024 •

edited

Loading