Repository navigation
Bug 2091167 - SizeLimit plugin parses all of /proc/self/smaps in pure Perl after every request (~3 ms per request) - #2822
Conversation
… Perl after every request (~3 ms per request)
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the SizeLimit memory-check optimization.
No open issues 🎉
What changed in this PR
This PR reduces per-request memory-check overhead in the SizeLimit plugin by reading VmSize from /proc/self/status instead of parsing /proc/self/smaps.
Changes:
- Replaces smaps parsing with a lightweight virtual-memory-size reader while preserving the worker shutdown threshold.
- Removes the
Linux::Smaps::Tinydependency. - Adds tests for memory-size reading, threshold behavior, and unavailable measurements.
| File | Description |
|---|---|
t/app-size-limit.t |
Tests memory readings and worker shutdown behavior. |
Makefile.PL |
Removes Linux::Smaps::Tiny from optional prerequisites, retaining BSD::Resource. |
cpanfile |
Removes the Linux::Smaps::Tiny dependency. |
Bugzilla/App/Plugin/SizeLimit.pm |
Reads VmSize from proc status for per-request memory checks. |
Review effort: Balanced
| @@ -284,11 +284,8 @@ my %optional_features = ( | |||
| prereqs => {runtime => {requires => {'Linux::Pid' => 0},},}, | |||
| }, | |||
| linux_smaps => { | |||
There was a problem hiding this comment.
Now that this feature only pulls in BSD::Resource, should we rename linux_smaps to something like rlimit? Or is the name referenced by --with-feature somewhere outside this repo, so it's safer to keep it?
There was a problem hiding this comment.
Renamed to rlimit in 7ccc262. Nothing in this repo referenced linux_smaps; checksetup.pl passes features through --with-feature, so the new name is picked up automatically.
| # reads a few dozen lines instead of parsing all of /proc/self/smaps (tens of | ||
| # thousands of lines in a BMO worker) after every request. Linux::Smaps::Tiny | ||
| # 0.10 also silently fell back to its pure-Perl parser: its XS loader runs in a | ||
| # BEGIN block before $VERSION is set, so the version check always failed. |
There was a problem hiding this comment.
Since Linux::Smaps::Tiny is removed entirely in this patch, could the history about its XS loader bug live in the commit message/bug instead? Something like "Virtual memory size in kB from VmSize, or undef if unavailable" seems like enough here.
There was a problem hiding this comment.
Trimmed to the one-line comment in 7ccc262. The Linux::Smaps::Tiny XS-loader history stays in the bug and the original commit message.
Addresses review feedback: the feature now only pulls in BSD::Resource, and the Linux::Smaps::Tiny history belongs in the bug rather than the code.
Xzzz
left a comment
Xzzz
left a comment
There was a problem hiding this comment.
lgtm, thanks! One optional doc comment, as you want ;)
Addresses review feedback: the limit is compared to VmSize, not unshared memory.
No description provided.