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

Consider getting rid of tokio #5907

Closed
emilk opened this issue Apr 11, 2024 · 0 comments · Fixed by #6043
Closed

Consider getting rid of tokio #5907

emilk opened this issue Apr 11, 2024 · 0 comments · Fixed by #6043
Assignees
Labels
😤 annoying Something in the UI / SDK is annoying to use 🧑‍💻 dev experience developer experience (excluding CI)

Comments

@emilk
Copy link
Member

emilk commented Apr 11, 2024

We only have a couple of crates using tokio. There are no benefits to it that I can see, but the costs are:

Requires a tokio runtime

We and users need to spawn a tokio runtime, e.g. using #[tokio::main] or tokio::runtime::Builder::new_multi_thread().

Forcing async onto users of the rerun crate is not very nice.

For us this is also annoying - if you forget to create a runtime you get a runtime crash if you call something requiring that runtime. Same for our users: the code may compile and run until they use a specific rerun feature (e.g. calling .serve())

async code is more complicated

And it requires special primitives, e.g. special "async" channels, when normal channels don't suffice.

Additional compilation times

It's not too bad, but is not nothing either:

tokio v1.28.1
├── bytes v1.4.0
├── libc v0.2.150
├── mio v0.8.11
│   ├── libc v0.2.150
│   └── log v0.4.17
│       └── cfg-if v1.0.0
├── num_cpus v1.15.0
│   └── libc v0.2.150
├── pin-project-lite v0.2.13
├── signal-hook-registry v1.4.1
│   └── libc v0.2.150
├── socket2 v0.4.9
│   └── libc v0.2.150
└── tokio-macros v2.1.0 (proc-macro)
    ├── proc-macro2 v1.0.78
    │   └── unicode-ident v1.0.8
    ├── quote v1.0.35
    │   └── proc-macro2 v1.0.78 (*)
    └── syn v2.0.48
        ├── proc-macro2 v1.0.78 (*)
        ├── quote v1.0.35 (*)
        └── unicode-ident v1.0.8
[build-dependencies]
└── autocfg v1.1.0
@emilk emilk added 🧑‍💻 dev experience developer experience (excluding CI) 😤 annoying Something in the UI / SDK is annoying to use labels Apr 11, 2024
@Wumpf Wumpf self-assigned this Apr 16, 2024
Wumpf added a commit that referenced this issue Apr 17, 2024
### What

* Big chunk of #5907

Removes tokio & async code from re_ws_comms. Achieves this by spawning a
thread for listening and another one for relaying messages (the later
was already the case but via tokio).
Main disadvantage of this change here is that all clients now share the
same thread for sending whereas before this was handled by the tokio
pool. In theory we have less latency though since before we had to go
from the broadcast thread to the client worker threads in a separate
step.

Ended up depending on the
[`polling`](https://docs.rs/polling/latest/polling/index.html) crate
because otherwise shutting down a tcplistener with vanilla std rust is
close to impossible. The version I picked is such that Cargo.lock
doesn't have (yet) another instance of this crate.

This change fixes a bug in the process where we'd not send all so far
received messages to new clients.

⚠️ Rerun serve still needs a tokio runtime with this change ⚠️: To fix
this we need to remove it also for the web server which should be quite
a bit easier 🤞

Testing done:
* [x] `pixi run rerun-web <some rrd>` to serve several local browsers
* [x] `pixi run python .\examples\python\face_tracking\main.py --serve`
to serve several local browsers and checking that they catch up on old
data
* [x] keeping an eye on debug output ensuring that shutdown is graceful
* [x] Test the same on Mac (did only windows so far)

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6005?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6005?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6005
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
Wumpf added a commit that referenced this issue Apr 18, 2024
#6030)

### What

* Followup to #6005
* another piece of #5907

Removes tokio dependency from re_web_viewer_server:
* async `WebViewerServer` no longer uses tokio internally, instead use
`future_channel::oneshot` for shutdown
* `WebViewerServerHandle` is now optional (but default enabled) and
spawns a thread that internally uses the tiny pollster crate
* removed `re_web_viewer_server`'s main file - it's not used by any test
and may thus rot
    * this allows us to also remove the clap dependency from this crate


⚠️ Rerun serve/connect still needs a tokio runtime with this change ⚠️:
Likely the last piece after this is `re_sdk_comms` which is very similar
in nature to `re_ws_comms`.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6030?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6030?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6030)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
Wumpf added a commit that referenced this issue Apr 22, 2024
### What

Rust users now no longer need to ensure to have tokio runtime set up
when using `serve`.

* Fixes #5907
* Direct follow-up / merges into #6042

Last PR in a series of PRs for removal of the tokio runtime:
* removes need for tokio in re_sdk_comms
*similar to the refactor in re_ws_comms only in spirit - a lot simpler:
no fancy broadcast, no other libraries involved. Just a bunch of threads
hammering on blocking sockets).
* remove remaining usages of async in this context
* removes tokio need from all documentation

-----

Dependency count `0.15.1` -> last friday `d90ed2f7e`-> `this pr`

* `cargo build -p rerun --no-default-features` 225 -> 274 -> 275
dependencies
* `cargo build -p rerun -F default` 361 -> 364 -> 364 dependencies
* `cargo build -p rerun -F web_viewer` 409 -> 412 -> 374 dependencies

Notes:
* 0.15.1 already regressed quite a bit compared back to January:
#4824
* we currently have a few temporary crates while migrating to re_query2
which weren't present in 0.15

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6043?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6043?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6043)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
😤 annoying Something in the UI / SDK is annoying to use 🧑‍💻 dev experience developer experience (excluding CI)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants