[WasmFS] Adds support for large (>4GB) files in OPFS using __i53abi #22561
Conversation
src/library_wasmfs_opfs.js
Outdated
| }, | ||
|
|
||
| _wasmfs_opfs_write_access__i53abi: true, | ||
| _wasmfs_opfs_write_access__sig: 'iipij', |
There was a problem hiding this comment.
None of these sigs should be needed since I believe they are already stored in src/library_sigs.js
There was a problem hiding this comment.
The sigs need to be updated. Not sure if those changes have been added to this request. I had to add them for testing, but are not needed once library_sigs.js is updated.
There was a problem hiding this comment.
Yes, changes to function signatures should be accompanied by the change in library_sigs.js. I normally rebuild using ./test/runner other.test_gen_sig_info --rebase.
There was a problem hiding this comment.
I've removed the sigs. Just need to make sure that they are updated.
There was a problem hiding this comment.
I should mention that during development when I add a __i53abi decorator to a function in my own library (outside of the emscripten library) and did not have an explicit sig, the implicit sigs would be wrong (in my case it used a BigInt because I had WASM_BIGINT selected). How are the sigs determined at compile/link time?
There was a problem hiding this comment.
__sig attributes are pre-calculated for builtin JS libraries (via tools/maint/gen_sig_info.py). For external libraries you need to specifiy the __sig manually, alongside the function.
I made a change recently that will make it impossible to forget to add the __sig when using __i53abi: #22557
|
|
||
| // TODO: return 64-byte off_t. | ||
| uint32_t _wasmfs_opfs_get_size_blob(int blob_id); | ||
| void _wasmfs_opfs_get_size_blob(int blob_id, off_t* size); |
There was a problem hiding this comment.
Why can't off_t be used as a return type here?
There was a problem hiding this comment.
Maybe it can. Not sure if/how i64 values are returned, so I just used the same method as the other get_size function to be safe.
There was a problem hiding this comment.
I think if you mark this function as i53abi and j is the return type it should "just work".
There was a problem hiding this comment.
Good to know. I'll keep that in mind for future updates.
There was a problem hiding this comment.
Can we make this part of this change? No need to the out parameter then a simple return type will do (IMHO).
There was a problem hiding this comment.
Can we make this part of this change? No need to the out parameter then a simple return type will do (IMHO).
There was a problem hiding this comment.
I've change it to return the size directly. Seems to work fine.
Sigs in library_sigs.js need to be updated.
sbc100
changed the title
|
lgtm, but that do we think about testing this? |
|
I am not sure we can test this well. We'd need a file larger than 4GB for that, which will strain CI limits. Even creating such a file on disk and using some kind of streaming read might be challenging (and I don't think we have a backend that does such streaming reads in a guaranteed manner). So I'd be ok landing this without specific testing for 4GB+ files. |
4 participants
Currently wasmfs OPFS is limited to 2GB files or smaller. These changes allow working with much larger files by using the __i53abi decorator.
Note that individual reads/writes are still limited to uint32_t block sizes.