-
Notifications
You must be signed in to change notification settings - Fork 110
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
Correctly calculate cpu_seconds for processtree #9943
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #9943 will not alter performanceComparing Summary
|
If a subprocess in the processtree terminates early, its cpu_seconds value will be lost unless we keep all values pr. pid until the end. Since pids can be reused (although with little probability) we need to detect this through a decreasing cpu_second value for a pid that has already been observed in the processtree. The test has been modified in order to trigger this scenario.
4852ce2
to
14c9939
Compare
(memory_rss, cpu_seconds_snapshot, oom_score, pids) = _get_processtree_data( | ||
process | ||
) | ||
for pid, seconds in cpu_seconds_snapshot.items(): |
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.
You could extract this part to make it easier to test:
@dataclass
class CpuTimer:
_cpu_seconds_pr_pid: dict[str, float] = field(default_factory=dict, init=False)
def update(self, cpu_seconds_snapshot):
...
def total_cpu_seconds(self):
return sum(self._cpu_seconds_pr_pid.values())
Then you can easily test the logic by giving it fake cpu_seconds_snapshots
.
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.
I had a small suggestion about improving the testability of the logic, but I see no need to block merging. The suggested refactoring and adding of testing can be done in a separate PR.
If a subprocess in the processtree terminates early, its cpu_seconds value will be lost unless we keep all values pr. pid until the end.
Since pids can be reused (although with little probability) we need to detect this through a decreasing cpu_second value for a pid that has already been observed in the processtree.
The test has been modified in order to trigger this scenario.
Issue
Resolves #9941
Approach
🗒️ Do the book-keeping necessary.
git rebase -i main --exec 'just rapid-tests'
)When applicable