-
Notifications
You must be signed in to change notification settings - Fork 726
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.
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
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GarbageCollectorMemoryBallast maybe?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.
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)
}
There was a problem hiding this comment.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this test adds value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. You may wish to remove the environment variable test, but not a deal breaker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Pull request #2049 removed the legacy
/auction
endpoint. 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.