From 43c3ecb414d4de731a9f68c3793970c68938143c 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. This involves writing the size measurements to separate JSON files (one per matrix job) and retaining them as artifacts, as there's no built-in way to collect all matrix job outputs into a subsequent job's inputs. --- .../report-code-size-changes/action.yml | 126 ++++++++++++------ .github/workflows/check-binary-size.yml | 66 +++++++-- 2 files changed, 140 insertions(+), 52 deletions(-) diff --git a/.github/actions/report-code-size-changes/action.yml b/.github/actions/report-code-size-changes/action.yml index b029aa02..ede26975 100644 --- a/.github/actions/report-code-size-changes/action.yml +++ b/.github/actions/report-code-size-changes/action.yml @@ -1,14 +1,21 @@ -# Github composite action to report on code size changes +# 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 resulting from a PR as a comment on the PR - (accessed via context). + 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: - reference: - description: The size in bytes of the reference binary (base of PR). - required: true - updated: - description: The size in bytes of the updated binary (head of PR). + data-directory: + description: > + Path to directory containing size data as a set of "*.json" files. required: true runs: using: composite @@ -16,56 +23,89 @@ runs: - name: Post a PR comment if the size has changed uses: actions/github-script@v6 env: - SIZE_REFERENCE: ${{ inputs.reference }} - SIZE_UPDATED: ${{ inputs.updated }} + DATA_DIRECTORY: ${{ inputs.data-directory }} with: script: | - const reference = process.env.SIZE_REFERENCE; - const updated = process.env.SIZE_UPDATED; + const fs = require("fs"); - if (!(reference > 0)) { - core.setFailed(`Reference size invalid: ${reference}`); - return; - } + const size_dir = process.env.DATA_DIRECTORY; - if (!(updated > 0)) { - core.setFailed(`Updated size invalid: ${updated}`); - return; - } + // 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 formatter = Intl.NumberFormat("en", {useGrouping: "always"}); + const diff_pct_str = Intl.NumberFormat("en", { + style: "percent", + useGrouping: "always", + sign: "exceptZero", + maximumFractionDigits: 2 + }).format(diff_pct); - const updated_str = formatter.format(updated); - const reference_str = formatter.format(reference); + 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}\`: - const diff = updated - reference; - const diff_pct = (updated / reference) - 1; + - Original binary size: **${reference_str} B** + - Updated binary size: **${updated_str} B** + - Difference: **${diff_str} B** (${diff_pct_str}) - 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); + return [report]; + } else { + return []; + } + }); - 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. + // 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: - Original binary size: **${reference_str} B** - Updated binary size: **${updated_str} B** - Difference: **${diff_str} B** (${diff_pct_str})`; + ${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 35dc99a6..d045fb7b 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 @@ -96,8 +140,12 @@ jobs: steps: # Clone backtrace to access Github composite actions to report size. - uses: actions/checkout@v3 - # Run the size reporting action. - - uses: ./.github/actions/report-code-size-changes + - name: Download size data + uses: actions/download-artifact@v3 + with: + name: size-files + path: ${{ env.SIZE_DATA_DIR }} + - name: Analyze and report size changes + uses: ./.github/actions/report-code-size-changes with: - reference: ${{ needs.measure.outputs.binary-size-reference }} - updated: ${{ needs.measure.outputs.binary-size-updated }} + data-directory: ${{ env.SIZE_DATA_DIR }}