Add a garbage collection threshold #2081
Conversation
config/config.go
Outdated
| @@ -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"` | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Far better worded. Done
config/config.go
Outdated
| @@ -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"` | |||
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
GarbageCollectorMemoryBallast maybe?
There was a problem hiding this comment.
GarbageCollectorThreshold makes the most sense to me (I even named this PR that). @VeronikaSolovei9, @SyntaxNode let me know your veredict.
There was a problem hiding this comment.
I like GarbageCollectorThreshold.
config/config_test.go
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Consider using a simpler test value, like 1. :)
main.go
Outdated
| // 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) |
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
Going with the defer runtime.KeepAlive(garbageCollectionLimit) recommendation then.
To clarify, this is entirely dependent on how the Host has configured their PBS instance. This does not affect everyone. |
config/config_test.go
Outdated
|
|
||
| // Assert config value differs from the default value of 0 | ||
| cmpInts(t, "garbage_collector_threshold", 1, cfg.GarbageCollectorThreshold) | ||
| } |
There was a problem hiding this comment.
I don't believe this test adds value.
Pull request #2049 removed the legacy
/auctionendpoint. An unintended consequence of removing the legacy code was an increase in CPU time caused by garbage collection overhead. After a thorough investigation of the performance of the system, the Prebid Server Team found out that the initialization of a very large slice in the old code was preventing the garbage collection to be called repeatedly, saving CPU time. This pull request puts back a soft memory limit to save CPU performance and makes this value configurable.Issue #48409 posted in the go development project better explains what happens behind the scenes, in summary, we are setting an amount of memory that is going to be held garbage before a garbage collection cycle gets triggered.