RSS Shrinker Attempt 2 #32507
RSS Shrinker Attempt 2 #32507
Conversation
scottopell
force-pushed
the
sopell/rss
branch
from
18d81f1 to
2cbd847
Compare
github-actions
bot
added
[deprecated] team/agent-shared-components
Uncompressed package size comparisonComparison with ancestor Diff per package
Decision |
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv aws.create-vm --pipeline-id=51778631 --os-family=ubuntuNote: This applies to commit 2cbd847 |
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 26280de Optimization Goals: ✅ Improvement(s) detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +4.25 | [+1.40, +7.11] | 1 | Logs bounds checks dashboard |
| ➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | +2.89 | [+2.00, +3.79] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency_linear_load | egress throughput | +0.20 | [-0.02, +0.42] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | +0.14 | [-0.52, +0.79] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | +0.07 | [-0.54, +0.69] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http1 | egress throughput | +0.06 | [-0.55, +0.68] | 1 | Logs |
| ➖ | file_to_blackhole_300ms_latency | egress throughput | +0.05 | [-0.58, +0.68] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | +0.05 | [-0.55, +0.66] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.01 | [-0.01, +0.03] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.01 | [-0.27, +0.25] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.02 | [-0.62, +0.59] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http2 | egress throughput | -0.06 | [-0.66, +0.55] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -0.79 | [-0.85, -0.72] | 1 | Logs |
| ➖ | docker_containers_cpu | % cpu utilization | -1.28 | [-4.45, +1.88] | 1 | Logs |
| ✅ | otlp_ingest_metrics | memory utilization | -6.68 | [-6.83, -6.52] | 1 | Logs |
| ✅ | otlp_ingest_logs | memory utilization | -7.94 | [-8.06, -7.81] | 1 | Logs |
| ✅ | quality_gate_idle_all_features | memory utilization | -8.41 | [-8.56, -8.27] | 1 | Logs bounds checks dashboard |
| ✅ | docker_containers_memory | memory utilization | -9.34 | [-9.41, -9.27] | 1 | Logs |
| ✅ | file_tree | memory utilization | -10.03 | [-10.24, -9.82] | 1 | Logs |
| ✅ | ddot_metrics | memory utilization | -10.57 | [-10.69, -10.46] | 1 | Logs |
| ✅ | ddot_logs | memory utilization | -12.02 | [-12.16, -11.89] | 1 | Logs |
| ✅ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | -12.22 | [-12.27, -12.16] | 1 | Logs |
| ✅ | quality_gate_idle | memory utilization | -12.71 | [-12.77, -12.65] | 1 | Logs bounds checks dashboard |
Bounds Checks: ❌ Failed
| perf | experiment | bounds_check_name | replicates_passed | links |
|---|---|---|---|---|
| ❌ | docker_containers_memory | memory_usage | 0/10 | |
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency_linear_load | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | |
| ✅ | quality_gate_idle | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | lost_bytes | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
cmd/system-probe/main.go
Outdated
| "github.com/DataDog/datadog-agent/cmd/internal/runcmd" | ||
| "github.com/DataDog/datadog-agent/cmd/system-probe/command" | ||
| "github.com/DataDog/datadog-agent/cmd/system-probe/subcommands" | ||
| ) | ||
|
|
||
| func main() { | ||
| rssshrinker.Setup() |
There was a problem hiding this comment.
Do we agree that this is only to make the RSS metric look better than to actually reduce the actual memory used or the number of OOM kills ?
There was a problem hiding this comment.
I think everyone agrees with that (although maybe this should be clearer on the PR description and in code comments), we're just releasing what's not used to the OS, but of course (and unfortunately !) it doesn't actually reduce used memory 😄
| //revive:disable:var-naming | ||
|
|
||
| // MADV_PAGEOUT Reclaim these pages. | ||
| const MADV_PAGEOUT = 21 | ||
|
|
||
| //revive:enable:var-naming |
There was a problem hiding this comment.
🥜 nitpick: I believe this should work similarly while being simpler and less error prone
| //revive:disable:var-naming | |
| // MADV_PAGEOUT Reclaim these pages. | |
| const MADV_PAGEOUT = 21 | |
| //revive:enable:var-naming | |
| // MADV_PAGEOUT Reclaim these pages. | |
| // | |
| //nolint:revive | |
| const MADV_PAGEOUT = 21 |
| // Release the memory garbage collected by the Go runtime to Linux | ||
| debug.FreeOSMemory() |
There was a problem hiding this comment.
💬 suggestion: This is not Linux specific so we could have a version of rssshrinker just calling it for Windows/Darwin
There was a problem hiding this comment.
🔨 warning: this should be controlled by a config so that we can disable it when needed. It means we can't just do it at the beginning but have to wait for the config component though...
There was a problem hiding this comment.
I think by default ASC will own this package, I'm wondering if ASC and SMP should co-own it, what do you think ?
|
@scottopell Were you able to measure the impact on a long run? My guess - and the results I got when experimenting with this is - that there was no win after a few minutes. The reason is probably that Linux is smart and will free the pages it doesn't need and will only keep the needed one resident. |
|
@scottopell if you are still working on this PR, can you move it to draft? We get pinged for reviews daily. |
|
Marked as draft, sorry for the noise @brycekahle |
What does this PR do?
At the beginning of each sub-agent's execution, force all reclaimable memory to be returned to the OS.
Motivation
This builds on @L3n41c 's more tongue-in-cheek version ( #32312 )
The underlying idea here is simple, we explicitly ask Linux to reclaim as much memory as we can once. The process will inevitably page back in some amount of the memory, but this is a trade-off that is worth it to page out memory that was used at agent-init and never again.
Describe how you validated your changes
None yet, needs more validation
Possible Drawbacks / Trade-offs
Additional Notes
Ideally I could swap out the current
pageOutFileBackedMemorywith a version that utilizes some kernel functionality, however I can't figure out how to trigger memory reclamation for a single process, I found a patch proposing it, but it didn't seem to get any attention.echo N > /proc/[PID]/reclaim(see https://lwn.net/Articles/544319/)