[charts] Profile charts benchmarks using chromium #18528
Conversation
|
Deploy preview: https://deploy-preview-18528--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: 0B(0.00%) - Total Gzip Change: 0B(0.00%) Show details for 100 more bundles (22 more not shown)@mui/x-charts parsed: 0B(0.00%) gzip: 0B(0.00%) |
933c5e4 to
ca329c0
Compare
CodSpeed Performance ReportMerging #18528 will not alter performanceComparing Summary
|
| - name: Upload results | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: charts-benchmarks-results-master.json | ||
| path: ./test/performance-charts/results.json | ||
| if-no-files-found: error |
There was a problem hiding this comment.
You probably want the file-name to be based on the branch
| // Read results file | ||
| let results = '{}'; | ||
| try { | ||
| results = exec('pnpm --filter @mui-x-internal/performance-charts test:performance:ci', 'utf8'); |
There was a problem hiding this comment.
You ran it on line 44, why is it being ran again?
| console.error('Error running performance benchmarks.', error); | ||
| } | ||
|
|
||
| const body = `${marker} |
There was a problem hiding this comment.
I would separate running the tests and commenting. Store the result somewhere, then just create a new step to create a comment.
There was a problem hiding this comment.
just a heads up: we are actively trying to remove noise on PRs and adding comments is only encouraged when there is no other way of doing this.
TBH you can do that, but make sure this only runs once or the existing comment gets rewritten.
You could also explore adding a separate section to the PR body instead.
For commenting and updating we previously used: https://github.com/peter-evans/create-or-update-comment
There was a problem hiding this comment.
I think if we are running everything ourselves, it can be better to just use the ci status instead, if we want to decrease noise.
Then fail the CI if it crosses a threshold.
We can eventually setup an accept flow. But it takes a lot more time. We could just "merge anyway" for now if we need to accept a decrease
There was a problem hiding this comment.
Makes sense. I'll take that into account and check what to do.
My reasoning for using comments is because our current solution also comments in our PRs: example.
Setting the CI status also probably makes sense. Just trying things for now.
There was a problem hiding this comment.
yeah no worries, they are just ideas 👍
| - name: Run performance tests | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | |
There was a problem hiding this comment.
In mui-public we can the script, so we can have it typed: https://github.com/mui/mui-public/blob/master/.github/workflows/prs_check-if-pr-has-type-label.yml
eg:
script: |
const script = require('./.github/workflows/scripts/prs/checkTypeLabel.js');
await script({core, github, context});
https://github.com/mui/mui-public/blob/master/.github/workflows/scripts/prs/checkTypeLabel.js
There was a problem hiding this comment.
For this to work you need to checkout the repo first
There was a problem hiding this comment.
| - name: Run performance tests | ||
| uses: actions/github-script@v7 | ||
| with: |
There was a problem hiding this comment.
Also you will want to comment only when on a PR
| - 'next' | ||
| paths: | ||
| - 'packages/x-charts*/**' | ||
| pull_request: |
There was a problem hiding this comment.
| pull_request: | |
| pull_request_target: |
There was a problem hiding this comment.
Don't think this has worked, @michelengelen. The job doesn't show up in CI 🤔
Maybe it's only a permissions issue?
There was a problem hiding this comment.
No, this is correct. The pull_request can't have access to repo's secrets. So it is using your fork repo's secrets to try and add a PR comment in the original/mui-x repo
There was a problem hiding this comment.
The issue here is that these changes need to be in master for the PR to successfully run. And since the Action needs the repo's secrets to create comments, we will never be able to actually test the commenting workflow in this PR 😆
There was a problem hiding this comment.
thats why we make use of the reusable workflows as well ... we can easily add or remove them to test (or run manually)
There was a problem hiding this comment.
I see, so I'll get this PR ready for review and we can test it once it's merged.
| console.error('Error running performance benchmarks.', error); | ||
| } | ||
|
|
||
| const body = `${marker} |
There was a problem hiding this comment.
just a heads up: we are actively trying to remove noise on PRs and adding comments is only encouraged when there is no other way of doing this.
TBH you can do that, but make sure this only runs once or the existing comment gets rewritten.
You could also explore adding a separate section to the PR body instead.
For commenting and updating we previously used: https://github.com/peter-evans/create-or-update-comment
|
Could we maybe split this PR in 2?
|
368cdf4 to
ad93804
Compare
2506f80 to
7a3aa11
Compare
Signed-off-by: Bernardo Belchior <Bernardo.belchior1@gmail.com>
| env: { TRACE: isTrace ? 'true' : 'false' }, | ||
| environment: isTrace ? 'node' : 'jsdom', |
There was a problem hiding this comment.
We should probably always run this in browser mode though
There was a problem hiding this comment.
Regardless if tracing or not I mean
There was a problem hiding this comment.
I didn't want to change the defaults. Using JSDOM is consistent with CI, so it's probably better to investigate performance issues. Once our CI is running in the browser, we can switch. What do you think?
There was a problem hiding this comment.
ah, I missed you removed the ci stuff
There was a problem hiding this comment.
Yeah, this PR only includes the trace. I'll handle the CI stuff in a separate PR.
This PR allows profiling charts benchmark using Chromium. It will create a performance profile in the
test/performance-charts/tracesthat can be imported into Chrome for investigation.You can run a trace by executing
pnpm test:performance:trace. Optionally, you can run it for a single file by doingpnpm test:performance:trace ScatterChart.bench.tsx.