-
Notifications
You must be signed in to change notification settings - Fork 31
Add short section on how to measure performance counters of Apptainer instances using perf #382
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
Conversation
Signed-off-by: Petr Konstantin Milev <petrkmilev@gmail.com>
DrDaveD
left a comment
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.
Thanks for this, I think this is helpful.
I'm confused by what you mean by {path}. Please explain that in the text.
In addition, I see a couple of small wording problems. One is those to facts which should perhaps be that fact. The other is you are using gathering which should probably drop the using.
Finally, instances aren't always run in a cgroup, so you could add when possible to Instances are run within a cgroup.
|
Thank you for your feedback. I added a small explaination and example for {path} in the paragraph. I'll post it here if you want to edit it more so that I don't clog up the commit history and once we agree on good wording, I'll post the more finalized commit. For in-depth container perfomance analysis it would be useful to collect data from Linux's performance counters. Normally this is done by supplying the target PID to .. code:: perf -a -e cache-misses --cgroup "{path}/apptainer-{instance id number}" -- sleep 10 .. note:: Thank you for taking your time to review my change :) |
|
What I usually do for PR modifications is to do force pushes after comments and I find that works pretty well. Alternatively we can do a squash merge when a PR is merged to collapse to 1 commit. Having said that, here's my comments on the draft you posted in a comment. We use a substitution mechanism in the docs for some things that is done by putting a keyword surrounded by brackets. My first comment related to that is we have a convention of using Second, please change Finally, I'm a bit uncomfortable with using that same convention also within the text. One potential issue is that we might someday decide to use the same keyword for a substitution. Also, I looked over the rest of the documentation and I didn't see anything similar. My suggestion is to instead use shell variables for example |
…conflict Signed-off-by: Petr Konstantin Milev <petrkmilev@gmail.com>
|
I should have fixed the phrasing and the variables as per your feedback. If you have any more feedback feel free to tell me and I'll fix it :) |
DrDaveD
left a comment
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.
Getting close, a few more comments.
Please break up the lines into no more than 80 characters since that's the style of the rest of the file. That also makes for easier reviews.
Drop those from use those that fact.
In Usually the {command} instance change the to for. Just after that drop one in from in in.
Signed-off-by: Petr Konstantin Milev <petrkmilev@gmail.com>
|
ok done :) happy to keep editing if you have more feedback |
Signed-off-by: Petr Konstantin Milev <petrkmilev@gmail.com>
|
ok that should be the comments you have left for now. I'll be afk for the rest of the evening so if you have more feedback, I'll get to you tomorrow :) |
DrDaveD
left a comment
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.
Thanks!
Description of the Pull Request (PR):
A short subsection to the "Resource Usage / Limits" section, that describes how to gather more detailed system metrics using cgroups and perf Linux program.
Reason for addition
After struggling to figure out how to get performance counters from Apptainer containers, I am hoping to share my findings. I believe that being able to get software and hardware counter information from Apptainer containers is crucial to deeply understanding and optimizing the running programs.
Testing
I checked that the docs I wrote produced correct ouput using:
make html SKIPCLI=1P.S. if you have any changes you want me to make for clarity and such I am open to feedback. Additionally if you do not see the utility of this section in the docs then I understand your decision to close this PR.