perf: use the faster api to collect rss#742
Conversation
There was a problem hiding this comment.
Pull request overview
Updates default RSS collection on non-Linux platforms to use Node’s faster RSS-only API, reducing overhead during metrics collection.
Changes:
- Switch non-Linux
process_resident_memory_bytescollection fromprocess.memoryUsage()toprocess.memoryUsage.rss(). - Add a new
safeRss()helper to safely read RSS with a try/catch wrapper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/metrics/osMemoryHeap.js | Uses safeRss() to collect RSS for the non-Linux variant of process_resident_memory_bytes. |
| lib/metrics/helpers/safeRss.js | Introduces a helper that calls process.memoryUsage.rss() and swallows platform/runtime errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| try { | ||
| return process.memoryUsage.rss(); | ||
| } catch { | ||
| return; |
There was a problem hiding this comment.
I'd like to use debug for this in the future but for now we only use it for the benchmarks, so I'm not going to force the issue here, despite the silent return being kind of smelly.
|
@chidam333 Something is glitching with github actions and I'm unsure why I can't make this build. You might need to push a do-nothing change. |
|
hello @jdmarshall i added a dummy commit, is the issue resolved ? |
|
DCO check happened now? was that the issue ? |
|
Not entirely sure yet. There’s a handful that are in each state. I submitted one without DCO and I’m fairly sure the builds ran anyway. |
|
Can you update the change list? also, do we have a benchmark for this? Would be good to identify or write one for “perf” changes. |
I am seeing 30-40% improvement. (added changelog) chidam333#1 <- code i used for benchmarking. |
|
@jdmarshall i had to rebase for signing, but i can see that it shows a bunch of unrelated 
added patch file for confirmation |
|
That's... weird for a rebase. I'm going to say no specifically because of the workflow changes. I'd try resetting your branch to HEAD and cherry-picking your changes back on top, please. |
Signed-off-by: chidam333 <chidam.sync@gmail.com>
Signed-off-by: chidam333 <chidam.sync@gmail.com>
Signed-off-by: chidam333 <chidam.sync@gmail.com>
|
done, thank you |
|
thank you :) also faceoff looks great ❤️ |
https://nodejs.org/api/process.html#processmemoryusagerss
i think we can use the faster api to collect rss