From 2e5cc5f2eb7bde79be2529019fb4df8128327a7f Mon Sep 17 00:00:00 2001 From: Jason Heeris Date: Thu, 20 Jul 2023 13:24:29 +0800 Subject: [PATCH] Use matrix strategy to measure size data on multiple platforms. --- .../report-code-size-changes/action.yml | 112 +++++++++++++++++ .github/workflows/check-binary-size.yml | 118 +++++++++--------- 2 files changed, 170 insertions(+), 60 deletions(-) create mode 100644 .github/actions/report-code-size-changes/action.yml diff --git a/.github/actions/report-code-size-changes/action.yml b/.github/actions/report-code-size-changes/action.yml new file mode 100644 index 000000000..ccbaece06 --- /dev/null +++ b/.github/actions/report-code-size-changes/action.yml @@ -0,0 +1,112 @@ +# Github composite action to report on code size changes across different +# platforms. + +name: Report binary size changes on PR +description: | + Report on code size changes across different platforms resulting from a PR. + The only input argument is the path to a directory containing a set of + "*.json" files (extension required), each file containing the keys: + + - platform: the platform that the code size change was measured on + - reference: the size in bytes of the reference binary (base of PR) + - updated: the size in bytes of the updated binary (head of PR) + + The size is reported as a comment on the PR (accessed via context). +inputs: + data-directory: + description: > + Path to directory containing size data as a set of "*.json" files. + required: true +runs: + using: composite + steps: + - name: Post a PR comment if the size has changed + uses: actions/github-script@v6 + env: + DATA_DIRECTORY: ${{ inputs.data-directory }} + with: + script: | + const fs = require("fs"); + + const size_dir = process.env.DATA_DIRECTORY; + + // Map the set of all the *.json files into an array of objects. + const globber = await glob.create(`${size_dir}/*.json`); + const files = await globber.glob(); + const sizes = files.map(path => { + const contents = fs.readFileSync(path); + return JSON.parse(contents); + }); + + // Map each object into some text, but only if it shows any difference + // to report. + const size_reports = sizes.flatMap(size_data => { + const platform = size_data["platform"]; + const reference = size_data["reference"]; + const updated = size_data["updated"]; + + if (!(reference > 0)) { + core.setFailed(`Reference size invalid: ${reference}`); + return; + } + + if (!(updated > 0)) { + core.setFailed(`Updated size invalid: ${updated}`); + return; + } + + const formatter = Intl.NumberFormat("en", { + useGrouping: "always" + }); + + const updated_str = formatter.format(updated); + const reference_str = formatter.format(reference); + + const diff = updated - reference; + const diff_pct = (updated / reference) - 1; + + const diff_str = Intl.NumberFormat("en", { + useGrouping: "always", + sign: "exceptZero" + }).format(diff); + + const diff_pct_str = Intl.NumberFormat("en", { + style: "percent", + useGrouping: "always", + sign: "exceptZero", + maximumFractionDigits: 2 + }).format(diff_pct); + + if (diff !== 0) { + // The body is created here and wrapped so "weirdly" to avoid whitespace at the start of the lines, + // which is interpreted as a code block by Markdown. + const report = `On platform \`${platform}\`: + + - Original binary size: **${reference_str} B** + - Updated binary size: **${updated_str} B** + - Difference: **${diff_str} B** (${diff_pct_str}) + + `; + + return [report]; + } else { + return []; + } + }); + + // If there are any size changes to report, format a comment and post + // it. + if (size_reports.length > 0) { + const comment_sizes = size_reports.join(""); + const body = `Code size changes for a hello-world Rust program linked with libstd with backtrace: + + ${comment_sizes}`; + + github.rest.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body + }); + } + diff --git a/.github/workflows/check-binary-size.yml b/.github/workflows/check-binary-size.yml index 0d189b951..886361e79 100644 --- a/.github/workflows/check-binary-size.yml +++ b/.github/workflows/check-binary-size.yml @@ -9,13 +9,20 @@ on: branches: - master +# Both the "measure" and "report" jobs need to know this. +env: + SIZE_DATA_DIR: sizes + # Responsibility is divided between two jobs "measure" and "report", so that the # job that builds (and potentnially runs) untrusted code does not have PR write # permission, and vice-versa. jobs: measure: name: Check binary size - runs-on: ubuntu-latest + strategy: + matrix: + platform: [ubuntu-latest, windows-latest] + runs-on: ${{ matrix.platform }} permissions: contents: read env: @@ -26,9 +33,7 @@ jobs: TEST_MAIN_RS: foo.rs BASE_COMMIT: ${{ github.event.pull_request.base.sha }} HEAD_COMMIT: ${{ github.event.pull_request.head.sha }} - outputs: - binary-size-reference: ${{ steps.size-reference.outputs.test-binary-size }} - binary-size-updated: ${{ steps.size-updated.outputs.test-binary-size }} + SIZE_DATA_FILE: size-${{ strategy.job-index }}.json steps: - name: Print info shell: bash @@ -37,7 +42,7 @@ jobs: echo "Base SHA: $BASE_COMMIT" # Note: the backtrace source that's cloned here is NOT the version to be # patched in to std. It's cloned here to access the Github action for - # building the test binary and measuring its size. + # building and measuring the test binary. - name: Clone backtrace to access Github action uses: actions/checkout@v3 with: @@ -87,6 +92,45 @@ jobs: main-rs: ${{ env.TEST_MAIN_RS }} rustc-dir: ${{ env.RUSTC_DIR }} id: size-updated + # There is no built-in way to "collect" all the outputs of a set of jobs + # run with a matrix strategy. Subsequent jobs that have a "needs" + # dependency on this one will be run once, when the last matrix job is + # run. Appending data to a single file within a matrix is subject to race + # conditions. So we write the size data to files with distinct names + # generated from the job index. + - name: Write sizes to file + uses: actions/github-script@v6 + env: + SIZE_REFERENCE: ${{ steps.size-reference.outputs.test-binary-size }} + SIZE_UPDATED: ${{ steps.size-updated.outputs.test-binary-size }} + PLATFORM: ${{ matrix.platform }} + with: + script: | + const fs = require("fs"); + const path = require("path"); + + fs.mkdirSync(process.env.SIZE_DATA_DIR, {recursive: true}); + + const output_data = JSON.stringify({ + platform: process.env.PLATFORM, + reference: process.env.SIZE_REFERENCE, + updated: process.env.SIZE_UPDATED, + }); + + // The "wx" flag makes this fail if the file exists, which we want, + // because there should be no collisions. + fs.writeFileSync( + path.join(process.env.SIZE_DATA_DIR, process.env.SIZE_DATA_FILE), + output_data, + { flag: "wx" }, + ); + - name: Upload size data + uses: actions/upload-artifact@v3 + with: + name: size-files + path: ${{ env.SIZE_DATA_DIR }}/${{ env.SIZE_DATA_FILE }} + retention-days: 1 + if-no-files-found: error report: name: Report binary size changes runs-on: ubuntu-latest @@ -94,59 +138,13 @@ jobs: permissions: pull-requests: write steps: - - name: Post a PR comment if the size has changed - uses: actions/github-script@v6 - env: - SIZE_REFERENCE: ${{ needs.measure.outputs.binary-size-reference }} - SIZE_UPDATED: ${{ needs.measure.outputs.binary-size-updated }} + # Clone backtrace to access Github composite actions to report size. + - uses: actions/checkout@v3 + - name: Download size data + uses: actions/download-artifact@v3 with: - script: | - const reference = process.env.SIZE_REFERENCE; - const updated = process.env.SIZE_UPDATED; - - if (!(reference > 0)) { - core.setFailed(`Reference size invalid: ${reference}`); - return; - } - - if (!(updated > 0)) { - core.setFailed(`Updated size invalid: ${updated}`); - return; - } - - const formatter = Intl.NumberFormat("en", {useGrouping: "always"}); - - const updated_str = formatter.format(updated); - const reference_str = formatter.format(reference); - - const diff = updated - reference; - const diff_pct = (updated / reference) - 1; - - const diff_str = Intl.NumberFormat("en", { - useGrouping: "always", - sign: "exceptZero" - }).format(diff); - - const diff_pct_str = Intl.NumberFormat("en", { - style: "percent", - useGrouping: "always", - sign: "exceptZero", - maximumFractionDigits: 2 - }).format(diff_pct); - - if (diff !== 0) { - // The body is created here and wrapped so "weirdly" to avoid whitespace at the start of the lines, - // which is interpreted as a code block by Markdown. - const body = `Below is the size of a hello-world Rust program linked with libstd with backtrace. - - Original binary size: **${reference_str} B** - Updated binary size: **${updated_str} B** - Difference: **${diff_str} B** (${diff_pct_str})`; - - github.rest.issues.createComment({ - issue_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo, - body - }) - } + name: size-files + path: ${{ env.SIZE_DATA_DIR }} + - uses: ./.github/actions/report-code-size-changes + with: + data-directory: ${{ env.SIZE_DATA_DIR }}