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.
| } | ||
| } | ||
|
|
||
| # Virtual memory size in kB from VmSize, or undef if unavailable (non-Linux). |
There was a problem hiding this comment.
BMO_size_limit in README.md and localconfig_size_limit in template/en/default/setup/strings.txt.pl still mention the limit as "unshared memory", while it is compared to VmSize (much larger).
Since this PR makes it explicit, maybe could they say "virtual memory size (VmSize, in kB)" instead?
There was a problem hiding this comment.
Updated both BMO_size_limit in README.md and localconfig_size_limit in strings.txt.pl to say "virtual memory size (VmSize, in kB)" in a80ae4b14.
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.