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

Fix the max_mem_rss measurement #192

Merged
merged 6 commits into from
Jun 12, 2024
Merged

Fix the max_mem_rss measurement #192

merged 6 commits into from
Jun 12, 2024

Conversation

mdboom
Copy link
Collaborator

@mdboom mdboom commented Jun 12, 2024

In c960443, which fixed a bug in scaling the mem_max_rss value on Darwin, I inadvertently changed the measurement from using RUSAGE_SELF to RUSAGE_CHILDREN. This has resulted in the measurements being much coarser and incorrect on Darwin. This reverts to the old correct behavior.

if resource is not None:
usage = resource.getrusage(resource.RUSAGE_CHILDREN)
if resource_type is None:
resource_type = resource.RUSAGE_CHILDREN
Copy link
Member

Choose a reason for hiding this comment

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

We should not use RUSAGE_SELF on Linux, FreeBSD and other platforms?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function is called in two contexts -- one when the benchmark runs entirely in a separate subprocess, in which case we want CHILDREN, and one when in which it runs in the same process, where we want SELF. Saying that, it would probably clearer to not have a default value for resource_type here.

@@ -61,7 +61,7 @@ def bench_process(loops, args, kw, profile_filename=None):
args = [args[0], "-m", "cProfile", "-o", temp_profile_filename] + args[1:]

for _ in range_it:
start_rss = get_max_rss()
start_rss = get_max_rss(resource.RUSAGE_CHILDREN)
Copy link
Member

Choose a reason for hiding this comment

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

resource.RUSAGE_CHILDREN doesn't work on Windows: resource module is not available there.

You should add an abstraction, like having a children=True parameter for get_max_rss().

pyperf/_process_time.py Outdated Show resolved Hide resolved
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Victor Stinner <[email protected]>
@mdboom mdboom merged commit 7b9a23a into psf:main Jun 12, 2024
13 checks passed
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