-
Notifications
You must be signed in to change notification settings - Fork 12
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
Worker bundle #70
base: main
Are you sure you want to change the base?
Worker bundle #70
Conversation
…r bytes object sent, and plugin loader
src/worker_proxy.ts
Outdated
type ACCESS_MODESTRING = keyof typeof ACCESS_MODES; | ||
|
||
const worker = new DedicatedWorker(); // new Worker('./worker.js'); | ||
const remote = Comlink.wrap(worker) as Comlink.Remote<typeof api>; |
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.
Instead of exporting the api
object, have you tried exporting its type?
export type WorkerApi = typeof api;
This might help remove the need for the two files, lib_worker
and h5wasm.worker.ts
? 🤷
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.
Ha! ...this is why it's great to have a TypeScript pro review your code...
src/lib_worker.ts
Outdated
|
||
async function save_to_workerfs(file: File) { | ||
const { FS, WORKERFS, mount } = await workerfs_promise; | ||
const { name: filename, size } = file; |
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.
In H5Web, I generate a random ID with nanoid
and use it as the filename when writing to Emscripten's file system. This removes any issue/complexity with parsing paths or dealing with conflicting filenames.
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.
It's a good idea. nanoid
has the right number of dependencies.
return await this.proxy.paths(); | ||
} | ||
|
||
async get(name: string = "/") { |
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.
OK, I think I understand, you have to basically reimplement your own Group#get
method that returns proxied entity objects because the objects returned by the original method loose all their methods.
The downside I see is that we're basically exposing a Group
class from the worker that doesn't quite work as designed. Also, when we get an entity at a given path, we end up either with a "native" h5wasm entity (Dataset
, Datatype
, etc.) or with a GroupProxy
, which might be confusing.
I have an idea for a slightly different approach:
- Instead of the "native" h5wasm
Group
,Dataset
, etc. classes, expose superclasses that forbid calling all unsupported methods, likeGroup#get
. - Provide a functional utility, maybe
get_entity(path)
that implements the same logic asGroup#get
(including callingModule#get_type
,Module#get_external_link
, etc.) but returns instances of the proxied superclasses.
class WorkerGroup extends Group {
public override get(path: string) {
throw new Error('Method `get` not supported);
}
}
Comlink.expose({
ready: h5wasm.ready,
WorkerGroup
});
export async function get_entity(fileId: string, entityPath: string): Promise<WorkerGroup | WorkerDataset | ...> {
const module = await remote.ready;
const kind = await module.get_type(fileId, entityPath);
switch (kind) {
case "Group":
return new remote.WorkerGroup(fileId, entityPath);
...
default:
throw new Error(...);
}
}
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.
With this superclass approach, we can also override some methods to return Transferable
values to avoid copying buffers and increase performance.
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 agree that it will be important for performance to refine this so it transfers large objects instead of copying them.
For the Group proxy though, I would hate to lose the ability to use Group.get
... I feel like that's an important part of the current API, particularly when this is being used in an interactive session, where you might want to explore a Group or its children without having to reconstruct the whole HDF path every time (and append to it). I understand the reasons for including a get_entity(<absolute_hdf_path>)
method, but can't we do both?
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.
Fair enough, it's true that they are not incompatible approaches. I guess you could just call get_entity
from GroupProxy#get
and if the result is a WorkerGroup
, wrap it in another GroupProxy
. 👍
This is a derivative product from h5wasm - @axelboc what do you think about making it a separate (but dependent) package (e.g. https://github.com/h5wasm/h5wasm-worker-proxy or something like that?) It would make the packaging easier, and also simplify importing it into another project. |
Good call! |
moved to https://github.com/h5wasm/h5wasm-worker ... maybe this will be the last move :) |
Add a new build target: a bundled interface that accesses
h5wasm
through a Web Worker proxy.Motivation
Differences from standard library
Usage