WASM support

[dakom]

What can be improved or is missing?

wasm32-unknown-unknown target

Implementation Approach

at a glance, I think the only requirement is to make threadpool/multithreading optional. It can be enabled by default, but then wasm builds can opt out of it

[dakom]

A use case would be for rust-powered wasm+webgl (and eventually webgpu) renderers in browsers

[johannesvollmer]

hi! parallelism in this library is optional at runtime. that should be enough to build for WASM. see #148

what problems did you encounter when building for WASM?

to gain more confidence, we could add some tests to check for WASM target

[dakom]

oh, sorry, opened the PR before seeing the response here

unfortunately it's a runtime error, I'm not sure how to demonstrate that easily without spinning up a public repo / proof of concept, and even then you'd have to kinda look at the browser logs

but it happens to be that this issue from a totally unrelated image processing crate has a good report, and you can see it's the same sortof situation (need a way to disable the multithreading crate): image-rs/image#1496

[johannesvollmer]

well, when running in WASM, you will need to explicitly disable the multi threading in the reader. by default, it will use multiple threads. but it's just one line more to make it single threaded

[dakom]

btw with the changes in #201 I am able to successfully load and display an image in browser :)

[dakom]

ah... but you're right, I can also just call .non_parallel() 😅

so - that PR is not an absolute requirement for WASM... but, it's nice to be able to opt-out of a dependency I'm not using too

[johannesvollmer]

so, to summarize:

• WASM target builds
• WASM fails at runtime when attempting to use multiple threads
• a feature gate can statically remove thread pools (but only if you don't forget to disable the flag), falling back to non-parallel code
• WASM runtime succeeds when using non-parallel code (when not forgetting to switch the runtime flag)

I'm thinking if there is a way for the library to automatically fall back to single threaded code on WASM, without the caller having to change anything?

is it a panic or an error on runtime in WASM?

maybe the thread pool library will return an error object which we must simply react to. alternatively we could use #[cfg(target_arch = "wasm32")], but it feels like a less clean approach, because there might be other platforms without threads

[dakom]

yup, I think that's accurate.

I believe it's a runtime panic in the threadpool library, basically:

1. exrs/src/block/reader.rs

   Line 244 in 66a2477

   .build();

2. https://github.com/rust-threadpool/rust-threadpool/blob/a9dd15c278a8ab07d694efb169348138823bb89d/src/lib.rs#L777

but in general, I think it's cleaner to feature gate this... fwiw I don't have a strong opinion among these ideas:

• gate by architecture
• gate by threadpool only
• gate by parallel (the current PR)

however, my weak opnion: I agree with your assessment that there might be other platforms without native threads. The reason I split the "parallel" feature vs. "threadpool" is maybe premature... I'm thinking that wasm32 actually does support threads through webworkers, but as of right now it can't use the same underlying std::thread stuff that threadpool uses, and so in theory it's possible that a future improvement could be to change what the "parallel" feature means depending on the platform - native will use threadpool/rayon, and wasm32 would use some other thing.... but this is maybe a bit too premature and it would make more sense to just gate on whether or not threadpool is enabled. lmk if you want me to change that.

[johannesvollmer]

i think the cleanest approach is to attempt building the threadpool, and if it fails, run sequential decompression, like this:

return match thread_pool_builder.try_build() {
   Ok(pool) => decompress_parallel(pool, ...),
   Err(_) => decompress_sequential(...)
}

this code is resilient, small, does not increase complexity of the codebase or the crate api, and it will work regardless of the the reason for multi threading not being available. also, a similar mechanism is already in place, because parallelism will only be used if there is any compression in the file. otherwise sequential reading will be chosen anyways.

the only question is, does it actually do what we want? or will wasm still panic for a different reason in the threading dependency?

[johannesvollmer]

note, we can still add the threading feature, maybe for another reason? i don't know why though.

keep in mind that it increases the complexity of the code base and for the users. it should be worth the trouble.

[dakom]

I don't see try_build in threadpool ?

As a user, I'd be surprised if I used the parallel option and it silently switched to sequential. If there is a way to capture the error here (without something weird like panic hook), I'd suggest that the builder should bubble up the error, then the user can decide if they want to try again with sequential.

[dakom]

that also kinda speaks to the wasm story... it wouldn't be completely crazy to do something like this (pseudocode-ish):

if let Err(err) = try_build_parallel() {
  if err == Error::MultithreadingFailed {
    // somehow split the sequential work between WebWorkers
  }
}

I'm not familiar enough with the EXR format or library to know if that's possible, like to actually chunk the raw bytes and send it to different sequential builders on different "threads", but I think bubbling up the error and letting the user decide is better for niche ideas like this

With that in place - I think my PR would be refactored similarly, it would return an error if parallel feature isn't enabled and any parallel paths are hit. In other words there would be two ways the user would get this error:

1. They have the parallel feature enabled (i.e. default), they try to create a parallel builder, and it fails due to third-party errors
2. They do not have the parallel feature enabled and they try to create a parallel builder

[johannesvollmer]

I don't see try_build in threadpool ?

we're thinking about switching to rayon-core for other reasons, which has this functionality

As a user, I'd be surprised if I used the parallel option and it silently switched to sequential.

what are the reasons you want to know whether multi threading happened?

in this library, multi threading is an optimization, which makes things go fast, given the hardware supports it. if you want to load an image, but a certain optimization is not available, you'd still want the image, even if it will take a little longer.

what if some rust project is ported to wasm, and they only test uncompressed images, and they think it's works. in production then, someone wants to use a compressed image, and it fails, even though the library could perfectly return the expected image (only slower than what we know is theoretically possible).

the only reason to control mulit threading I can think of, is to disable it, because your threads are already occupied with a different task, or you don't want to use up the hardware resources for a different reason.

if read_image().parallel().from_file(...) suggest some kind of guarantee to use multiple threads, then we should definitely rename that function. maybe try_parallel or similar

[dakom]

what are the reasons you want to know whether multi threading happened?

Maybe I have two images - a high quality EXR and a low quality JPG. If multithreading fails, I want to fall back to the low quality JPG because I'm choosing the tradeoff of speed over quality.

Totally made-up example, but generally I feel like calling parallel() should always do multithreading, and either panic or error if it can't (I'd expect parallel() to panic, and try_parallel() to return an error, if both are available - but if only parallel() is available I'd prefer it to error instead of panic)

Your decision though - just sharing my opinion, I don't feel strongly about it :)

[johannesvollmer]

Maybe I have two images - a high quality EXR and a low quality JPG. If multithreading fails, I want to fall back to the low quality JPG because I'm choosing the tradeoff of speed over quality.

Interesting, indeed. But the problem is, multi threading is only one of many optimizations that affect the speed of image loading, and it's not a reliable indicator. If you want to avoid too long load times, try to load the image for 2s, then after that elapsed, abort. Using the availability of multi threading does not say anything about the speed of loading a particular image, some images may be 50KB, and others may be 50GB. This even applies when the images are compressed to the same size on disk. The only way to know is to actually measure time while trying to load an image. Paralellization an optimization that should not be relied upon, just as you should not rely on the compiler to perform loop-unrolling.

Also, if you have an uncompressed image (meaning multi threaded decompression is not needed), but creating a threadpool would fail, should the library still report an error, or decode the image sequentially?

I can't find any convincing arguments to return an error... to me, fallback to sequential decoding still seems like the best option.
Of course this doesn't mean it's decided forever. If someone arrives with a particular real-world use case that requires returning an error, it should be reconsidered.

I'd expect parallel() to panic, and try_parallel() to return an error

Yes, I agree. Maybe we should rename the methods to allow_parallel & forbid_parallel instead? Would that make it clear? I think it could successfully imply that parallelization cannot be forced.