Skip to content
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

fix: transferrable not being transferred #155

Merged
merged 4 commits into from
Dec 2, 2021
Merged

Conversation

Deivu
Copy link
Contributor

@Deivu Deivu commented Oct 19, 2021

I noticed that returning a Transferable objects from a worker doesn't get the object transferred, instead it's being cloned.

To verify this, I created my own Implementation of Transferrable

class Shared {
    constructor({ ok, time, data } = {}) {
        this.ok = ok;
        this.time = time;
        this.data = data;
    }

    get [Piscina.transferableSymbol]() {
        return [this.data];
    }

    get [Piscina.valueSymbol]() {
        return { ok: this.ok, time: this.time, data: this.data };
    }

    make() {
        return Piscina.move(this);
    }
}

module.exports = Shared;

Then logged my buffer's byteLength intially, then after 1 second.

Without this fix, the byteLength didn't change, which signifies the buffer was not transferred
image
With the fix, the byteLength did change, which signifies the buffer was transferred
image

@addaleax
Copy link
Collaborator

@Deivu Is this something that you could add a test for?

@Deivu
Copy link
Contributor Author

Deivu commented Oct 20, 2021

@addaleax Took me some time to get the test working, but I did manage to add a test for it

@arielszn
Copy link

Hey, we tried using the Piscina.move() feature for transferring a big object we need to deserialise on a worker thread, and then again a Piscina.move() from the worker thread to transfer back the deserialised object to the main thread, but this is not impacting on memory usage and latency as we were expecting for this change, we think it's not behaving as expected because of the issue that @Deivu is solving here.
Do you plan to merge this soon?

@jasnell
Copy link
Collaborator

jasnell commented Nov 12, 2021

Hmm looks like we've got a CI problem. I'll have to investigate that this weekend

@sogame
Copy link

sogame commented Nov 18, 2021

Hi @jasnell, I'm also interested in this fix for my project, any luck with the CI issues? 😬

@arielszn
Copy link

Hey, we tried using the Piscina.move() feature for transferring a big object we need to deserialise on a worker thread, and then again a Piscina.move() from the worker thread to transfer back the deserialised object to the main thread, but this is not impacting on memory usage and latency as we were expecting for this change, we think it's not behaving as expected because of the issue that @Deivu is solving here. Do you plan to merge this soon?

Hey, any news over here :) ?

@nicholas-l
Copy link
Contributor

Hey all, I have identified the issue with CI and have created a quick fix: #161

@sogame
Copy link

sogame commented Nov 29, 2021

Hi @Deivu, now that the CI issue has been solved (in #161), could you update your branch with master, so the new build is triggered? 😇

@sogame
Copy link

sogame commented Dec 2, 2021

Hi @jasnell would it be possible to get this fix merged (and even release a new version of piscina with it), please? 😇
Really looking forward to test my project with this improvement 🚀

@jasnell
Copy link
Collaborator

jasnell commented Dec 2, 2021

Running CI now, let's see where that lands first!

@jasnell jasnell merged commit e9056ed into piscinajs:current Dec 2, 2021
@jasnell
Copy link
Collaborator

jasnell commented Dec 2, 2021

merged! I'll look at cutting a new release tomorrow afternoon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants