Repository navigation
Memory regression tests - #275
alan-george-lk wants to merge 20 commits into
Conversation
2a21a8a to
d638cca
Compare
…ature-memory-regression-tests
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
🔍 Devin Review: 2 flags
Not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
stephen-derosa
left a comment
There was a problem hiding this comment.
overall looks great, thanks for adding these!
| matrix.e2e-testing && | ||
| (inputs.integration_repeat > 0 || | ||
| inputs.run_stress_tests || | ||
| (matrix.name == 'ubuntu-24.04-x64' && inputs.build_type == 'release')) |
There was a problem hiding this comment.
we are only running these on ubuntu x64 release builds?
There was a problem hiding this comment.
Fair callout, but some specific points:
- The original customer report was on Linux/x64
- The tools I built this with are linux-specific (the way RSS is tracked, and the
MALLOC_ARENAstuff), given the original customer report - Running on multiple platforms will clog up the GitHub summary page/take more CI time for likely negligible coverage differences
So I felt having a single run on the most representative platform for robotics/physical AI (resource constrainted linux) would be most applicable
| delegate.requireDisconnect(livekit::DisconnectReason::ClientInitiated); | ||
| } | ||
|
|
||
| // Exercises data-track publication, payload enqueueing, explicit unpublication, |
There was a problem hiding this comment.
these doc strings are great
…ature-memory-regression-tests
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 1 new potential issue.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| capture-clock | ||
| capture-pattern |
There was a problem hiding this comment.
🟡 All-scenario runs fail on standard builds
After a default release-tests build, --scenario all runs capture scenarios that the SDK cannot execute. Both fail, making the all-scenario run fail even when every standard scenario passes.
Learn more
The build defaults to capture disabled, but still produces the Linux memory tester. The script now appends both capture scenarios to all regardless of that build configuration. In a standard build, runCycle rejects each capture scenario, and the script returns a failure even if every other scenario passes.
Example: Run ./build.sh release-tests followed by source scripts/set-test-tokens.sh and scripts/memory-tester.sh --scenario all. The six standard scenarios can pass, but both capture scenarios fail with a capture-enabled-build error.
Recommended fix: Make all select capture scenarios only when the built SDK supports their respective sources, or provide a standard-only group and retain the existing behavior of all for non-capture builds. Keep explicit capture scenario requests failing clearly when unsupported.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "${CMAKE_CURRENT_SOURCE_DIR}/memory/memory_tester.cpp" | ||
| ) | ||
| target_link_libraries(livekit_memory_tester PRIVATE livekit) | ||
| target_include_directories(livekit_memory_tester PRIVATE |
There was a problem hiding this comment.
hmu why is this no longer needed?
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
5 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if (( BASH_REMATCH[11] > advisory_max_thread_growth )); then | ||
| advisory_exceeded=true |
There was a problem hiding this comment.
🟡 Leading-zero thread limits hide advisories
With --advisory-max-thread-growth 08, a nine-thread increase passes the blocking gate but produces no advisory. Bash rejects 08 as octal inside the conditional, then continues to report PASS.
Learn more
The advisory thread limit accepts decimal digits, including strings with leading zeros. Bash arithmetic treats a leading zero as an octal literal, so 08 or 09 raises an arithmetic error in the conditional. Since the expression is in an if, the script continues without setting advisory_exceeded; a passing test can appear as PASS despite exceeding its advisory limit.
Example: Run with --max-thread-growth 12 --advisory-max-thread-growth 08. A final growth of nine threads passes the binary gate, while the shell prints an arithmetic error and skips the warning.
Recommended fix: Normalize advisory_max_thread_growth to decimal before using it in Bash arithmetic, or explicitly convert its digits as base 10. Cover leading-zero limits such as 08 in a shell test.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if awk -v growth="${BASH_REMATCH[6]}" -v limit="${advisory_max_rss_growth_mib}" \ | ||
| 'BEGIN { exit !(growth > limit) }'; then |
There was a problem hiding this comment.
🟡 Rounded RSS growth hides threshold breaches
When RSS grows 8 MiB plus 4 KiB, awk compares the displayed 8.00 MiB against the advisory limit of 8. No warning appears, although measured growth exceeds the limit.
Learn more
The tester computes its RSS deltas in KiB, but main formats the report with two decimal places. The wrapper parses that rounded report for the advisory gate. A delta slightly above the configured limit can round down to the limit, leaving the scenario marked PASS instead of ADVISORY.
Example: The warmup-to-final delta is 8196 KiB, or 8.00390625 MiB. The report prints 8.00 MiB; against an 8 MiB advisory limit the wrapper emits no warning.
Recommended fix: Make the tester emit an unrounded machine-readable delta for the wrapper, or pass both limits into the tester and compare against its KiB samples before formatting.
Was this helpful? React with 👍 or 👎 to provide feedback.
This PR introduces a new test binary called
memory_testerfor Linux, which has the following features:CI is also updated to run this tester and provide a nice markdown table report of the results, and fail jobs that introduce changes that fail this.
Documentation is also updated around testing in general, as it was a bit out of date.
Results
Here are two FFI release results with the following params:
Prior release
livekit-ffi/v0.12.81results:Current FFI release results: