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

Attempt to gather similar stats as rusage on Windows #82754

Merged
merged 3 commits into from
Mar 19, 2021

Conversation

rylev
Copy link
Member

@rylev rylev commented Mar 4, 2021

A follow up to #82532. This is a bit hacked in because I think we need to discuss this before merging, but this is an attempt to gather similar metrics as libc::rusage on Windows.

Some comments on differences:

  • Currently, we're passing RUSAGE_CHILDREN to rusage which collects statistics on all children that have been waited on and terminated. I believe this is currently just the invocation of the real rustc that the shim is wrapping. Does rustc itself spawn children processes? The windows version gets the child processes handle when spawning it, and uses that to collect the statistics. For maxrss, rusage will return "the resident set size of the largest child, not the maximum resident set size of the process tree.", the Windows version will only collect statistics on the wrapped rustc child process directly even if some theoretical sub process has a larger memory footprint.
  • There might be subtle differences between rusage's "resident set" and Window's "working set". The "working set" and "resident set" should both be the number of pages that are in memory and which would not cause a page fault when accessed.
  • I'm not yet sure how best to get the same information that ru_minflt, ru_inblock, ru_oublock, ru_nivcsw and ru_nvcsw provide.

r? @pnkfelix

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2021
@rylev rylev added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Mar 4, 2021
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

We spawn - at minimum - linkers as child processes, I think.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 4, 2021

I'm not yet sure how best to get the same information that ru_minflt, ru_inblock, ru_oublock, ru_nivcsw and ru_nvcsw provide.

To be honest, I only included those because they were "immediately available" and were not explicitly documented as being unused. I did not include them because I thought they were crucial statistics to gather.

So I recommend you follow a similar strategy for Windows: If there are statistics that are immediately available, go ahead and print them with their own tag. This particular piece of machinery need not attempt to be 100% cross-platform, in my opinion.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 4, 2021

For maxrss, rusage will return "the resident set size of the largest child, not the maximum resident set size of the process tree."

Hmm. I realize now that I might have been misreading this sentence that you have quoted.

I had been interpreting it has "the resident set size of the largest descendant" (child, grandchild, et cetera), and I had been interpreting the qualification about the process tree as just pointing out that its not going to tell you about the maximum resident size that results from summing the simultaneously live children at all points in time and taking the maximum of that.

But I now realize that its possible that that text is merely saying that it only tells you about the immediate children; that the data is not gathered recursively. (On the other hand: Why not gather the data recursively? A child process needs to gather it and deliver it to its parent in any case, right?)

Anyway, that was just an idle thought further showing my lack of knowledge of this particular API.

@rylev
Copy link
Member Author

rylev commented Mar 5, 2021

But I now realize that its possible that that text is merely saying that it only tells you about the immediate children

I've not seen any information that clarifies this question unfortunately...

So I recommend you follow a similar strategy for Windows

I've pushed changes that now show more Windows specific numbers that are "immediately available".

src/bootstrap/bin/rustc.rs Outdated Show resolved Hide resolved
@pnkfelix
Copy link
Member

pnkfelix commented Mar 5, 2021

The implementation of format_rusage_data looks good, (as far as I know regarding the Windows API).

r=me once rylev changes the call site according to my suggestion above. (Or provides an argument for why that would not be good.)

@pnkfelix pnkfelix added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2021
@rylev rylev added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2021
@rylev
Copy link
Member Author

rylev commented Mar 11, 2021

@pnkfelix I cleaned this up, and it's ready for your sign off now.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 18, 2021

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Mar 18, 2021

📌 Commit 302867c has been approved by pnkfelix

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 18, 2021
Attempt to gather similar stats as rusage on Windows

A follow up to rust-lang#82532. This is a bit hacked in because I think we need to discuss this before merging, but this is an attempt to gather similar metrics as `libc::rusage` on Windows.

Some comments on differences:
* Currently, we're passing `RUSAGE_CHILDREN` to `rusage` which collects statistics on all children that have been waited on and terminated. I believe this is currently just the invocation of the real `rustc` that the shim is wrapping. Does `rustc` itself spawn children processes? The windows version gets the child processes handle when spawning it, and uses that to collect the statistics. For maxrss, `rusage` will return "the resident set size of the largest child, not the maximum resident set size of the process tree.", the Windows version will only collect statistics on the wrapped `rustc` child process directly even if some theoretical sub process has a larger memory footprint.
* There might be subtle differences between `rusage`'s "resident set" and Window's "working set". The "working set" and "resident set" should both be the number of pages that are in memory and which would not cause a page fault when accessed.
* I'm not yet sure how best to get the same information that `ru_minflt`, `ru_inblock`, `ru_oublock`, `ru_nivcsw ` and `ru_nvcsw` provide.

r? `@pnkfelix`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 18, 2021
Attempt to gather similar stats as rusage on Windows

A follow up to rust-lang#82532. This is a bit hacked in because I think we need to discuss this before merging, but this is an attempt to gather similar metrics as `libc::rusage` on Windows.

Some comments on differences:
* Currently, we're passing `RUSAGE_CHILDREN` to `rusage` which collects statistics on all children that have been waited on and terminated. I believe this is currently just the invocation of the real `rustc` that the shim is wrapping. Does `rustc` itself spawn children processes? The windows version gets the child processes handle when spawning it, and uses that to collect the statistics. For maxrss, `rusage` will return "the resident set size of the largest child, not the maximum resident set size of the process tree.", the Windows version will only collect statistics on the wrapped `rustc` child process directly even if some theoretical sub process has a larger memory footprint.
* There might be subtle differences between `rusage`'s "resident set" and Window's "working set". The "working set" and "resident set" should both be the number of pages that are in memory and which would not cause a page fault when accessed.
* I'm not yet sure how best to get the same information that `ru_minflt`, `ru_inblock`, `ru_oublock`, `ru_nivcsw ` and `ru_nvcsw` provide.

r? ``@pnkfelix``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 19, 2021
Attempt to gather similar stats as rusage on Windows

A follow up to rust-lang#82532. This is a bit hacked in because I think we need to discuss this before merging, but this is an attempt to gather similar metrics as `libc::rusage` on Windows.

Some comments on differences:
* Currently, we're passing `RUSAGE_CHILDREN` to `rusage` which collects statistics on all children that have been waited on and terminated. I believe this is currently just the invocation of the real `rustc` that the shim is wrapping. Does `rustc` itself spawn children processes? The windows version gets the child processes handle when spawning it, and uses that to collect the statistics. For maxrss, `rusage` will return "the resident set size of the largest child, not the maximum resident set size of the process tree.", the Windows version will only collect statistics on the wrapped `rustc` child process directly even if some theoretical sub process has a larger memory footprint.
* There might be subtle differences between `rusage`'s "resident set" and Window's "working set". The "working set" and "resident set" should both be the number of pages that are in memory and which would not cause a page fault when accessed.
* I'm not yet sure how best to get the same information that `ru_minflt`, `ru_inblock`, `ru_oublock`, `ru_nivcsw ` and `ru_nvcsw` provide.

r? ```@pnkfelix```
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 19, 2021
Attempt to gather similar stats as rusage on Windows

A follow up to rust-lang#82532. This is a bit hacked in because I think we need to discuss this before merging, but this is an attempt to gather similar metrics as `libc::rusage` on Windows.

Some comments on differences:
* Currently, we're passing `RUSAGE_CHILDREN` to `rusage` which collects statistics on all children that have been waited on and terminated. I believe this is currently just the invocation of the real `rustc` that the shim is wrapping. Does `rustc` itself spawn children processes? The windows version gets the child processes handle when spawning it, and uses that to collect the statistics. For maxrss, `rusage` will return "the resident set size of the largest child, not the maximum resident set size of the process tree.", the Windows version will only collect statistics on the wrapped `rustc` child process directly even if some theoretical sub process has a larger memory footprint.
* There might be subtle differences between `rusage`'s "resident set" and Window's "working set". The "working set" and "resident set" should both be the number of pages that are in memory and which would not cause a page fault when accessed.
* I'm not yet sure how best to get the same information that `ru_minflt`, `ru_inblock`, `ru_oublock`, `ru_nivcsw ` and `ru_nvcsw` provide.

r? ````@pnkfelix````
@bors
Copy link
Contributor

bors commented Mar 19, 2021

⌛ Testing commit 302867c with merge b97fd3e...

@bors
Copy link
Contributor

bors commented Mar 19, 2021

☀️ Test successful - checks-actions
Approved by: pnkfelix
Pushing b97fd3e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 19, 2021
@bors bors merged commit b97fd3e into rust-lang:master Mar 19, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants