-
Notifications
You must be signed in to change notification settings - Fork 279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use tracemalloc for memory measurements. #5948
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5948 +/- ##
=======================================
Coverage 89.78% 89.78%
=======================================
Files 93 93
Lines 23007 23007
Branches 5017 5017
=======================================
Hits 20657 20657
Misses 1620 1620
Partials 730 730 ☔ View full report in Codecov by Sentry. |
⏱️ Performance Benchmark Report: a809e9aPerformance shifts
Full benchmark results
Generated by GHA run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After running the benchmarks in #5946, I'm satisfied that tracemalloc gives a more accurate view of memory for more cases than the current set up. It seems like there are still some potential difficulties with memory benchmarking for some parallelisation setups, but I don't believe any more problems are being introduced. Otherwise, I think this PR looks good to go.
Thanks @stephenworsley ! |
Addresses #5947
Replaces the current process RSS ("resident set size") based mechanism.
See #5946 for info on how they compare.
N.B. this is hard to test, possibly only takes effect when merged ?
@trexfeathers can you advise? I'm a bit vague on this.