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.

Sign up for GitHub

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

Jump to bottom

Add PID to PGO profile data filename #97110

Merged
merged 1 commit into from
May 18, 2022
Merged

Add PID to PGO profile data filename #97110

merged 1 commit into from
May 18, 2022

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented May 17, 2022 •
edited
Loading

After experimenting with PGO, it looks like the generated profile data files can be sometimes overwritten if there is a race condition, because multiple rustc processes are usually invoked in parallel by cargo. Adding the PID to the resulting profile filename pattern makes sure that the profiles will be stored in separate files.

This generates ~20 GiB more space on disk on the CI run, but that seems harmless (?). Merging the profiles is not a bottleneck, the perf. run took the same amount of time as usually (~1h 24m).

r? @lqd

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 17, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2022
@Kobzol
Copy link
Contributor Author

Kobzol commented May 17, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 17, 2022
@bors
Copy link
Contributor

bors commented May 17, 2022

⌛ Trying commit cb61b975d3f0595f006725497d2544baeca02c52 with merge c9f0f31b6e23192d7c066d87758706b79ebde0b1...

@bors
Copy link
Contributor

bors commented May 17, 2022

☀️ Try build successful - checks-actions
Build commit: c9f0f31b6e23192d7c066d87758706b79ebde0b1 (c9f0f31b6e23192d7c066d87758706b79ebde0b1)

@rust-timer
Copy link
Collaborator

Queued c9f0f31b6e23192d7c066d87758706b79ebde0b1 with parent 7355d97, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c9f0f31b6e23192d7c066d87758706b79ebde0b1): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 0 79 54 79
mean2 N/A N/A -0.3% -0.4% -0.3%
max N/A N/A -0.5% -1.2% -0.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 17, 2022
@Kobzol Kobzol marked this pull request as ready for review May 17, 2022 17:13
@Kobzol
Copy link
Contributor Author

Kobzol commented May 17, 2022

@Mark-Simulacrum While playing with PGO, me and @lqd have noticed that the generated profile files are being re-written and merged all the time. When rustc is invoked in parallel, this could in theory cause data updates to be lost. This has (probably) happened on Windows for @lqd, so I tried what would happen if we fixed this on Linux, where it didn't cause problems so far.

By adding the PID placeholder (%p) to the generated profile path, we make (mostly) sure that no profiles will be overwritten or lost. It seems that this causes slight performance improvements (and mainly, no regressions), see the previous performance run.

It generates ~20 GiB more space on disk on the CI run, but that seems harmless (?). Merging the profiles is not a bottleneck, the perf. run took the same amount of time as usually (~1h 24m).

@Kobzol
Copy link
Contributor Author

Kobzol commented May 17, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 17, 2022
@bors
Copy link
Contributor

bors commented May 17, 2022

⌛ Trying commit 19a3558 with merge 524d12d1a77cff27739df8f27668cd58d821e35b...

@Mark-Simulacrum
Copy link
Member

Seems alright. If you can put that in the PR description that would be great, much easier to find in the future.

@lqd
Copy link
Member

lqd commented May 17, 2022 •
edited
Loading

After this perf run, let's also try it with LLVM profiling data, which will be in ./build/$PGO_HOST/llvm/build/profiles.

edit: in another PR will be fine.

@bors
Copy link
Contributor

bors commented May 17, 2022

☀️ Try build successful - checks-actions
Build commit: 524d12d1a77cff27739df8f27668cd58d821e35b (524d12d1a77cff27739df8f27668cd58d821e35b)

@rust-timer
Copy link
Collaborator

Queued 524d12d1a77cff27739df8f27668cd58d821e35b with parent 3655175, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (524d12d1a77cff27739df8f27668cd58d821e35b): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 0 75 49 75
mean2 N/A N/A -0.3% -0.4% -0.3%
max N/A N/A -0.5% -1.2% -0.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 17, 2022
@lqd
Copy link
Member

lqd commented May 17, 2022

I feel we've stumbled into some unexpected/interesting/strange unstability and non-determinism issues here. The perf runs seem to agree there's something to our discovery though, so:

@bors r+

@bors
Copy link
Contributor

bors commented May 17, 2022

📌 Commit 19a3558 has been approved by lqd

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 17, 2022
@Kobzol Kobzol mentioned this pull request May 18, 2022
Merged
@bors
Copy link
Contributor

bors commented May 18, 2022

⌛ Testing commit 19a3558 with merge e5732a2...

@bors
Copy link
Contributor

bors commented May 18, 2022

☀️ Test successful - checks-actions
Approved by: lqd
Pushing e5732a2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 18, 2022
@bors bors merged commit e5732a2 into rust-lang:master May 18, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 18, 2022
@Kobzol Kobzol deleted the pgo-pid-in-profile branch May 18, 2022 12:57
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e5732a2): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 0 94 54 94
mean2 N/A N/A -0.3% -0.4% -0.3%
max N/A N/A -0.5% -1.2% -0.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2022
@bors
Auto merge of rust-lang#97137 - Kobzol:ci-llvm-pgo-pid, r=Mark-Simula…
6364179 
…crum

Add PID to LLVM PGO profile path

This is a continuation of rust-lang#97110, which adds PID to the filename pattern of LLVM profiles. It also adds some metrics to the pgo.sh script, so that we can observe how many profiles there are and how large are they.

r? `@lqd`
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lqd lqd

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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Kobzol @rust-timer @bors @Mark-Simulacrum @lqd @rust-highfive @rustbot