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

change: Rust APIの脱Tokioと、voicevox_core::{tokiononblocking} #831

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Sep 12, 2024

内容

題名の通りです。

#830 (comment)についてはTODOとして残し、脱TokioとパブリックAPIのリネームに絞っています。

関連 Issue

ref #388

その他

@qryxip qryxip changed the title change: Rust APIの脱Tokioを行い、モジュール名をtokio:nonblocking change: Rust APIの脱Tokioを行い、モジュール名をtokiononblocking Sep 12, 2024
@qryxip qryxip changed the title change: Rust APIの脱Tokioを行い、モジュール名をtokiononblocking change: Rust APIの脱Tokioを行い、モジュール名をtokiononblocking Sep 12, 2024
@qryxip qryxip changed the title change: Rust APIの脱Tokioを行い、モジュール名をtokiononblocking change: Rust APIの脱Tokioと、voicevox_core::tokio〃::nonblocking Sep 12, 2024
@qryxip qryxip requested a review from Hiroshiba September 12, 2024 16:01
@qryxip qryxip changed the title change: Rust APIの脱Tokioと、voicevox_core::tokio〃::nonblocking change: Rust APIの脱Tokioと、voicevox_core::{tokiononblocking} Sep 12, 2024
@Hiroshiba
Copy link
Member

Hiroshiba commented Sep 13, 2024

なぜtokioを脱したいのかのwhyをこのプルリクエストに関連付けておくと、後から見返すときとか後から入ってきた方に優しいかもと思いました!
(tokioがマイナーなら問題ないけど、確か結構デファクトなんでしたっけ)

tokioではなくnonblockingだとランタイムが不要で、wasmなどの特殊な環境でもブロッキングできるようになるから、とかでしたっけ。

どこかで理由のコメントがあったと思いますが、そこの URL とかで良さそう!

Comment on lines +5 to +6
//! これらは[blocking]クレートにより動いている。特定の非同期ランタイムを必要とせず、[pollster]など
//! でも動かすことができる。
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

・・・・・・・あれ、これどういうことでしたっけ。。。

blokingモジュール(クレート)があって、これはAsyncを実装してくれてる感じ・・・?
(プレーンなRustのasyncは非同期の機能がなく、非同期用のクレートを使わないと非同期にできない、という理解でいます)

blocking、名前めちゃくちゃややこしいですね・・・。

Copy link
Member Author

@qryxip qryxip Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そうですね。こんな感じです。

  • blocking: 外部ライブラリ名。VOICEVOX COREのパブリックAPIとしてはドキュメントのみ登場
  • voicevox_core::blocking: パブリックAPIの一部
  • voicevox_core::nonblocking: パブリックAPIの一部。内部でblocking(外部ライブラリ名)を使っている

blocking、名前めちゃくちゃややこしいですね・・・。

ですね。

前身としてunblockという、unblock::unblock(…)という形で使えるクレートがあって更新が途絶えているのですが、よく見たらこれblockingよりに誕生したものですね。なんでこの名前にしなかったんだろう…

2回くらい言ったかもしれませんが、せめてリポジトリ名(smol-rs/blocking)がblocking-rsとかだったらそっちで呼べるのに…

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!!

コード的には問題なしなので、任意のタイミングでマージしていただければ!!
一部質問コメントだけしました!

@qryxip
Copy link
Member Author

qryxip commented Sep 13, 2024

なぜtokioを脱したいのかのwhyをこのプルリクエストに関連付けておくと、後から見返すときとか後から入ってきた方に優しいかもと思いました! (tokioがマイナーなら問題ないけど、確か結構デファクトなんでしたっけ)

コードのドキュメントとしては脱Tokioのメリットを書きましたが、GitHubではどこにも書いてませんでしたね。ちょうど今 VOICEVOX/voicevox_project#56 で話している「PRの結論」を、why込みで書いてみました。

Tokioに依存したプログラムは、async-stdやsmolで使うことはできない。一方、
現在のVOICEVOX CORE Rust APIのTokio依存部分は
`tokio::task::spawn_blocking`のみである。そのため
`tokio::task::spawn_blocking`を、同等の機能を持つblockingクレートのも
のに置き換えることでRust APIの"脱Tokio"を行う。

https://docs.rs/crate/blocking

またこの"脱Tokio"に伴い、Rust APIの`voicevox_core::tokio`を
`voicevox_core::nonblocking`にリネームする。

Python APIではpyo3-asyncioが現在Tokio版かasync-std版しかない状態なので、
Tokioに依存した状態のままにしてある。またtest_utilやdownloaderではこれま
で通りreqwestに依存する。

また将来`Synthesizer`や`OpenJtalk`なども`trait Async`をベースにした設計
にすることを考えているが、本PRではTODOコメントを残すのみにしてある。

https://github.com/VOICEVOX/voicevox_core/pull/830#discussion_r1750081919

tokioではなくnonblockingだとランタイムが不要で、wasmなどの特殊な環境でもブロッキングできるようになるから、とかでしたっけ。

WASMのためではないですね。WASMだとスレッドプールすら使えないので、 #830 ではSingleTaskedという名前を絞り出してまで別実装を用意した、という感じでした。

@qryxip qryxip merged commit b8118b9 into VOICEVOX:main Sep 13, 2024
35 checks passed
@qryxip qryxip deleted the change-tokio-to-blocking branch September 13, 2024 17:52
@qryxip qryxip mentioned this pull request Oct 1, 2024
3 tasks
qryxip added a commit that referenced this pull request Nov 2, 2024
#831 で残した次のTODOのうち、`Synthesizer`についてだけ解決する。

````rs
// TODO: `VoiceModelFile`のように、次のような設計にする。
//
// ```
// pub(crate) mod blocking {
//     pub struct Synthesizer(Inner<SingleTasked>);
//     // …
// }
// pub(crate) mod nonblocking {
//     pub struct Synthesizer(Inner<BlockingThreadPool>);
//     // …
// }
// ```
````

目的としては、 #687 のようなことを行うのを円滑にするため。

もしかしたらパフォーマンスには影響が出ているかもしれないが、そこまで大き
なものではないはず。

Refs: #831, #687
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.

2 participants