bloomberg / memray

Memray is a memory profiler for Python
https://bloomberg.github.io/memray/
Apache License 2.0
13.36k stars 397 forks source link

Added new commands to memray attach #458

Closed ivonastojanovic closed 1 year ago

ivonastojanovic commented 1 year ago

Letting memray attach supports deactivating an attached tracker manually, or after a specified heap size is reached, or after a specified time limit has elapsed.

Issue number of the reported bug or feature request: #

Describe your changes A clear and concise description of the changes you have made.

Testing performed Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.

Additional context Add any other context about your contribution here.

codecov-commenter commented 1 year ago

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (baf7f41) 91.96% compared to head (56d5b3a) 92.15%. Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #458 +/- ## ========================================== + Coverage 91.96% 92.15% +0.19% ========================================== Files 91 91 Lines 10790 10849 +59 Branches 1485 1498 +13 ========================================== + Hits 9923 9998 +75 + Misses 865 849 -16 Partials 2 2 ``` | [Flag](https://app.codecov.io/gh/bloomberg/memray/pull/458/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | Coverage Δ | | |---|---|---| | [cpp](https://app.codecov.io/gh/bloomberg/memray/pull/458/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `85.68% <ø> (+0.43%)` | :arrow_up: | | [python_and_cython](https://app.codecov.io/gh/bloomberg/memray/pull/458/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `95.44% <90.10%> (+0.03%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/bloomberg/memray/pull/458?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | Coverage Δ | | |---|---|---| | [tests/unit/test\_attach.py](https://app.codecov.io/gh/bloomberg/memray/pull/458?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#diff-dGVzdHMvdW5pdC90ZXN0X2F0dGFjaC5weQ==) | `100.00% <100.00%> (ø)` | | | [tests/integration/test\_attach.py](https://app.codecov.io/gh/bloomberg/memray/pull/458?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#diff-dGVzdHMvaW50ZWdyYXRpb24vdGVzdF9hdHRhY2gucHk=) | `96.47% <94.28%> (+5.24%)` | :arrow_up: | | [src/memray/commands/attach.py](https://app.codecov.io/gh/bloomberg/memray/pull/458?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#diff-c3JjL21lbXJheS9jb21tYW5kcy9hdHRhY2gucHk=) | `64.32% <87.27%> (+5.15%)` | :arrow_up: | ... and [3 files with indirect coverage changes](https://app.codecov.io/gh/bloomberg/memray/pull/458/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

godlygeek commented 1 year ago

I've pushed quite a few fixup commits here to make various improvements. Most of them are pretty small things. The biggest one is refactoring the thread for the heap size check so that it can be cancelled, like the duration check one can. I also moved the global state to be attributes of the memray module, as we discussed above.

@ivonastojanovic Please review my changes and let me know if you spot any mistakes, bugs, issues, or shortcomings. Each of the fixup commits has a short explanation of why it was made, but I typed those up quickly, so also let me know if you don't understand the purpose of one of the changes.

godlygeek commented 1 year ago

OK, I've updated this PR to make memray detach a separate command, and to fix the small issue with your fixup that caused the CI to fail. Take a look when you can, @pablogsal

godlygeek commented 1 year ago

Oh, and https://godlygeek.github.io/memray/attach.html#cli-reference shows how the docs changes render.

godlygeek commented 1 year ago

After some offline discussions, @pablogsal and I have decided to drop --heap-limit from this PR. The name is misleading, since the implementation is looking at maximum RSS rather than at the size of the heap, and it refuses to work reliably on i686 for reasons we haven't managed to track down (operations that we expect to be raising ru_maxrss appear not to, though only on i686 and not on x86-64 or arm64).

Instead, our plan is to add a way to set these limits when Tracker is created and installed, and have it enforce the limits itself. That change will come in a future release, though. For now, we're going to remove the option from this PR to unblock the way for including the rest of this work in a release.