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

ci: self-hosted runners for benchmarks #29955

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HowardBraham
Copy link
Contributor

@HowardBraham HowardBraham commented Jan 29, 2025

Description

Major changes

  • benchmarks.yml - Does some important and new-to-us things like:
    • Self-hosted runner runs-on: gha-mm-scale-set-ubuntu-22.04-amd64-med
    • Running in the same kind of Docker container we use in CircleCI container: image: cimg/node:22.13-browsers
    • Unlike on CircleCI, does not work without installing Xvfb
  • codespaces.yml
    • I originally added this workflow, and it was important to caching in the past, but with the new workflows in GHA, it's no longer needed, so I deleted it

Prerequisites to merging this PR

This is just Part 1 of a larger 4-part task to make the startup time quality gate, but I think it's the hardest part

  1. Run on self-hosted GitHub Actions runners so that hopefully the numbers will be more stable (this PR)
  2. Get this to measure the same measurements in the same way as we do for Sentry
  3. Make metamaskbot post with the results of this instead of the current CircleCI source
  4. Set up the quality gate

Open in GitHub Codespaces

Related issues

Progresses: MetaMask/MetaMask-planning#3679

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot added the team-tiger Tiger team (for tech debt reduction + performance improvements) label Jan 29, 2025
@HowardBraham HowardBraham removed the team-tiger Tiger team (for tech debt reduction + performance improvements) label Jan 29, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [9c55124]
Page Load Metrics (1730 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22520381648351169
domContentLoaded15082026169912861
load15682041173013364
domInteractive2397422512
backgroundConnect1068342010
firstReactRender1597502813
getState45713147
initialActions00000
loadScripts10521569124612359
setupStore891192311
uiStartup177225952011234112
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

1 similar comment
@metamaskbot
Copy link
Collaborator

Builds ready [9c55124]
Page Load Metrics (1730 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22520381648351169
domContentLoaded15082026169912861
load15682041173013364
domInteractive2397422512
backgroundConnect1068342010
firstReactRender1597502813
getState45713147
initialActions00000
loadScripts10521569124612359
setupStore891192311
uiStartup177225952011234112
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@HowardBraham HowardBraham force-pushed the self-hosted-benchmarks branch from 9c55124 to ce07be7 Compare January 29, 2025 04:08
with:
name: ${{ inputs.name }}
path: test-artifacts/chrome/benchmark/
retention-days: 5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The number of retention-days was chosen very arbitrarily, and maybe someone has an opinion

@@ -182,4 +182,4 @@ jobs:
env:
GITHUB_TOKEN: ${{ secrets.LAVAMOAT_UPDATE_TOKEN }}
PR_NUMBER: ${{ github.event.issue.number }}
ACTION_RUN_URL: "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}"
ACTION_RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just Prettier running on a file

@metamaskbot
Copy link
Collaborator

Builds ready [e5a4333]
Page Load Metrics (2151 ± 76 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22725071986600288
domContentLoaded17602409210516579
load18422476215115876
domInteractive30150583216
backgroundConnect1594512813
firstReactRender1890472210
getState990352612
initialActions01000
loadScripts12891877157815273
setupStore106622189
uiStartup211033722513279134
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@HowardBraham HowardBraham force-pushed the self-hosted-benchmarks branch from e5a4333 to a6f5acb Compare January 29, 2025 05:03
@HowardBraham HowardBraham marked this pull request as ready for review January 29, 2025 05:13
@HowardBraham HowardBraham added the DO-NOT-MERGE Pull requests that should not be merged label Jan 29, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [a6f5acb]
Page Load Metrics (1708 ± 72 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15542173171514369
domContentLoaded15092097167613364
load15142175170815172
domInteractive259439199
backgroundConnect10226404723
firstReactRender1695472612
getState589202512
initialActions01000
loadScripts10751614122011656
setupStore76114136
uiStartup17222398197920297
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt
Copy link
Member

Gudahtt commented Jan 29, 2025

Could you split this into multiple PRs? A lot of these changes seem to be distinct from each other

@HowardBraham HowardBraham force-pushed the self-hosted-benchmarks branch from a6f5acb to c9f1e1e Compare January 29, 2025 17:59
@HowardBraham HowardBraham changed the title ci: self-hosted runners for benchmarks, with a new prep-deps workflow ci: self-hosted runners for benchmarks Jan 29, 2025
@HowardBraham HowardBraham force-pushed the self-hosted-benchmarks branch from c9f1e1e to 6266945 Compare January 29, 2025 21:50
@metamaskbot
Copy link
Collaborator

Builds ready [6266945]
Page Load Metrics (1958 ± 284 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint28932341555702337
domContentLoaded138636081945585281
load139436451958592284
domInteractive22106452311
backgroundConnect86819157
firstReactRender1584492311
getState45415147
initialActions01000
loadScripts100827631440483232
setupStore76124209
uiStartup153241692280685329
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@itsyoboieltr
Copy link
Contributor

The migration to GitHub Actions is a necessary step, but before committing to self-hosted runners, we should first validate the stability of measurements across different environments.

  1. We need to compare benchmark results between CircleCI, GitHub-hosted (ubuntu-latest), and self-hosted runners to determine if self-hosted runners indeed reduce fluctuations. I am not convinced that they actually would.
  2. We need to ensure that the same type of build is measured across all environments to make results comparable. This PR is testing the webpack build, but CircleCI is testing browserify.
  3. We should definitely avoid premature infrastructure changes (e.g., provisioning self-hosted runners) without confirming that they improve measurement stability. We need reproducible experiments, and most importantly, NUMBERS!

The suggested approach is to conduct benchmark runs on GitHub-hosted runners first (ubuntu-latest) and compare them against CircleCI and self-hosted runner results. If fluctuations remain high across all environments, this method may not be suitable for a quality gate. However, the migration to GitHub Actions remains beneficial regardless.

Next steps:

  • Run benchmarks on ubuntu-latest and self-hosted runners, compare results.
  • If stability issues persist, scrap the idea of using the results for a quality gate while still proceeding with the migration to GitHub Actions.

Would be great to see some comparison data in the PR before merging.

@HowardBraham
Copy link
Contributor Author

As requested by @itsyoboieltr, here are benchmarks running in 4 different configurations:

  • browserify build on self-hosted runners
  • browserify build on ubuntu-latest
  • webpack build on self-hosted runners
  • webpack build on ubuntu-latest

Probably the most important stat here is uiStartup (btw our old system highlights load). Two additional things I see:

  • Standard deviation is pretty low across all configurations, which is nice
  • All configurations are approximately twice as fast as our CircleCI machine

I'm going to run this again to see if it varies over time, and also with some other options.

PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
Browserify-self-hostHomefirstPaint1581195714302145
domContentLoaded82111778687536
load82411798737436
domInteractive14272021
backgroundConnect511711
firstReactRender12171211
getState35410
initialActions01000
loadScripts6188096554220
setupStore69710
uiStartup89912929718139
Browserify-ubuntu-latestHomefirstPaint2161061869299143
domContentLoaded965114410164019
load973115010224019
domInteractive266936147
backgroundConnect8121011
firstReactRender18372142
getState511821
initialActions01000
loadScripts7518747913015
setupStore9141111
uiStartup1090137111696330
Webpack-self-hostHomefirstPaint15473666811957
domContentLoaded666718685157
load670734692157
domInteractive22442552
backgroundConnect16241921
firstReactRender11131200
getState36510
initialActions01000
loadScripts656710676157
setupStore47510
uiStartup804882833178
Webpack-ubuntu-latestHomefirstPaint170896520318153
domContentLoaded7088908293517
load7148988363617
domInteractive21403442
backgroundConnect22292521
firstReactRender13151310
getState38611
initialActions00000
loadScripts6978798183517
setupStore58610
uiStartup88310499983316

@HowardBraham
Copy link
Contributor Author

Second run of this, numbers are pretty close to the first run

PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
Browserify-self-hostHomefirstPaint15790577620096
domContentLoaded802868833168
load807875839178
domInteractive14241921
backgroundConnect58710
firstReactRender11131210
getState35410
initialActions01000
loadScripts602669630178
setupStore57610
uiStartup8939869382311
Browserify-ubuntu-latestHomefirstPaint2091107863322155
domContentLoaded960108210083416
load963109110143517
domInteractive176436157
backgroundConnect7161021
firstReactRender18372242
getState512721
initialActions01000
loadScripts7548567882914
setupStore9231131
uiStartup1107126511524019
Webpack-self-hostHomefirstPaint17275267711756
domContentLoaded6557456952110
load669754705209
domInteractive227028105
backgroundConnect17241921
firstReactRender11131210
getState36410
initialActions01000
loadScripts6477376872110
setupStore47511
uiStartup8228978472010
Webpack-ubuntu-latestHomefirstPaint169878458307148
domContentLoaded8058958302110
load8129048372110
domInteractive268137126
backgroundConnect21272421
firstReactRender12141300
getState410611
initialActions01000
loadScripts7948838192110
setupStore58610
uiStartup96310819912613

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO-NOT-MERGE Pull requests that should not be merged team-extension-platform
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

4 participants