fix(build): Increase maximum wasm memory size from 2GB to 4GB (fixes #25). #38
Conversation
WalkthroughThe changes in this pull request focus on updating the Changes
Possibly related issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CMakeLists.txt (1)
69-73: Clarify memory growth strategy with both ALLOW_MEMORY_GROWTH and MAXIMUM_MEMORYThe configuration uses both
-sALLOW_MEMORY_GROWTHand-sMAXIMUM_MEMORY=4GB. Consider documenting the intended memory growth behaviour:
- Initial memory allocation
- Growth increments
- Impact on performance
Consider adding a comment in the CMakeLists.txt explaining the memory configuration strategy.
set(CLP_FFI_JS_COMMON_LINK_OPTIONS -fwasm-exceptions -sALLOW_MEMORY_GROWTH -sEXPORT_ES6 + # Allow dynamic memory growth up to 4GB to handle large log processing + # Initial memory: Default + # Growth: On-demand up to maximum -sMAXIMUM_MEMORY=4GB -sMODULARIZE -sWASM_BIGINT )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CMakeLists.txt(1 hunks)
🔇 Additional comments (2)
CMakeLists.txt (2)
Line range hint 11-16: LGTM: Project version update follows semantic versioning
The project version is properly specified using semantic versioning format.
71-71: Memory increase looks good, but verify low-memory environment compatibility
The increase to 4GB is well-justified by the performance tests and minimal file size impact. However, we should verify the behaviour in low-memory environments.
Let's check if there are any environment-specific memory configurations:
✅ Verification successful
Memory configuration is appropriate for both Node.js and Web Worker environments
The memory settings are well-configured with both ALLOW_MEMORY_GROWTH and a reasonable 4GB maximum limit. This setup supports:
- Dynamic memory growth as needed
- Both Node.js and Web Worker environments
- Large dataset processing (4.5M log events)
The configuration provides flexibility for both high and low-memory scenarios since:
- Memory growth is allowed, starting small and expanding as needed
- The 4GB maximum acts as a safety cap, not an initial allocation
- Both supported environments (node and worker) can handle dynamic memory management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any memory-related configurations or environment checks
rg -i "memory|heap" --type cmake
Length of output: 206
Script:
#!/bin/bash
# Let's check the context around memory settings and any conditional configurations
rg -B3 -A3 "MAXIMUM_MEMORY|ALLOW_MEMORY_GROWTH" CMakeLists.txt
# Also check for any environment-specific configurations
rg -i "environment|platform|target" CMakeLists.txt
Length of output: 991
Script:
#!/bin/bash
# Let's check what environments are supported
rg -B2 -A2 "CLP_FFI_JS_SUPPORTED_ENVIRONMENTS" CMakeLists.txt
# And check if there are any memory-related configurations in the source files
rg -i "memory|heap" src/ --type cpp
Length of output: 822
|
discussed with @davemarco offline and he will review this later in the evening |
|
Do you want to check that this causes no bugs in firefox/chrome? Normally what i have been doing is adding a "dist" folder to clp-ffi-js repo. Then I add the build files into the dist folder. Then I run |
There was a problem hiding this comment.
see comment above. I am just curious about testing since in docs, they basically warn you against using the feature (https://emscripten.org/docs/tools_reference/settings_reference.html#maximum-memory)
Right, it was tested working in below browsers:
|
|
For title how about
|
|
Also reminder to close #25 manually when you merge this since it is an enhancement and not a bug |
junhaoliao
changed the title
MAXIMUM_MEMORY=4GB in linker flags (fixes #25).
Description
MAXIMUM_MEMORY=4GBin linker flags.Validation performed
v22.9.0, ran perf test 3 times with a version0.1.0structured log stream file containing 4.5 million log event entries and observed no significant drop in time performance.Test code:
1.23 KB (1,261 bytes)->1.23 KB (1,261 bytes)(unchanged).25.8 KB (26,427 bytes)->27.3 KB (28,007 bytes)(5% increase).742 KB (760,314 bytes)->742 KB (760,314 bytes)(unchanged).Summary by CodeRabbit