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

Port pgo.sh to Python #103019

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Port pgo.sh to Python #103019

merged 1 commit into from
Jan 30, 2023

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Oct 13, 2022

This PR ports the pgo.sh multi stage build file from bash to Python, to make it easier to add new functionality and gather statistics. Main changes:

  1. pgo.sh rewritten from Bash to Python. Jump from ~200 Bash LOC to ~650 Python LOC. Bash is, unsurprisingly, more concise for running scripts and binaries.
  2. Better logging. Each separate stage is now clearly separated in logs, and the logs can be quickly grepped to find out which stage has completed or failed, and how long it took.
  3. Better statistics. At the end of the run, there is now a table that shows the duration of the individual stages, along with a percentual ratio of the total workflow run:
2023-01-15T18:13:49.9896916Z stage-build INFO: Timer results
2023-01-15T18:13:49.9902185Z ---------------------------------------------------------
2023-01-15T18:13:49.9902605Z Build rustc (LLVM PGO):                 1815.67s (21.47%)
2023-01-15T18:13:49.9902949Z Gather profiles (LLVM PGO):              418.73s ( 4.95%)
2023-01-15T18:13:49.9903269Z Build rustc (rustc PGO):                 584.46s ( 6.91%)
2023-01-15T18:13:49.9903835Z Gather profiles (rustc PGO):             806.32s ( 9.53%)
2023-01-15T18:13:49.9904154Z Build rustc (LLVM BOLT):                1662.92s (19.66%)
2023-01-15T18:13:49.9904464Z Gather profiles (LLVM BOLT):             715.18s ( 8.46%)
2023-01-15T18:13:49.9914463Z Final build:                            2454.00s (29.02%)
2023-01-15T18:13:49.9914798Z Total duration:                         8457.27s
2023-01-15T18:13:49.9915305Z ---------------------------------------------------------

A sample run can be seen here.

I tried to keep the code compatible with Python 3.6 and don't use dependencies, which required me to reimplement some small pieces of functionality (like formatting bytes). I suppose that it shouldn't be so hard to upgrade to a newer Python or install dependencies in the CI container, but I'd like to avoid it if it won't be needed.

The code is in a single file stage-build.py, so it's a bit cluttered. I can also separate it into multiple files, although having it in a single file has some benefits. The code could definitely be nicer, but I'm a bit wary of introducing a lot of abstraction and similar stuff, as long as the code is stuffed into a single file.

Currently, the Python pipeline should faithfully mirror the bash pipeline one by one. After this PR, I'd like to try to optimize it, e.g. by caching the LLVM builds on S3.

r? @Mark-Simulacrum

@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Oct 13, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2022
@Noratrieb
Copy link
Member

Would it make sense to port it to Rust instead?

@Kobzol
Copy link
Contributor Author

Kobzol commented Oct 13, 2022

You're not the first one to ask 😅 It would definitely be possible, but I'm not sure if it's a good tradeoff here. It would definitely complicate the build process, because we would have to build the script and/or somehow integrate it into bootstrap.

This script mostly calls external programs (well, x.py most of the time) and we experiment with it quite often, so Python seemed like a good choice for it, in order to not be too verbose, but also to enable complex computation (like timing statistics or S3 caching of LLVM). Already the Python script has more lines than bash, and it doesn't even contain half of it, so I'm a bit worried that writing it in Rust would make it unnecessarily verbose and bloated, and I'm not sure if we would gain that much.

@Mark-Simulacrum @jyn514 What would you prefer here? Rust or Python?

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Oct 13, 2022

This script mostly calls external programs (well, x.py most of the time) and we experiment with it quite often

Python seems fine to me; we run this in environments we control so there's not as much motivation to have as few dependencies as possible.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum Mark-Simulacrum 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 Oct 16, 2022
@bors
Copy link
Contributor

bors commented Oct 23, 2022

☔ The latest upstream changes (presumably #101403) made this pull request unmergeable. Please resolve the merge conflicts.

@Kobzol Kobzol force-pushed the ci-multistage-python branch from 8c38f5e to e87274f Compare January 14, 2023 18:15
@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jan 14, 2023
@Kobzol Kobzol force-pushed the ci-multistage-python branch from e87274f to 400af2b Compare January 14, 2023 18:17
@Kobzol Kobzol marked this pull request as ready for review January 14, 2023 18:22
@Kobzol Kobzol force-pushed the ci-multistage-python branch 2 times, most recently from 77a5836 to 34a419e Compare January 14, 2023 18:29
@Kobzol Kobzol marked this pull request as draft January 14, 2023 18:29
@Kobzol Kobzol force-pushed the ci-multistage-python branch 2 times, most recently from b574e2e to 6c01196 Compare January 14, 2023 21:43
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 15, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

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

bors commented Jan 15, 2023

⌛ Trying commit a719397c5d3d637807edb6c953cd9acaf963b747 with merge 2ab228588f97afe565aed325316268b90f0ca680...

@Kobzol Kobzol changed the title WIP: port pgo.sh to Python Port pgo.sh to Python Jan 15, 2023
@Kobzol Kobzol marked this pull request as ready for review January 15, 2023 21:58
@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 15, 2023

@rustbot ready

@bors
Copy link
Contributor

bors commented Jan 24, 2023

💔 Test failed - checks-actions

@Kobzol Kobzol force-pushed the ci-multistage-python branch from efc40e0 to 5b77cb4 Compare January 24, 2023 20:51
@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 24, 2023

@bors try @rust-timer queue

@bors
Copy link
Contributor

bors commented Jan 24, 2023

⌛ Trying commit 5b77cb4 with merge 383be05d5680f29b331dca7795e09a8a2a1f6f80...

@rust-timer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 24, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (383be05d5680f29b331dca7795e09a8a2a1f6f80): comparison URL.

Overall result: no relevant changes - no action needed

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-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
6.1% [6.1%, 6.1%] 1
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.9% [-0.9%, -0.9%] 2
All ❌✅ (primary) 6.1% [6.1%, 6.1%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jan 25, 2023
@rust-log-analyzer

This comment has been minimized.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 25, 2023

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jan 25, 2023

@Kobzol: 🔑 Insufficient privileges: Not in reviewers

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 25, 2023

@rustbot ready

@rustbot rustbot 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 Jan 25, 2023
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 29, 2023

📌 Commit 5b77cb4 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@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 Jan 29, 2023
@bors
Copy link
Contributor

bors commented Jan 29, 2023

⌛ Testing commit 5b77cb4 with merge f55b002...

@bors
Copy link
Contributor

bors commented Jan 30, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing f55b002 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 30, 2023
@bors bors merged commit f55b002 into rust-lang:master Jan 30, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 30, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f55b002): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.3% [1.3%, 1.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.4% [-3.5%, -3.2%] 2
Improvements ✅
(secondary)
-4.3% [-4.3%, -4.3%] 1
All ❌✅ (primary) -3.4% [-3.5%, -3.2%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

@Kobzol Kobzol deleted the ci-multistage-python branch January 30, 2023 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants