Skip to content
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

Periodically report memory usage #1503

Merged
merged 3 commits into from
Dec 21, 2024

Conversation

andyleiserson
Copy link
Collaborator

@andyleiserson andyleiserson commented Dec 17, 2024

The reporting is wired into seq_join. The reporting interval increases as the record count increases, which results in a tolerable amount of log messages for loops with many iterations, while still providing some reporting for shorter loops. It's still fairly chatty, so I put it at debug level. Here's a sample -- 1141 lines reporting memory usage vs. 583 other log lines -- but I reduced the frequency after generating this, so it's ~half of that now.

Note that the reporting depends on jemalloc. That is the default on linux. It can be activated on other platforms with a feature.

@akoshelev
Copy link
Collaborator

I originally disabled jemalloc for MacOS builds because using it had to value but building that crate takes a lot of time on M1 Pro. Now, with live profiling enabled, we could reconsider that as it is valuable to observe memory usage on dev machines

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.24%. Comparing base (6531c97) to head (007630e).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1503      +/-   ##
==========================================
- Coverage   93.24%   93.24%   -0.01%     
==========================================
  Files         238      241       +3     
  Lines       43688    43877     +189     
==========================================
+ Hits        40739    40912     +173     
- Misses       2949     2965      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andyleiserson andyleiserson marked this pull request as draft December 17, 2024 03:07
@andyleiserson
Copy link
Collaborator Author

There is more work needed before this could be merged. Right now jemalloc is only used in helper and oneshot_ipa, but telemetry::memory doesn't know whether it's active. The behavior when it's not is printing lots of 0 MiB allocated.

Copy link
Collaborator

@akoshelev akoshelev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that there is no perf impact, so should be good to go

@andyleiserson andyleiserson marked this pull request as ready for review December 19, 2024 22:57
@andyleiserson
Copy link
Collaborator Author

I originally disabled jemalloc for MacOS builds because using it had to value but building that crate takes a lot of time on M1 Pro. Now, with live profiling enabled, we could reconsider that as it is valuable to observe memory usage on dev machines

As we discussed, I left the jemalloc defaults unchanged since the build time concern remains outstanding. jemalloc is active by default on Linux, can be activated on MacOS with the jemalloc feature.

I also reduced the reporting frequency, so there are now fewer memory usage log messages than when I originally wrote the description.

@andyleiserson andyleiserson merged commit 3413c3c into private-attribution:main Dec 21, 2024
12 checks passed
@andyleiserson andyleiserson deleted the memprof2a branch December 21, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants