-
Notifications
You must be signed in to change notification settings - Fork 3.5k
WasmFS JS API: Implement mmap #20019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WasmFS JS API: Implement mmap #20019
Conversation
| @@ -1 +1 @@ | |||
| 2054 | |||
| 9174 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code size increase is quite concerning. From the .funcs above it looks like it's due to adding malloc support .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some changes, it looks like malloc isn't showing up in .funcs, but there's still a slight code size increase. Is this to be expected from the things I've added to library_wasmfs.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small increase with WasmFS+FORCE_FILESYSTEM is expected, but the changes here look odd. For example test/other/test_unoptimized_code_size.js.size doesn't even use WasmFS. Perhaps try --rebaseline after emsdk install tot and emcc --clear-cache to make sure you're building the latest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've been seeing the code size increases on main as well. I did both emsdk install tot, emsdk activate tot, and emcc --clear-cache. Are you able to reproduce this?
test_unoptimized_code_size (test_other.other) ... Clearing existing test directory
seen wasm size: 12153 (expected: 12153) (delta: 0), ratio to expected: 0.000000
seen js size: 60352 (expected: 58664) (delta: 1688), ratio to expected: 0.028774
seen wasm size: 11600 (expected: 11600) (delta: 0), ratio to expected: 0.000000
seen js size: 32938 (expected: 31978) (delta: 960), ratio to expected: 0.030021
seen wasm size: 12153 (expected: 12153) (delta: 0), ratio to expected: 0.000000
seen js size: 59249 (expected: 57603) (delta: 1646), ratio to expected: 0.028575
ok
----------------------------------------------------------------------
Ran 1 test in 2.106s
OK
(base) PS C:\Users\james\Documents\GitHub\emscripten> git branch
library-wasmfs-ioctl
library-wasmfs-mkdev
library-wasmfs-mmap
library-wasmfs-mmap-august
library-wasmfs-mount
library-wasmfs-mount-acct
library-wasmfs-readlink-fix
library-wasmfs-syncfs
* main
src/library.js
Outdated
| // Allocate memory for an mmap operation. This allocates space of the right | ||
| // page-aligned size, and clears the allocated space. | ||
| $mmapAlloc__deps: ['$zeroMemory', '$alignMemory'], | ||
| $mmapAlloc__deps: ['$zeroMemory', '$alignMemory', 'emscripten_builtin_memalign'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would guess this is the cause of the code size increases. Why is this necessary? I think perhaps we can leave this unchanged but make FORCE_FILESYSTEM in library_wasmfs include this method, as that is the only case it is needed? (since FS.mmap would only be added if FORCE_FILESYSTEM)
|
Code size issues might have been related to recent landings of LLVM library changes. But that should now be resolved and looks good here. |
This PR copies the changes from #19624 after the LLVM patch (https://reviews.llvm.org/D153466). I decided it might be easier to open a new PR since there were too many merge conflicts to resolve with the original branch back from June.