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

IOが発生するメソッドをすべてasync化する #667

Merged
merged 5 commits into from
Nov 23, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Oct 30, 2023

内容

IOを起こす次のメソッドを、SynthesizerVoiceModelの方に合わせてasync化します。

  • UserDict::load
  • UserDict::save
  • OpenJtalk::new_with_initialize OpenJtalk::new
  • OpenJtalk::use_user_dict

#662 という話もありますが、統一性を保つ方を優先すべきかなと思いました。

関連 Issue

ref #545

その他

@qryxip qryxip mentioned this pull request Oct 30, 2023
69 tasks
@Hiroshiba
Copy link
Member

Hiroshiba commented Oct 30, 2023

とりあえず方針としては良いのかなと思いました!
asyncはおそらくマジョリティではないかもしれませんが、統一感を出すのは反対じゃないです!

@qryxip qryxip marked this pull request as ready for review November 21, 2023 17:17
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です!!

たしか以前ちょこっと「async全部やめても良いかも」みたいな話があったと思うのですが、どうしましょう・・・?
個人的には、今後も結構ずっと大変になりそうなのであれば、あえてやらないのも全然ありかなと思います。(同期版を作るのであればその工数もありそう)
逆にもうちょっとでできるのであれば、このまま進んでも良いのかなと思っています!

})?;
Ok(s)

pub async fn new(open_jtalk_dict_dir: impl AsRef<Path>) -> crate::result::Result<Self> {
Copy link
Member

@Hiroshiba Hiroshiba Nov 22, 2023

Choose a reason for hiding this comment

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

newは同期処理を想像する人が多いと思います。個人的には妙な実装だな〜と感じます。must/should/want/canのmustとshouldの間くらいの気持ちです。
new_and_load_dicとかならまあ。

同じくらいの必須感があればこのままでも良いかなと!
Discordで他の人に聞いてもいいかも。

(この話以前結論出てましたっけ、出てたらすみません 🙇 )

Copy link
Member Author

Choose a reason for hiding this comment

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

長文を打っていたら内容が消失したので簡潔にすると、以下のような感じかと思います。

  • Rustに限定すると「newという名前でCPU/IO-boundな処理をするべきではない」という慣習は、私が知る限り無い。fallibleなnewもasyncなnewも世にありふれている。
  • 他の言語だとそのような慣習はあるのか。「狭義のコンストラクタでI/O操作をしたらDIがやりずらくなる」みたいな理由くらいでは?
    • newとかNewとかみたいな名前でIO処理をしている例も結構あるのでは?

考えたり調べたりしてもよくわからなかったので、そのような原則について述べている書籍があれば知りたいと思っています。

Copy link
Member

Choose a reason for hiding this comment

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

直感に反する実装は避けるというプログラミングの原則と、newは同期処理っぽいという自分の直感をもとにコメントした感じでした。

実際C#でasync newを実装するかでめちゃくちゃ議論されてるのを見かけました。
dotnet/csharplang#419
賛成多数だけど賛否両論で、たぶんasync newが許容される世界へ移行途中なのかなと感じました。
(個人的には、使うかどうかは置いといてとりあえず言語には実装したほうが便利なのではと思いました)

まあでも、たぶんユーザーがほしいのはblocking/nonblockingが選べるAPIでしょうし、ここはnewでも
async newでもどちらでも良いかなと思いました!

@qryxip
Copy link
Member Author

qryxip commented Nov 23, 2023

たしか以前ちょこっと「async全部やめても良いかも」みたいな話があったと思うのですが、どうしましょう・・・?
個人的には、今後も結構ずっと大変になりそうなのであれば、あえてやらないのも全然ありかなと思います。(同期版を作るのであればその工数もありそう)
逆にもうちょっとでできるのであれば、このまま進んでも良いのかなと思っています!

同期版は作りたいですね。実装は多分簡単なので、コード量が増えるのを許容できたらという感じだと思っています。

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!!

@Hiroshiba Hiroshiba merged commit 280cc79 into VOICEVOX:main Nov 23, 2023
32 checks passed
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.

3 participants