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

[8.0] Fix memory reporting #7518

Merged
merged 2 commits into from
Mar 18, 2024
Merged

Conversation

fstagni
Copy link
Contributor

@fstagni fstagni commented Mar 18, 2024

After #7509

BEGINRELEASENOTES

*WMS
FIX: Fix memory reporting

ENDRELEASENOTES

@DIRACGridBot DIRACGridBot added the alsoTargeting:integration Cherry pick this PR to integration after merge label Mar 18, 2024
@@ -847,9 +827,9 @@ def __getUsageSummary(self):
if "MemoryUsed" in self.parameters:
memory = self.parameters["MemoryUsed"]
if memory:
summary["MemoryUsed(kb)"] = abs(float(memory[-1]) - float(self.initialValues["MemoryUsed"]))
summary["MemoryUsed(mb)"] = abs(float(memory[-1]) - float(self.initialValues["MemoryUsed"]))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that computing the MemoryUsed as the difference between the last measured value of RSS and the initial value is fine in most of the cases. However it seems to me that it does not take into account eventual peaks of memory usage during the program execution. So I'm wondering if it wouldn't make sense to add another parameter "PeakMemoryUsed" or "MaxMemoryUsed".
Anyway this could go in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can propose this in a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok fine for me. I will not do it immediately but I keep it in mind.

@arrabito
Copy link
Contributor

To be consistent I suggest that we use MB also for the Memory parameter.

@fstagni fstagni force-pushed the fixMemoryReporting branch 2 times, most recently from d9753c2 to ce4ee93 Compare March 18, 2024 13:10
@fstagni fstagni force-pushed the fixMemoryReporting branch from ce4ee93 to 7d0f76d Compare March 18, 2024 13:50
@fstagni fstagni force-pushed the fixMemoryReporting branch from 7d0f76d to 27f2f18 Compare March 18, 2024 14:11
@arrabito
Copy link
Contributor

I've just added an "approve" review but I see thate Merging is still blocked.
Is it normal or some other actions is needed from my side?

@fstagni fstagni merged commit 108861c into DIRACGrid:rel-v8r0 Mar 18, 2024
25 checks passed
@DIRACGridBot DIRACGridBot added sweep:done All sweeping actions have been done for this PR sweep:failed Sweeping failed and needs manual intervention labels Mar 18, 2024
@DIRACGridBot
Copy link

Sweep summary

Sweep ran in https://github.com/DIRACGrid/DIRAC/actions/runs/8329879707

Failed:

  • integration
    cherry-pick 108861c into integration failed
    check merge conflicts on a local copy of this repository
    git fetch upstream
    git checkout upstream/integration -b cherry-pick-2-108861c3f-integration
    git cherry-pick -x -m 1 108861c3f
    # Fix the conflicts
    git cherry-pick --continue
    git commit --amend -m 'sweep: #7518 Fix memory reporting' --author='Federico Stagni <[email protected]>'
    git push -u origin cherry-pick-2-108861c3f-integration
    
    # If you have the GitHub CLI installed the PR can be made with
    gh pr create \
         --label 'sweep:from rel-v8r0' \
         --base integration \
         --repo DIRACGrid/DIRAC \
         --title '[sweep:integration] Fix memory reporting' \
         --body 'Sweep #7518 `Fix memory reporting` to `integration`.
    
    Adding original author @fstagni as watcher.
    
    BEGINRELEASENOTES
    
    *WMS
    FIX: Fix memory reporting
    
    ENDRELEASENOTES
    Closes #7520'

fstagni added a commit to fstagni/DIRAC that referenced this pull request Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alsoTargeting:integration Cherry pick this PR to integration after merge sweep:done All sweeping actions have been done for this PR sweep:failed Sweeping failed and needs manual intervention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants