Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
preinlein
changed the title
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
3088726 to
9696ba7
Compare
preinlein
force-pushed
the
paul.reinlein/fix-get-available-memory
branch
from
808f116 to
3d782ef
Compare
| version: 0.31.2 | ||
|
|
||
| target: | ||
| ddprof_replicas: 0 |
There was a problem hiding this comment.
Note: i was running into some strange profiling errors so I turned this off temporarily.
Easy enough to re-add and re-test.
There was a problem hiding this comment.
I'd be interested in seeing those errors if you have a link handy.
There was a problem hiding this comment.
Ugh, too old. It's one of the many many runs I did last week and have totally lost track of it.
It's going to be easy enough to reproduce though.
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
9696ba7 to
530bc15
Compare
preinlein
changed the title
|
Container build errors are fixed in #1827 |
|
Latest run with 100 replicas: Regression Detector ResultsRun ID: 5b1183a5-5725-4e58-b3fc-1826d36be97c Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | idle | memory utilization | +0.08 | [+0.06, +0.11] | 1 | |
| ➖ | http_ascii_1000mib | memory utilization | -0.00 | [-0.06, +0.05] | 1 | |
| ➖ | http_ascii_100mib | memory utilization | -0.01 | [-0.05, +0.04] | 1 | |
| ➖ | http_ascii_1mib | memory utilization | -0.05 | [-0.11, +0.01] | 1 | |
| ➖ | http_ascii_10mib | memory utilization | -0.06 | [-0.10, -0.02] | 1 |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | http_ascii_1000mib | cpu_usage | 100/100 | 317.35 ≤ 400 | |
| ✅ | http_ascii_1000mib | memory_usage | 100/100 | 1.02GiB ≤ 1.10GiB | |
| ✅ | http_ascii_100mib | cpu_usage | 100/100 | 88.73 ≤ 155 | |
| ✅ | http_ascii_100mib | memory_usage | 100/100 | 522.85MiB ≤ 575MiB | |
| ✅ | http_ascii_10mib | cpu_usage | 100/100 | 9.58 ≤ 14 | |
| ✅ | http_ascii_10mib | memory_usage | 100/100 | 120.94MiB ≤ 132MiB | |
| ✅ | http_ascii_1mib | cpu_usage | 100/100 | 1.33 ≤ 2.40 | |
| ✅ | http_ascii_1mib | memory_usage | 100/100 | 30.48MiB ≤ 34MiB | |
| ✅ | idle | cpu_usage | 100/100 | 0.35 ≤ 0.60 | |
| ✅ | idle | memory_usage | 100/100 | 12.46MiB ≤ 14MiB |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 530bc15bfa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| parallel_connections: 1 | ||
| method: | ||
| post: | ||
| maximum_prebuild_cache_size_bytes: "5 GiB" |
There was a problem hiding this comment.
u32 limits
Setting maximum_prebuild_cache_size_bytes to "5 GiB" here silently overflows when the HTTP generator builds its cache: Http::new converts this byte value with as u32 (lading/src/generator/http.rs), so values above u32::MAX wrap instead of erroring. In this case, the intended 5 GiB cache becomes 1 GiB at runtime, which materially changes memory behavior and makes the 1000 MiB/s regression gate measure a different workload than configured.
Useful? React with 👍 / 👎.
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
530bc15 to
20ad18b
Compare
preinlein
force-pushed
the
paul.reinlein/fix-get-available-memory
branch
from
3d782ef to
cc851e7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20ad18b2ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| headers: {} | ||
| target_uri: "http://127.0.0.1:8080/" | ||
| bytes_per_second: "1000 MiB" | ||
| parallel_connections: 1 |
There was a problem hiding this comment.
With the default maximum_block_size of 1 MiB (lading_payload::block::default_maximum_block_size), this config has to complete about 1000 HTTP request/response cycles per second to really deliver 1000 MiB/s. Http::spin only allows one in-flight request per configured connection, so parallel_connections: 1 makes the highest-throughput case depend on a single loopback connection keeping up. When that does not hold on a given runner, lading simply falls behind the target rate and experiment.yaml still passes because it only bounds CPU/PSS, not achieved bytes, so the "1000 MiB/s" regression gate ends up measuring an unknown lower load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
imo, this feels like a bug that isn't being caught at runtime.
I'd prefer for lading to fail in some way rather than silently "fail".
| target_metrics: | ||
| - prometheus: | ||
| uri: "http://127.0.0.1:9000/metrics" |
There was a problem hiding this comment.
The idle gate in cases/idle/experiment.yaml only checks total_pss_bytes and avg(total_cpu_usage_millicores), both of which come from the observer, but this extra target_metrics entry makes the controller issue a Prometheus scrape against the target every sample period (target_metrics::prometheus::Prometheus::run). In the idle case that periodic /metrics traffic becomes one of the main CPU sources, so the baseline is no longer measuring a quiescent lading process; it is measuring the exporter-plus-scraper path as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Personally, I want this as is. But maybe it's worth while to update the README to reflect this? Don't feel too strongly, I'll wait and see what reviewer says.
preinlein
force-pushed
the
paul.reinlein/fix-get-available-memory
branch
from
cc851e7 to
a479460
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
20ad18b to
b8a9099
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8a9099af9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| bytes_per_second: "100 MiB" | ||
| parallel_connections: 1 | ||
| method: | ||
| post: | ||
| maximum_prebuild_cache_size_bytes: "500 MiB" |
There was a problem hiding this comment.
maximum_block_size to the paired ASCII benchmark
These HTTP ASCII cases are described as being paired with lading_payload/benches/ascii.rs, but leaving maximum_block_size unset makes the HTTP generator fall back to 1 MiB blocks (lading/src/generator/http.rs, lading_payload::block::default_maximum_block_size). The criterion bench's 10/100/1000 MiB entries serialize single buffers of those exact sizes, whereas this config only exercises repeated <=1 MiB serializations to reach 100 MiB/s. That means regressions in large-buffer ASCII generation will not be caught by the SMP gate even though the README says the two should stay in sync.
Useful? React with 👍 / 👎.
| target_metrics: | ||
| - prometheus: | ||
| uri: "http://127.0.0.1:9000/metrics" |
There was a problem hiding this comment.
At this load the target is only generating about one 1 MiB block per second because HTTP defaults maximum_block_size to 1 MiB (lading_payload/src/block.rs), but target_metrics.prometheus makes the observer issue another HTTP request against /metrics every sample period (1 Hz by default in lading/src/config.rs, via target_metrics::prometheus::Prometheus::run). In practice that means the exporter/scraper path becomes comparable to the actual workload, so this gate no longer isolates the 1 MiB/s generator+blackhole path and can miss or misattribute low-throughput regressions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8a9099af9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| method: | ||
| post: | ||
| maximum_prebuild_cache_size_bytes: "500 MiB" |
There was a problem hiding this comment.
maximum_block_size to the benchmarked payload size
For the 10/100/1000 MiB/s cases, ascii_throughput in lading_payload/benches/ascii.rs benchmarks serializing one buffer of that exact size, but these configs never set maximum_block_size, so lading::generator::http::Config falls back to lading_payload::block::default_maximum_block_size() = 1 MiB. In practice these SMP cases send a stream of 1 MiB requests instead of exercising 10/100/1000 MiB ASCII serialization, so regressions that only show up on large contiguous buffers will be missed even though the README/comment says the experiments are paired with that benchmark.
Useful? React with 👍 / 👎.
| - name: cpu_usage | ||
| description: "CPU usage quality gate. Bounds total average millicore usage." | ||
| bounds: | ||
| series: avg(total_cpu_usage_millicores) | ||
| upper_bound: 2.4 |
There was a problem hiding this comment.
This is the last check in the file, so the 1/10/100 MiB/s experiments introduced here only bound PSS and CPU. Because the controller is already scraping the target's Prometheus telemetry via target_metrics, a generator regression that under-drives the advertised rate would make these cases cheaper and still pass green, meaning the regression detector can stop exercising the intended load without any signal. (The 1000 MiB/s case already has a separate comment; the same missing lower-bound exists for the smaller throughput cases too.)
Useful? React with 👍 / 👎.
preinlein
force-pushed
the
paul.reinlein/fix-get-available-memory
branch
from
a479460 to
db73f9e
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
b8a9099 to
89f0f93
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89f0f939a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| series: total_pss_bytes | ||
| upper_bound: "14 MiB" |
There was a problem hiding this comment.
Checked the new cases/idle and cases/http_ascii_* configs: the only metric source wired into case/lading/lading.yaml is target_metrics.prometheus, but the target command launches lading with --no-target. In inner_main, --no-target makes config.target = None, so the observer is skipped entirely (lading/src/bin/lading.rs:603-615), and the total_pss_bytes / total_cpu_usage_millicores series used by these new quality gates are only emitted by the observer (lading/src/observer/linux/procfs.rs:161-164, lading/src/observer/linux/cgroup/v2/cpu.rs:115-123). That means these experiments never export the memory/CPU series they are checking here, so the new gates either fail on missing data or silently stop enforcing the intended limits.
Useful? React with 👍 / 👎.
| # This is acceptable as we are less concerned about the shutdown performance | ||
| command: "/usr/bin/lading --no-target --experiment-duration-infinite --config-path /etc/lading-target/lading.yaml" | ||
| cpu_allotment: 2 | ||
| memory_allotment: 1250 MiB |
There was a problem hiding this comment.
I'm surprised this doesn't OOM given the 5GiB prebuild cache - am I missing something?
There was a problem hiding this comment.
It's the issue that codex spotted above.
lading is limiting the cache size to 1G
imo, this feels like a bug in lading that I'd like to address
preinlein
changed the base branch from
paul.reinlein/fix-get-available-memory
to
graphite-base/1826
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
89f0f93 to
2f5b785
Compare
preinlein
force-pushed
the
graphite-base/1826
branch
from
db73f9e to
2619756
Compare
preinlein
changed the base branch from
graphite-base/1826
to
paul.reinlein/fix-get-available-memory
preinlein
force-pushed
the
paul.reinlein/fix-get-available-memory
branch
from
2619756 to
b573bc9
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
2f5b785 to
be1c3d4
Compare
💡 Codex Review Align the 10 MiB SMP case with the paired ASCII benchmark
This case is described as paired with ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
preinlein
changed the base branch from
paul.reinlein/fix-get-available-memory
to
graphite-base/1826
Merge activity
|
preinlein
force-pushed
the
graphite-base/1826
branch
from
b573bc9 to
f24286f
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
be1c3d4 to
3656530
Compare
preinlein
changed the base branch from
graphite-base/1826
to
paul.reinlein/fix-get-available-memory
preinlein
changed the base branch from
paul.reinlein/fix-get-available-memory
to
graphite-base/1826
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
3656530 to
5cbf63d
Compare
preinlein
force-pushed
the
paul.reinlein/introduce-smp-tests
branch
from
5cbf63d to
a7d2219
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7d2219459
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| //! Paired with SMP regression experiments in `experiments/regression/cases/http_ascii_*`. | ||
| //! If throughput sizes here change, update the corresponding SMP experiments and vice versa. |
There was a problem hiding this comment.
ascii_throughput
If these new http_ascii_* SMP cases are meant to catch regressions in lading_payload/benches/ascii.rs, they do not actually exercise the same hot path. I checked lading/src/generator.rs:5-10, lading/src/generator/http.rs:155-181,224-233, and lading/src/bin/lading.rs:546-549,624-633: the HTTP generator prebuilds its entire block cache during startup, before warmup begins, and spin() only rotates cached Bytes afterwards. So a slowdown in ascii::Ascii::to_bytes can still leave these experiments green as long as startup time is unchanged, which makes the new pairing comment misleading and leaves the advertised regression coverage gap in place.
Useful? React with 👍 / 👎.
2 participants
What does this PR do?
Introduces SMP (Single Machine Performance) regression detector experiments for lading. Adds an idle baseline test and four HTTP ASCII throughput tests (1, 10, 100, 1000 MiB/s) with memory and CPU quality gates.
Motivation
Establishes continuous performance regression detection for lading using the SMP regression detector. The idle test provides a resource floor, while the ASCII tests cover the HTTP generator + blackhole path across a range of throughput levels. Each experiment enforces bounds on
total_pss_bytesandavg(total_cpu_usage_millicores).Related issues
N/A
Additional Notes
lading_payload/benches/ascii.rs; a cross-reference comment was added to the benchmark file.--no-target --experiment-duration-infinitemode (shutdown performance is out of scope).experiments/regression/config.yamlpins lading version to 0.31.2 with profiling replicas disabled.