-
Notifications
You must be signed in to change notification settings - Fork 525
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
PGO: update the existing benchmarks workflow to enable PGO builds #13884
Conversation
This pull request does not have a backport label. Could you fix it @1pkg? 🙏
NOTE: |
c874a6e
to
f77b98c
Compare
576dbb8
to
b710c57
Compare
1727304
to
a6abd96
Compare
a6abd96
to
a320c3d
Compare
8622b95
to
cd3c7e1
Compare
77b0ec8
to
6bbd47d
Compare
0ed821e
to
e60e927
Compare
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.
Nice work - thank you for all the cleanups along the way!
.github/workflows/benchmarks.yml
Outdated
- name: Open PGO PR | ||
if: ${{ env.RUN_STANDALONE == 'true' && github.ref == 'refs/heads/main' }} | ||
run: make push-pgo-pr | ||
env: | ||
WORKSPACE_PATH: ${{ github.workspace }} | ||
PROFILE_PATH: ${{ env.WORKING_DIRECTORY }}/${{ env.BENCHMARK_CPU_OUT }} | ||
GITHUB_TOKEN: ${{ steps.get_token.outputs.token }} | ||
WORKFLOW: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}/attempts/${{ github.run_attempt }} |
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 wonder if instead of creating a new PR on every benchmark run, could we just push a commit to the branch?
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 think it's slightly risky to enable auto pushes to main branch right away, I'd prefer to start with more controlled PR based approach so we develop the confidence that this pipeline works well. Afterwards we can simplify and enable the direct merge.
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 also added a small update to the push-pgo-pr script so it enables auto merging too for PRs. This way we will only need to give it 1 approval and the pipeline tests need to pass.
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.
Fair enough.
c3f4efd
to
0d68b85
Compare
0d68b85
to
aae7c2f
Compare
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.
LGTM, thank you!
Add a benchmark workflow mode with automation to collect, preserve, and inject CPU profiles, enabling PGO builds. The new workflow will run on a schedule and raise a special pull request that includes the most recent representative CPU profile, which will be inserted as the `default.pgo` file into the main package and automatically used in the build pipeline. The actual schedule and the model for raising pull requests with updated profiles are subject to further revisions. This new workflow mode uses a lightweight output destination - a mock proxy (Moxy) from apm-perf to better isolate the performance component of the APM Server. (cherry picked from commit 5af8cf4)
…) (#14245) Add a benchmark workflow mode with automation to collect, preserve, and inject CPU profiles, enabling PGO builds. The new workflow will run on a schedule and raise a special pull request that includes the most recent representative CPU profile, which will be inserted as the `default.pgo` file into the main package and automatically used in the build pipeline. The actual schedule and the model for raising pull requests with updated profiles are subject to further revisions. This new workflow mode uses a lightweight output destination - a mock proxy (Moxy) from apm-perf to better isolate the performance component of the APM Server. (cherry picked from commit 5af8cf4) Co-authored-by: Kostiantyn Masliuk <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Motivation/summary
This PR implements changes outlined in #13859. It updates the existing benchmarks workflow to run standalone APM Server instance that produces a relevant CPU profile for PGO, then it copies, uploads and injects the obtained CPU profile into a PR, see example.
Benchmarks
The existing benchmarks results turned to be too unreliable to base PGO on. Because of the underlying dependency on ElasticSearch the difference in the throughput results could go above 10% from a workflow to workflow. The table below provides a view with the existing benchmarks results sample.
This all renders incremental PGO performance gains hard to observe and measure. Therefore, in this PR a new benchmark mode is introduced, which swaps ElasticSearch with a stubbed API http server (Moxy). Thus allowing us to better isolate and elevate APM Server performance component inside the benchmarks. The table below provides a view with the new isolated benchmarks results sample.
Using the benchmarks result sample data we can clearly observe that the results deviation for the new benchmark mode is in an order of magnitude lower in comparison to the existing ES based benchmarks. And now PGO performance improvements could be reliably observed.
The standalone APM Server benchmarks mode consists of running 3 separate EC2 instances in a VPC for apmbench, apm-server and moxy. Existing
benchmark_executor
andstandalone_apm_server
terraform modules are reused and a similar new terraform modulemoxy
is created.Results
PGO enabled builds show 5% performance gain on average across the standalone APM Server benchmarks workflow.
Checklist
For functional changes, consider:
How to test these changes
To observe and validate the changes please refer to the indexed PGO benchmarks results.
Related issues
#13859