josdejong / workerpool

Offload tasks to a pool of workers on node.js and in the browser
Apache License 2.0
2.06k stars 147 forks source link

feat: support transferable objects #374

Closed Michsior14 closed 1 year ago

Michsior14 commented 1 year ago

Adds transferable objects support based on ideas from #3.

Resolves #3

josdejong commented 1 year ago

Ow that is great! Thanks Michał, I will review your PR soon but need some time for that.

josdejong commented 1 year ago

Thanks for your PR @Michsior14 , it looks really solid and works like a charm. It looks well tested and documented.

One thought: the current implementation is really close to the Transferable objects API. So it looks like:

// send from main to worker
pool.exec("transferArray", [array], { transfer: [array.buffer] })
pool.exec("transferArrays", [array1, array2], { transfer: [array1.buffer, array2.buffer] })

// send from worker to main
function transferBack(array) {
  return new workerpool.Transfer({ array, byteLength: array.byteLength }, [array.buffer])
}

I was thinking, would it be possible to create a utility function to write this in a more concise way? Instead of (or alongside) the Transfer helper class. It would be nice if we could write something like the following, for both sending and receiving a transferable object?

// send from main to worker
pool.exec("transferArray", [workerpool.transfer(array)])
pool.exec("transferArrays", [workerpool.transfer(array1), workerpool.transfer(array2)])

// send from worker to main
function transferBack(array) {
  return workerpool.transfer(array)
}

That would be a cleaner API: working consistent in two directions, and removing the need to refer to the same array mulitple times in a single request. What do you think?

Michsior14 commented 1 year ago

@josdejong That would work for arrays, but how about others?

Additionally:

  1. We would need to iterate over objects/arrays to support nested transfer that might be a bit tricky.
  2. With the transfer, you won't be able to send from worker an object with both transferable and not transferable properties.
josdejong commented 1 year ago
  1. Hm, yeah. So what we would try to do is automatically determine the transferList based on the function arguments. I do not have much experience with it these other transferable object types, but I understand they require different handling when providing them in the transferList? Is that a matter of one line of code for every of the 10 different types (that could still be acceptable), or is much more involved?
  2. I'm not sure if I understand what you mean, I would think something like the following should be possible?

    function transferBack(array) {
     return {
       transferredArray: workerpool.transfer(array),
       otherData: { message: 'hello world' }
     }
    }
Michsior14 commented 1 year ago
  1. Yes, they all behaves differently. I don't have much experience with others (except MessagePort / OffscreenCanvas) as well. What I don't like here is that we would need to maintain this list for the foreseeable future, but we could I guess always allow both transfer function and Transfer class to mitigate the problem.

  2. Only if the deep array / objects traversal is implemented.

josdejong commented 1 year ago

Yes agree, it will definitely be more work to maintain all of it.

OK let's go for your current approach. In a later stage we can always have an other look at a higher-level utility function to make transfer easier if there is need for it. I will merge your PR now.

josdejong commented 1 year ago

Published now in v6.4.0, thanks again Michal!