[WasmFS] Add JSImplBackend #16209

kripken · 2022-02-04T23:18:21Z

This refactors the existing JSFileBackend to use a new model for creating JS
backends. A JSImplBackend is defined, which adds a layer of indirection in
JS, allowing a backend to be implemented almost 100% in JS. That is,
JSImplBackend is a new backend that forwards operations to an implemenation
in JS.

See full docs in js_impl_backend.h. Note that the diffs here may not be as
useful as reading the files in their final state. In particular, look at the how the
JSFileBackend is now implemented in a minimal way in C++ in
js_file_backend.cpp.

This is a step towards it being practical to make backends like Node and
OPFS.

None of the actual JSFileBackend logic is changed, just moved around.

kripken · 2022-02-05T00:46:02Z

This hits a minor but interesting issue. The JSImplFile does getBackend() to find out the backend, which it then informs JS about. But in the case of a ProxiedBackend on top of a JSImplFile, the actual backend is the ProxiedBackend - so the backend ID does not match what the JSBackend expects. I guess files will need to store their "immediate" backend, but another way might be to send JS the entire chain of backends, if there's a use case for that..?

tlively · 2022-02-04T23:56:14Z

src/library_wasmfs.js

+#if ASSERTIONS
+    assert(wasmFS$backends[backend]);
+#endif
+    return wasmFS$backends[backend].constructor(file);


We will probably need to differentiate DataFile constructors and destructors from those of Directory and Symlink.

Sounds good. So we'll want something like [new|delete][File|Directory|Symlink]? And then a JS backend would implement just the ones it needs to just newFile/deleteFile for a basic JS File.

Thoughts on new/delete? This is about the lifecycle of the C++ File object, so I think new/delete might make sense. But could also be alloc/free or something else. free might be less likely to be confused with removing a file from the actual filesystem which delete might...

I wonder if we want to expose the lifecycle of a conceptual file rather than the underlying C++ object. So instead of a destructor, maybe we should expose onUnlink, onClose, and onDelete. The C++ destructor would call onDelete only if the file is actually being deleted and not when the program is shutting down.

But a problem with that is that backends may actually want to know when the underlying C++ object is being destructed, perhaps because they own other C++ objects that should also be destructed at that point. So maybe we need a destructor hook in addition to the file lifecycle hooks? In that case, it is probably sufficient for this PR to just have the destructor hook.

In that case, I would want to avoid using delete to describe the destructor hook, since it would be ambiguous whether delete referred to the in-memory resources or the file itself, as you mentioned.

newFile/freeFile or createFile/freeFile or constructFile/destructFile might be good names. I don't feel strongly, except that I would avoid delete.

Makes sense. How about allocFile/freeFile?

src/library_wasmfs.js

src/library_wasmfs_js_file.js

tlively · 2022-02-05T01:04:09Z

src/library_wasmfs_js_file.js

+    '$wasmFS$backends',
+    '$wasmFS$JSMemoryFiles',
+  ],
+  _wasmfs_backend_add_js_file: function(backend) {


Should this be add_js_backend rather than add_js_file?

I was trying to be consistent, wasmfs_backend_* are APIs to add various backends. This backend is called the "js file backend".

Hmm, maybe we should change the name of the backend, or maybe change the naming convention. _wasmfs_add_js_file_backend would be much clearer that we are adding a backend rather than a File.

How about wasmfs_backend_add_jsfile? The lack of a _ makes it clear that "jsfile" is a "thing".

Yeah, that seems ok as well.

Actually, this is the internal API, so we might as well change it as much as we want I guess. Also we have a public API wasmfs_create_js_file_backend so I'll make it more similar to that, and follow the convention of duplicating the public C API + adding _js to the end.

tlively · 2022-02-05T01:11:31Z

system/lib/wasmfs/js_file_backend.cpp

 extern "C" backend_t wasmfs_create_js_file_backend() {
-  return wasmFS.addBackend(std::make_unique<JSFileBackend>());
+  backend_t backend = wasmFS.addBackend(std::make_unique<JSImplBackend>());
+  _wasmfs_backend_add_js_file(backend);


One way to get this to be executed on the correct thread would be to call it lazily the first time a backend operation is called, since those operations will be proxied to the worker thread.

An even better way might be to have the proxying backend constructor take a function that creates a backend as its argument rather than having it take a handle to a backend that has already been created. Then the proxying backend constructor can proxy the creation of the underlying backend to the worker thread and the underlying backend can perform initialization in its constructor, as I would have expected.

No matter what we do there, this call should be somewhere in the JSImplBackend so that the C++ API (even if it's private) is able to fully initialize the backend without needing to be called through the C wrapper.

Interesting, I like the idea for making the proxied backend get a constructor function. But how would we do that in the C API? Literally a backend_t (*)()..? That seems unoptimal but I'm not sure how else.

tlively · 2022-02-05T01:33:49Z

system/lib/wasmfs/js_impl_backend.h

+//  2. Add a cpp file for the new backend, and implement the C function from 1,
+//     which should create it on both the C++ (using JSImplBackend) and JS
+//     sides.


The C layer should be a thin wrapper around the underlying C++ and should not perform any important initialization itself.

We can also improve this workflow so that JS backends can be defined completely in JS userspace with no need to modify the WasmFS source. All we need is a JS API function that creates an instance of the JSImplBackend and allows users to install methods on it and perform whatever other instantiation they want, all from JS. The JSImplBackend constructor should probably install the backend on the JS side then call a user-installable JS function to perform custom initialization.

Adding a small C wrapper to emscripten/wasmfs.h can be a step on top of that workflow for Emscripten developers creating "official" backends, so I would expect to see that documented as an optional last step rather than as the first step.

The C layer should be a thin wrapper around the underlying C++ and should not perform any important initialization itself.

Added a comment now, thanks.

We can also improve this workflow so that JS backends can be defined completely in JS userspace with no need to modify the WasmFS source.

Yes, I think that can work. But it does mean a forced export which increases code size, both of the backend-generating code itself, and also the JSImplBackend itself. Those would end up bundled in all builds (unless metadce gets lucky and manages to remove parts). But I agree there are nice benefits to a pure JS approach. Perhaps that should be an option, say if the full JS API is enabled (FORCE_FILESYSTEM) - if we go that route, then user JS backends would need no C but perhaps our own would still have C files (so that they work without FORCE_FILESYSTEM).

Perhaps that should be an option, say if the full JS API is enabled (FORCE_FILESYSTEM) - if we go that route, then user JS backends would need no C but perhaps our own would still have C files (so that they work without FORCE_FILESYSTEM).

That makes sense to me.

kripken · 2022-02-09T19:39:44Z

Any remaining comments here @tlively ?

sbc100 · 2022-02-10T00:43:28Z

src/library_wasmfs_js_file.js

+  ],
+  _wasmfs_create_js_file_backend_js: function(backend) {
+    wasmFS$backends[backend] = {
+      alloc_file: function(file) {


You can use arrow function for this type of thing these days.

Ah, thanks, and looks like that's possible with async functions too. Added to #16229

kripken added 12 commits February 4, 2022 12:36

start

36bb44f

work [ci skip]

91326ec

work [ci skip]

91bf512

builds [ci skip]

11f82c7

test passes [ci skip]

fabf251

fix

09aa942

fix

0df35c1

comment

5a0653a

better

17c041f

format

cb4c42f

fix

5fa4f25

comments

650efab

kripken requested review from sbc100 and tlively February 4, 2022 23:18

tlively reviewed Feb 5, 2022

View reviewed changes

kripken added 5 commits February 7, 2022 13:04

comment [ci skip]

c80a891

Merge remote-tracking branch 'origin/main' into wfjsbs

a49a630

Merge remote-tracking branch 'origin/main' into wfjsbs

6d9e223

rename

e3c9012

rename

9a4d052

tlively approved these changes Feb 9, 2022

View reviewed changes

kripken merged commit 22b499d into main Feb 9, 2022

kripken deleted the wfjsbs branch February 9, 2022 23:24

sbc100 reviewed Feb 10, 2022

View reviewed changes

kripken mentioned this pull request Feb 16, 2022

Draft: Add support for read/seek on files in the native file system #16307

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WasmFS] Add JSImplBackend #16209

[WasmFS] Add JSImplBackend #16209

kripken commented Feb 4, 2022

kripken commented Feb 5, 2022

tlively Feb 4, 2022

kripken Feb 7, 2022

tlively Feb 7, 2022

kripken Feb 8, 2022

tlively Feb 8, 2022

tlively Feb 5, 2022

kripken Feb 7, 2022

tlively Feb 7, 2022

kripken Feb 8, 2022

tlively Feb 8, 2022

kripken Feb 8, 2022

tlively Feb 5, 2022

kripken Feb 7, 2022

tlively Feb 5, 2022

kripken Feb 7, 2022

tlively Feb 7, 2022

kripken commented Feb 9, 2022

sbc100 Feb 10, 2022

kripken Feb 10, 2022

[WasmFS] Add JSImplBackend #16209

[WasmFS] Add JSImplBackend #16209

Conversation

kripken commented Feb 4, 2022

kripken commented Feb 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment