feat(runtime/worker): Structured cloning worker message passing. #9323

inteon · 2021-01-29T18:45:00Z

Refactored webworkers to listen for host messages via an "async bin op" instead of directly injecting JSON into the runtime.

Fixes #3557.

CLAassistant · 2021-01-29T18:45:05Z

All committers have signed the CLA.

bartlomieju · 2021-02-02T17:14:37Z

Refactored webworkers to listen for host messages via an "async json op" instead of directly injecting JSON into the runtime.
First steps towards #3557, main goal is to change the message passing, so instead of JSON SharedArrayBuffers can be used (not yet part of this pull request).
[WIP] because a waker still has to be re-added to poll_event_loop
↳ need some more time to figure out how this works (feel free to help)

Would be nice if some intermediate feedback could be provided already, on both the approach and the current code.

Thanks for the PR, but the current design is on purpose. I'd rather not change code before there's a working prototype of structured cloned message passing and currently we're still missing appropriate bindings in deno_core to do that (which is in my backlog).

NOTE: code also contains some unrelated fixes for linting

It's not a good idea to throw such unrelated changes into a PR. Besides current formatting and linting passes so I'm not sure how that's better.

inteon · 2021-02-02T21:16:19Z

Refactored webworkers to listen for host messages via an "async json op" instead of directly injecting JSON into the runtime.
First steps towards #3557, main goal is to change the message passing, so instead of JSON SharedArrayBuffers can be used (not yet part of this pull request).
[WIP] because a waker still has to be re-added to poll_event_loop
↳ need some more time to figure out how this works (feel free to help)
Would be nice if some intermediate feedback could be provided already, on both the approach and the current code.

Thanks for the PR, but the current design is on purpose. I'd rather not change code before there's a working prototype of structured cloned message passing and currently we're still missing appropriate bindings in deno_core to do that (which is in my backlog).

NOTE: code also contains some unrelated fixes for linting

It's not a good idea to throw such unrelated changes into a PR. Besides current formatting and linting passes so I'm not sure how that's better.

Thank you for the feedback! I reverted the linting changes. Also, I will change the goal of this PR to create a structured cloning prototype. Like you said, that will make it easier to discuss what can be done better/ differently in my approach. While still making structured cloning work.
Then, the prototype can be used as a starting point to create the needed PRs to implement this feature (probably will involve multiple PRs).

Will update soon.

inteon · 2021-02-02T23:39:40Z

@bartlomieju I pushed a full prototype, please let me know what you think.

bartlomieju

@inteon great work! There's a lot going on in this PR and it might be hard to land it in one go. Changes to core/ look great, there are a few minor comments I have.

I'm yet to properly go through changes to worker implementation; could you open a new PR with only changes to the core/ directory so we can land them first?

core/serialize_deserialize.rs

core/bindings.rs

bartlomieju · 2021-02-24T13:04:25Z

@inteon let me know if this PR is ready, I don't think we'll land it for 1.8 though

inteon · 2021-02-24T15:47:46Z

@inteon let me know if this PR is ready, I don't think we'll land it for 1.8 though

Indeed, let's not rush things. First lets make sure it works well.

runtime/js/01_errors.js

runtime/js/99_main.js

runtime/ops/web_worker.rs

runtime/ops/worker_host.rs

bartlomieju · 2021-04-19T15:13:08Z

@inteon please rebase when you get a moment, let's land this feature for 1.10

inteon · 2021-05-09T20:07:32Z

@bartlomieju
I finished rebasing the structured cloning message passing.
Additionally, the PR includes some worker related cleanup (changed wrong message in error & moved benchmark).
I also revised WebWorkerHandle and WebWorkerInternalHandle to give better separation between cross-tread and same-thread handles.

runtime/ops/web_worker.rs

runtime/web_worker.rs

Co-authored-by: Ben Noordhuis <[email protected]>

bartlomieju

@inteon thank you so much for this PR, it's really great that you managed to implement all of that functionality in so little code. I have only some minor/nitpicky comments, other than that it's ready to land. Great to see this landed for 1.10!

runtime/js/11_workers.js

runtime/js/99_main.js

runtime/ops/web_worker.rs

runtime/ops/worker_host.rs

runtime/web_worker.rs

inteon · 2021-05-10T21:56:37Z

I optimized the worker <-> host message passing ops (removed all places where buffers were copied).
@AaronO I also made some small changes to serde_v8, please check if these are acceptable

bartlomieju

LGTM as well ✅ thanks for seeing this through @inteon, this is a massive feature that was highly requested.

runtime/js/11_workers.js

runtime/js/99_main.js

serde_v8/src/magic/buffer.rs

runtime/js/99_main.js

serde_v8/src/magic/buffer.rs

runtime/web_worker.rs

serde_v8/src/magic/buffer.rs

ry

LGTM - thank you much @inteon !

piscisaureus

LGTM!

…land#9323) This commit upgrade "Worker.postMessage()" implementation to use structured clone algorithm instead of non-spec compliant JSON serialization.

inteon changed the title ~~[WIP] refactor(runtime/worker): Use async json op to receive "Post message" sent from host to worker~~ [WIP] feat(runtime/worker): Structured cloning worker message passing. Feb 2, 2021

bartlomieju reviewed Feb 8, 2021

View reviewed changes

core/serialize_deserialize.rs Outdated Show resolved Hide resolved

core/serialize_deserialize.rs Outdated Show resolved Hide resolved

core/serialize_deserialize.rs Outdated Show resolved Hide resolved

core/bindings.rs Outdated Show resolved Hide resolved

This was referenced Feb 9, 2021

Value returned by "value_deserializer" cannot outlive the "value_deserializer" denoland/rusty_v8#607

Closed

feat: add structured cloning to Deno.core #9458

Merged

Base automatically changed from master to main February 19, 2021 14:59

inteon mentioned this pull request Feb 25, 2021

refactor(deno/core): Move buffer_ops to deno core & merge logic with json_ops #9457

Merged

inteon changed the title ~~[WIP] feat(runtime/worker): Structured cloning worker message passing.~~ feat(runtime/worker): Structured cloning worker message passing. Feb 28, 2021

inteon marked this pull request as ready for review February 28, 2021 17:58

inteon requested a review from bartlomieju March 20, 2021 20:52

bartlomieju reviewed Mar 22, 2021

View reviewed changes

bartlomieju added this to the 1.10.0 milestone Apr 16, 2021

bartlomieju modified the milestones: 1.10.0, 1.11.0 Apr 27, 2021

inteon added 4 commits May 8, 2021 17:54

move bench_large_message and cleanup bench

5ccf764

deserialize wrong error message

b6ce4fb

use structured cloning and improve worker handles

63dea20

fix racy worker

31f5da6

inteon requested a review from bartlomieju May 9, 2021 21:28

Merge branch 'main' into post_message

753296f

bnoordhuis approved these changes May 10, 2021

View reviewed changes

runtime/ops/web_worker.rs Outdated Show resolved Hide resolved

runtime/web_worker.rs Outdated Show resolved Hide resolved

runtime/web_worker.rs Outdated Show resolved Hide resolved

runtime/web_worker.rs Show resolved Hide resolved

inteon and others added 2 commits May 10, 2021 10:55

Apply suggestions from code review

4aa3ffe

Co-authored-by: Ben Noordhuis <[email protected]>

define WorkerId as new struct instead of type alias

676f4ba

added TODO comments

1601459

bartlomieju reviewed May 10, 2021

View reviewed changes

optimize worker <-> host message passing ops

1256524

bartlomieju approved these changes May 10, 2021

View reviewed changes

add comment to op_worker_unhandled_error

921f354

ry reviewed May 11, 2021

View reviewed changes

piscisaureus reviewed May 11, 2021

View reviewed changes

runtime/js/99_main.js Outdated Show resolved Hide resolved

piscisaureus reviewed May 11, 2021

View reviewed changes

serde_v8/src/magic/buffer.rs Outdated Show resolved Hide resolved

piscisaureus reviewed May 11, 2021

View reviewed changes

runtime/web_worker.rs Outdated Show resolved Hide resolved

piscisaureus reviewed May 11, 2021

View reviewed changes

serde_v8/src/magic/buffer.rs Outdated Show resolved Hide resolved

review comments

e95db55

ry approved these changes May 11, 2021

View reviewed changes

piscisaureus approved these changes May 11, 2021

View reviewed changes

piscisaureus and others added 3 commits May 11, 2021 18:14

nit

db000ce

fix buf copy

0e7705d

added postMessage structured cloning regression test

ba897c2

bartlomieju merged commit 635253b into denoland:main May 11, 2021

inteon deleted the post_message branch March 4, 2022 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(runtime/worker): Structured cloning worker message passing. #9323

feat(runtime/worker): Structured cloning worker message passing. #9323

inteon commented Jan 29, 2021 •

edited

Loading

CLAassistant commented Jan 29, 2021 •

edited

Loading

bartlomieju commented Feb 2, 2021

inteon commented Feb 2, 2021

inteon commented Feb 2, 2021 •

edited

Loading

bartlomieju left a comment

bartlomieju commented Feb 24, 2021

inteon commented Feb 24, 2021

bartlomieju commented Apr 19, 2021

inteon commented May 9, 2021 •

edited

Loading

bartlomieju left a comment

inteon commented May 10, 2021

bartlomieju left a comment

ry left a comment

piscisaureus left a comment

feat(runtime/worker): Structured cloning worker message passing. #9323

feat(runtime/worker): Structured cloning worker message passing. #9323

Conversation

inteon commented Jan 29, 2021 • edited Loading

CLAassistant commented Jan 29, 2021 • edited Loading

bartlomieju commented Feb 2, 2021

inteon commented Feb 2, 2021

inteon commented Feb 2, 2021 • edited Loading

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju commented Feb 24, 2021

inteon commented Feb 24, 2021

bartlomieju commented Apr 19, 2021

inteon commented May 9, 2021 • edited Loading

bartlomieju left a comment

Choose a reason for hiding this comment

inteon commented May 10, 2021

bartlomieju left a comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

piscisaureus left a comment

Choose a reason for hiding this comment

inteon commented Jan 29, 2021 •

edited

Loading

CLAassistant commented Jan 29, 2021 •

edited

Loading

inteon commented Feb 2, 2021 •

edited

Loading

inteon commented May 9, 2021 •

edited

Loading