Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a garbage collection threshold #2081

Merged
merged 7 commits into from Nov 15, 2021
Merged

Add a garbage collection threshold #2081

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e660e2f
First draft
Nov 11, 2021
e0f26f2
Scott's and Veronika's review
Nov 12, 2021
3d51756
Added defer call and additional environment variable test case
Nov 12, 2021
1a9f50d
Added env variable test case comment
Nov 12, 2021
83509d3
Comment correction
Nov 12, 2021
8400de6
config.go comment correction
Nov 12, 2021
f81934e
Removed TestGarbageCollectorThresholdFromEnv
Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions config/config.go
View file
Open in desktop
Expand Up @@ -24,6 +24,9 @@ type Configuration struct {
CacheClient HTTPClient `mapstructure:"http_client_cache"`
AdminPort int `mapstructure:"admin_port"`
EnableGzip bool `mapstructure:"enable_gzip"`
// GarbageCollectorLimit will serve as a fixed cost of memory that is going to be held garbage
// before a garbage collection cycle is triggered
GarbageCollectorLimit int `mapstructure:"garbage_collector_limit"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can improve the wording for clarity, and a link to the Go issue might be a good idea. Perhaps something like:

// GarbageCollectorLimit allocates virtual memory (in bytes) which is not used by PBS but serves as a hack to trigger the garbage collector only when the heap reaches at least this size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Far better worded. Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For naming, I like that GarbageCollector conveys the right intent, but I'm not sure "Limit" is a precise enough descriptor. Maybe "GarbageCollectorMinimum", "GarbageCollectorAllocationHack", "GarbageCollectorThreshold" etc...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GarbageCollectorMemoryBallast maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GarbageCollectorThreshold makes the most sense to me (I even named this PR that). @VeronikaSolovei9, @SyntaxNode let me know your veredict.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like GarbageCollectorThreshold.

// StatusResponse is the string which will be returned by the /status endpoint when things are OK.
// If empty, it will return a 204 with no content.
StatusResponse string `mapstructure:"status_response"`
Expand Down Expand Up @@ -634,6 +637,7 @@ func SetupViper(v *viper.Viper, filename string) {
v.SetDefault("port", 8000)
v.SetDefault("admin_port", 6060)
v.SetDefault("enable_gzip", false)
v.SetDefault("garbage_collector_limit", 0)
v.SetDefault("status_response", "")
v.SetDefault("auction_timeouts_ms.default", 0)
v.SetDefault("auction_timeouts_ms.max", 0)
Expand Down
2 changes: 2 additions & 0 deletions config/config_test.go
View file
Open in desktop
Expand Up @@ -289,6 +289,7 @@ external_url: http://prebid-server.prebid.org/
host: prebid-server.prebid.org
port: 1234
admin_port: 5678
garbage_collector_limit: 32768
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a simpler test value, like 1. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

auction_timeouts_ms:
max: 123
default: 50
Expand Down Expand Up @@ -426,6 +427,7 @@ func TestFullConfig(t *testing.T) {
cmpStrings(t, "host", cfg.Host, "prebid-server.prebid.org")
cmpInts(t, "port", cfg.Port, 1234)
cmpInts(t, "admin_port", cfg.AdminPort, 5678)
cmpInts(t, "garbage_collector_limit", cfg.GarbageCollectorLimit, 32768)
cmpInts(t, "auction_timeouts_ms.default", int(cfg.AuctionTimeouts.Default), 50)
cmpInts(t, "auction_timeouts_ms.max", int(cfg.AuctionTimeouts.Max), 123)
cmpStrings(t, "cache.scheme", cfg.CacheURL.Scheme, "http")
Expand Down
9 changes: 9 additions & 0 deletions main.go
View file
Open in desktop
Expand Up @@ -20,6 +20,8 @@ func init() {
rand.Seed(time.Now().UnixNano())
}

var garbageCollectionLimit []byte

func main() {
flag.Parse() // required for glog flags and testing package flags

Expand All @@ -28,6 +30,13 @@ func main() {
glog.Exitf("Configuration could not be loaded or did not pass validation: %v", err)
}

// Create a soft memory limit on the total amount of memory that PBS uses to tune the behavior
// of the Go garbage collector. In summary, `cfg.GarbageCollectorLimit` serves as a fixed cost
// of memory that is going to be held garbage before a garbage collection cycle is triggered.
// This amount of virtual memory won’t translate into physical memory allocation unless we attempt
// to read or write to the slice below, which PBS will not do.
garbageCollectionLimit = make([]byte, cfg.GarbageCollectorLimit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to test this works as intended. We saw the following recommendation from a Medium article:

func main() {
	ballast := make([]byte, 2<<30)
	defer runtime.KeepAlive(ballast)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going with the defer runtime.KeepAlive(garbageCollectionLimit) recommendation then.


err = serve(cfg)
if err != nil {
glog.Exitf("prebid-server failed: %v", err)
Expand Down