Skip to content

[WasmFS] Adds support for large (>4GB) files in OPFS using __i53abi #22561

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Sep 24, 2024
Merged

[WasmFS] Adds support for large (>4GB) files in OPFS using __i53abi #22561

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2936165
Add support for large (>4GB) files using __i53abi
goldwaving Sep 11, 2024
a2bcb3f
Add support for large files (>4GB) using __i53abi
goldwaving Sep 11, 2024
8dbd011
Add support for large files (>4GB) using __i53abi
goldwaving Sep 11, 2024
e51dbc2
Added support for large (>4GB) files in OPFS
goldwaving Sep 11, 2024
89ec894
Change sig for wasm OPFS blob large file (>4GB) support
goldwaving Sep 12, 2024
79f0f55
Returned large (>4GB) blob size directly. Remove unneeded sigs.
goldwaving Sep 12, 2024
e98906a
Returned large file (>4GB) blob size directly
goldwaving Sep 12, 2024
689a110
Returned large file (>4GB) blob size directly
goldwaving Sep 12, 2024
89cd47e
Minor whitespace fix
goldwaving Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/library_wasmfs_opfs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,8 @@ addToLibrary({
wasmfsOPFSBlobs.free(blobID);
},

_wasmfs_opfs_read_access__i53abi: true,
_wasmfs_opfs_read_access__sig: 'iipij',
_wasmfs_opfs_read_access__deps: ['$wasmfsOPFSAccessHandles'],
_wasmfs_opfs_read_access: {{{ asyncIf(!PTHREADS) }}} function(accessID, bufPtr, len, pos) {
let accessHandle = wasmfsOPFSAccessHandles.get(accessID);
Expand All @@ -334,6 +336,8 @@ addToLibrary({
}
},

_wasmfs_opfs_read_blob__i53abi: true,
_wasmfs_opfs_read_blob__sig: 'vpipijp',
_wasmfs_opfs_read_blob__deps: ['$wasmfsOPFSBlobs', '$wasmfsOPFSProxyFinish'],
_wasmfs_opfs_read_blob: async function(ctx, blobID, bufPtr, len, pos, nreadPtr) {
let blob = wasmfsOPFSBlobs.get(blobID);
Expand Down Expand Up @@ -363,6 +367,8 @@ addToLibrary({
wasmfsOPFSProxyFinish(ctx);
},

_wasmfs_opfs_write_access__i53abi: true,
_wasmfs_opfs_write_access__sig: 'iipij',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these sigs should be needed since I believe they are already stored in src/library_sigs.js

Copy link
Contributor Author

@goldwaving goldwaving Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the sigs. Just need to make sure that they are updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__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

_wasmfs_opfs_write_access__deps: ['$wasmfsOPFSAccessHandles'],
_wasmfs_opfs_write_access: {{{ asyncIf(!PTHREADS) }}} function(accessID, bufPtr, len, pos) {
let accessHandle = wasmfsOPFSAccessHandles.get(accessID);
Expand Down Expand Up @@ -394,9 +400,10 @@ addToLibrary({
},

_wasmfs_opfs_get_size_blob__deps: ['$wasmfsOPFSBlobs'],
_wasmfs_opfs_get_size_blob: (blobID) => {
_wasmfs_opfs_get_size_blob: (blobID, sizePtr) => {
// This cannot fail.
return wasmfsOPFSBlobs.get(blobID).size;
let size = wasmfsOPFSBlobs.get(blobID).size;
{{{ makeSetValue('sizePtr', 0, 'size', 'i64') }}};
},

_wasmfs_opfs_get_size_file__deps: ['$wasmfsOPFSFileHandles', '$wasmfsOPFSProxyFinish'],
Expand All @@ -413,6 +420,7 @@ addToLibrary({
},

_wasmfs_opfs_set_size_access__i53abi: true,
_wasmfs_opfs_set_size_access__sig: 'vpijp',
_wasmfs_opfs_set_size_access__deps: ['$wasmfsOPFSAccessHandles', '$wasmfsOPFSProxyFinish'],
_wasmfs_opfs_set_size_access: async function(ctx, accessID, size, errPtr) {
let accessHandle = wasmfsOPFSAccessHandles.get(accessID);
Expand All @@ -426,6 +434,7 @@ addToLibrary({
},

_wasmfs_opfs_set_size_file__i53abi: true,
_wasmfs_opfs_set_size_file__sig: 'vpijp',
_wasmfs_opfs_set_size_file__deps: ['$wasmfsOPFSFileHandles', '$wasmfsOPFSProxyFinish'],
_wasmfs_opfs_set_size_file: async function(ctx, fileID, size, errPtr) {
let fileHandle = wasmfsOPFSFileHandles.get(fileID);
Expand Down
2 changes: 1 addition & 1 deletion system/lib/wasmfs/backends/opfs_backend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class OPFSFile : public DataFile {
});
break;
case OpenState::Blob:
proxy([&]() { size = _wasmfs_opfs_get_size_blob(state.getBlobID()); });
proxy([&]() { _wasmfs_opfs_get_size_blob(state.getBlobID(), &size); });
break;
default:
WASMFS_UNREACHABLE("Unexpected open state");
Expand Down
9 changes: 4 additions & 5 deletions system/lib/wasmfs/backends/opfs_backend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,28 +67,27 @@ void _wasmfs_opfs_free_directory(int dir_id);
int _wasmfs_opfs_read_access(int access_id,
uint8_t* buf,
uint32_t len,
uint32_t pos);
off_t pos);

int _wasmfs_opfs_read_blob(em_proxying_ctx* ctx,
int blob_id,
uint8_t* buf,
uint32_t len,
uint32_t pos,
off_t pos,
int32_t* nread);

// Synchronous write. Return the number of bytes written.
int _wasmfs_opfs_write_access(int access_id,
const uint8_t* buf,
uint32_t len,
uint32_t pos);
off_t pos);

// Get the size via an AccessHandle.
void _wasmfs_opfs_get_size_access(em_proxying_ctx* ctx,
int access_id,
off_t* size);

// 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't off_t be used as a return type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you mark this function as i53abi and j is the return type it should "just work".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know. I'll keep that in mind for future updates.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this part of this change? No need to the out parameter then a simple return type will do (IMHO).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this part of this change? No need to the out parameter then a simple return type will do (IMHO).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've change it to return the size directly. Seems to work fine.


// Get the size of a file handle via a File Blob.
void _wasmfs_opfs_get_size_file(em_proxying_ctx* ctx, int file_id, off_t* size);
Expand Down
Loading