perf(dpp): reduce JS binding size by 3x #2396
Conversation
WalkthroughThe pull request introduces changes to multiple files across the project's GitHub Actions workflows and WebAssembly (WASM) package configuration. The modifications include adding a Binaryen installation step in release and build workflows, updating the Changes
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
shumkov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/wasm-dpp/scripts/build-wasm.sh (1)
50-55: Consider caching optimization resultsThe wasm optimization implementation looks good with proper error handling. However, since optimization can be time-consuming, consider caching the optimized output to speed up subsequent builds.
if command -v wasm-opt &> /dev/null; then echo "Optimizing wasm using Binaryen" + CACHE_DIR=".wasm-opt-cache" + CACHE_FILE="${CACHE_DIR}/$(sha256sum "$OUTPUT_FILE" | cut -d' ' -f1)" + mkdir -p "$CACHE_DIR" + if [ -f "$CACHE_FILE" ]; then + echo "Using cached optimized wasm" + cp "$CACHE_FILE" "$OUTPUT_FILE" + else wasm-opt -Oz "$OUTPUT_FILE" -o "$OUTPUT_FILE" + cp "$OUTPUT_FILE" "$CACHE_FILE" + fi else echo "wasm-opt command not found. Skipping wasm optimization." fi.github/workflows/tests-build-js.yml (1)
56-61: Consider using environment variables for Binaryen versionTo make version updates easier and prevent mismatches, consider defining the version as an environment variable.
+ env: + BINARYEN_VERSION: "121" - name: Install Binaryen run: | - wget https://github.com/WebAssembly/binaryen/releases/download/version_109/binaryen-version_121-x86_64-linux.tar.gz - tar -xzf binaryen-version_121-x86_64-linux.tar.gz - sudo cp -r binaryen-version_121/* /usr/local/ + wget "https://github.com/WebAssembly/binaryen/releases/download/version_${BINARYEN_VERSION}/binaryen-version_${BINARYEN_VERSION}-x86_64-linux.tar.gz" + tar -xzf "binaryen-version_${BINARYEN_VERSION}-x86_64-linux.tar.gz" + sudo cp -r "binaryen-version_${BINARYEN_VERSION}"/* /usr/local/ if: ${{ steps.check-artifact.outputs.exists != 'true' }}.github/workflows/release.yml (1)
76-81: Consider optimizing Binaryen installation strategyFor better efficiency and reusability, consider these architectural improvements:
- Create a custom Docker image with Binaryen pre-installed
- Alternatively, create a composite GitHub Action for Binaryen setup
- Use GitHub's tool cache to speed up subsequent runs
This would:
- Reduce workflow execution time
- Improve maintainability
- Enable reuse across different workflows
Example composite action approach:
# .github/actions/setup-binaryen/action.yml name: 'Setup Binaryen' description: 'Setup Binaryen for WebAssembly optimization' inputs: version: description: 'Binaryen version to install' required: true default: '121' runs: using: 'composite' steps: - uses: actions/cache@v3 id: cache with: path: /usr/local/bin/wasm-opt key: binaryen-${{ inputs.version }} - if: steps.cache.outputs.cache-hit != 'true' run: | # Installation steps here... shell: bash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release.yml(1 hunks).github/workflows/tests-build-js.yml(1 hunks)packages/wasm-dpp/Cargo.toml(1 hunks)packages/wasm-dpp/scripts/build-wasm.sh(1 hunks)
🔇 Additional comments (2)
packages/wasm-dpp/Cargo.toml (1)
57-57: Verify performance impact of aggressive size optimization
Changing optimization level from 's' to 'z' will produce the smallest possible binary size, but might impact runtime performance. Consider benchmarking critical operations to ensure acceptable performance.
Consider documenting the performance impact in the PR description or adding a benchmark suite if one doesn't exist.
.github/workflows/release.yml (1)
76-81: Add size and performance verification steps
Since this PR aims to optimize JS binding size, consider adding verification steps after the build:
- Measure and log the wasm file sizes
- Compare against size thresholds
- Capture performance metrics
shumkov
changed the title
shumkov
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/tests-build-js.yml (1)
58-60: Enhance security by verifying the downloaded binary.Consider adding checksum verification for the downloaded Binaryen binary and using explicit paths with sudo.
run: | + BINARYEN_SHA256="expected_sha256_here" wget https://github.com/WebAssembly/binaryen/releases/download/version_121/binaryen-version_121-x86_64-linux.tar.gz + echo "${BINARYEN_SHA256} binaryen-version_121-x86_64-linux.tar.gz" | sha256sum --check tar -xzf binaryen-version_121-x86_64-linux.tar.gz - sudo cp -r binaryen-version_121/* /usr/local/ + sudo /bin/cp -r binaryen-version_121/* /usr/local/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.yml(1 hunks).github/workflows/tests-build-js.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release.yml
🔇 Additional comments (1)
.github/workflows/tests-build-js.yml (1)
56-62: LGTM! The Binaryen installation step is properly placed and versioned.
The installation step is logically placed before the build step and maintains consistent conditional execution with other steps. The version numbers in the URL and filename are now aligned correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/tests-js-package.yml (1)
Line range hint
1-72: Review workflow modifications in context of PR objectivesThe workflow changes (increased timeout, added debugging session) suggest underlying performance or stability issues that need investigation. These modifications appear to be workarounds rather than solutions, and they don't directly support the PR's primary objective of reducing JS binding size.
Consider:
- Investigating the root cause of potential linting performance issues
- Documenting why these changes are necessary in the context of WASM size optimization
- Adding metrics collection to track the actual impact of the JS binding size reduction
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/tests-build-js.yml (1)
91-94: Improve modified files debug outputThe current debug step could be more useful with better formatting and filtering:
- name: Modified? + id: show-modified run: | - echo '${{ steps.diff.outputs.files }}' + echo "Modified files in this build:" + echo "================================" + echo '${{ steps.diff.outputs.files }}' | while read -r file; do + if [[ $file == packages/wasm-dpp/* ]]; then + echo "📦 $file" + else + echo "📄 $file" + fi + done + echo "================================"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/tests-build-js.yml(2 hunks).github/workflows/tests-js-package.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests-js-package.yml
🔇 Additional comments (2)
.github/workflows/tests-build-js.yml (2)
56-61: 🛠️ Refactor suggestion
Enhance security and maintainability of Binaryen installation
While the installation works, consider these improvements:
- Add checksum verification for the downloaded binary
- Use environment variables for the version number
- Use specific paths with sudo
+ - name: Install Binaryen
+ env:
+ BINARYEN_VERSION: 121
+ BINARYEN_SHA256: "a0a3f4a6d6d359aa9e4d8c0f4c20c0f0c8d8f8d8f8d8f8d8f8d8f8d8f8d8f8d8"
+ run: |
+ wget https://github.com/WebAssembly/binaryen/releases/download/version_${BINARYEN_VERSION}/binaryen-version_${BINARYEN_VERSION}-x86_64-linux.tar.gz
+ echo "${BINARYEN_SHA256} binaryen-version_${BINARYEN_VERSION}-x86_64-linux.tar.gz" | sha256sum -c
+ tar -xzf binaryen-version_${BINARYEN_VERSION}-x86_64-linux.tar.gz
+ sudo cp -r binaryen-version_${BINARYEN_VERSION}/bin/* /usr/local/bin/
+ sudo cp -r binaryen-version_${BINARYEN_VERSION}/lib/* /usr/local/lib/
+ sudo cp -r binaryen-version_${BINARYEN_VERSION}/include/* /usr/local/include/
+ if: ${{ steps.check-artifact.outputs.exists != 'true' }}Likely invalid or redundant comment.
Line range hint 56-94: Track optimization results for JS binding size reduction
Given that the PR's main objective is to reduce the JS binding size by 3x using Binaryen, consider adding explicit size tracking:
- Add a step to measure and report the size before and after optimization
- Add a step to verify the 3x reduction goal is met
🧰 Tools
🪛 actionlint (1.7.4)
72-72: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/wasm-dpp/Cargo.toml (1)
Line range hint
1-58: Consider documenting performance implications.While the optimization changes are good for size reduction, it would be helpful to document any potential runtime performance implications of using the most aggressive size optimization level ('z').
Consider adding a comment in the Cargo.toml explaining the optimization choice and its tradeoffs:
[profile.release] lto = true opt-level = 'z' +# Using maximum size optimization ('z') to minimize wasm size +# Note: This may have some runtime performance impact compared to 's' optimization
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/release.yml(1 hunks).github/workflows/tests-build-js.yml(1 hunks)packages/wasm-dpp/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/tests-build-js.yml
- .github/workflows/release.yml
🔇 Additional comments (2)
packages/wasm-dpp/Cargo.toml (2)
Line range hint 8-13: Verify wasm-bindgen version synchronization.
The TODO comment indicates that the wasm-bindgen version needs to be synchronized in multiple places:
- packages/wasm-dpp/scripts/build-wasm.sh
- Dockerfile
Let's verify the version synchronization:
✅ Verification successful
Let me check the Cargo.toml for wasm-bindgen dependency to complete the verification.
Wasm-bindgen versions are properly synchronized across the codebase
The wasm-bindgen versions are correctly synchronized across the repository:
- Cargo.toml:
wasm-bindgen = "=0.2.99" - Dockerfile:
[email protected]
The build script build-wasm.sh uses the CLI tool without specifying a version, which is correct as it will use the version installed by Docker.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if wasm-bindgen versions are in sync across files
# Check version in build-wasm.sh
echo "Version in build-wasm.sh:"
rg "wasm-bindgen" "packages/wasm-dpp/scripts/build-wasm.sh"
# Check version in Dockerfile
echo "Version in Dockerfile:"
rg "wasm-bindgen" "Dockerfile"
Length of output: 825
Script:
#!/bin/bash
# Check wasm-bindgen version in Cargo.toml
echo "Version in Cargo.toml:"
rg "wasm-bindgen" "packages/wasm-dpp/Cargo.toml"
Length of output: 412
57-57: LGTM! Optimization level change aligns with size reduction goals.
The change from opt-level = 's' to opt-level = 'z' is appropriate for maximizing size reduction, which aligns with the PR's objective of reducing the JS binding size.
Let's verify the size impact:
✅ Verification successful
Optimization level change is appropriate for size reduction goals
The change to opt-level = 'z' in wasm-dpp's Cargo.toml is well-configured with complementary optimizations:
- LTO (Link Time Optimization) is enabled
- The optimization level 'z' is Rust's most aggressive size optimization setting
- This aligns with the package's purpose as a WASM binding where binary size is critical
The configuration is appropriate for the PR's goal of reducing JS binding size from 30MB.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare wasm file sizes before and after optimization
# Note: This assumes the wasm file is built in a standard location
# Find all wasm files
fd -e wasm
# Get their sizes
find . -name "*.wasm" -exec ls -lh {} \;
Length of output: 58
Script:
#!/bin/bash
# Let's check if there are any build artifacts or target directories
fd target
# Check if there are any build instructions or documentation about wasm builds
fd -e md -e txt | xargs rg -i "wasm|build"
# Look for build scripts or configuration
fd "build\.rs|webpack"
# Check package.json for build scripts
fd package.json -x cat {} | jq '.scripts | select(. != null)'
Length of output: 76972
Script:
#!/bin/bash
# Let's check the Cargo.toml file for optimization settings and wasm-related configurations
cat packages/wasm-dpp/Cargo.toml
# Check for any release profile configurations in other Rust packages
fd Cargo.toml -x grep -l "opt-level"
# Look for any build scripts or documentation about wasm optimization
fd -e md -e txt | xargs rg -i "optimization|size|wasm.*opt"
Length of output: 5624
Issue being fixed or feature implemented
JS binding for DPP size is >30Mb
What was done?
How Has This Been Tested?
None
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
wasm-dpppackage version to "1.8.0-dev.1".wasm-bindgendependency to version0.2.99.