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

[Rust] supported_devices を実装した #149

Merged

Conversation

PickledChair
Copy link
Member

@PickledChair PickledChair commented Jun 7, 2022

内容

dll で export する C API のうち supported_devices 関数を実装しました。

C++ の実装(以下を参照)に習って、API を叩くたびに onnxruntime 側に使用できるデバイスを問い合わせる仕様にしています(必要に応じて、文字列の初期化時のみに使用できるデバイスを取得するように変更することを検討します)。

const char *supported_devices() {
SupportedDevices devices = get_supported_devices();
nlohmann::json json = devices;
supported_devices_str = json.dump();
return supported_devices_str.c_str();
}

関連 Issue

ref #128

その他

このAPIを実装することで、Rust製コアをエンジンで読み込むことがとりあえず可能になります。

..,
&(serde_json::to_string(&SupportedDevices::get_supported_devices().unwrap()).unwrap()
+ "\0"),
);
Copy link
Contributor

@qwerty2501 qwerty2501 Jun 7, 2022

Choose a reason for hiding this comment

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

この処理起動時に行うようにすれば SUPPORTED_DEVICES_STRING の Mutex外れるのでは?

get_supported_devicesって結構処理重かったりするんですかね

Copy link
Member Author

@PickledChair PickledChair Jun 7, 2022

Choose a reason for hiding this comment

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

この処理起動時に行うようにすれば SUPPORTED_DEVICES_STRING の Mutex外れるのでは?

そうだと思います!

get_supported_devicesって結構処理重かったりするんですかね

大して重くはない気がします。

C++ における実装に習った結果このような実装になっただけなので、初期化時に一度だけ文字列を作るだけにして良いのであれば、そのように改変しようと思っています。C++ の実装で supported_devices を呼ぶ度にこの文字列を作っている理由が何かあるのであれば知りたいです。

Copy link
Contributor

Choose a reason for hiding this comment

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

問題なさそうならやってしまいたいですね 👍

Copy link
Member

Choose a reason for hiding this comment

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

途中でGPUデバイス外したりしたら挙動変わりそうですが、どっちでも良いと思います!

Copy link
Member Author

Choose a reason for hiding this comment

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

わかりました! 最初の関数呼び出し時にのみ文字列を作成するように変更してみました。

Comment on lines 72 to 81
static SUPPORTED_DEVICES_STRING: Lazy<Mutex<String>> = Lazy::new(|| Mutex::new(String::new()));
pub fn supported_devices() -> MutexGuard<'static, String> {
let mut supported_devices_string = SUPPORTED_DEVICES_STRING.lock().unwrap();
supported_devices_string.replace_range(
..,
&(serde_json::to_string(&SupportedDevices::get_supported_devices().unwrap()).unwrap()
+ "\0"),
);
supported_devices_string
}
Copy link
Contributor

Choose a reason for hiding this comment

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

環境によって結果は変わりますが、呼び出せる程度を確認する何かしらのテストコードほしいところではあります

Copy link
Member Author

Choose a reason for hiding this comment

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

おっしゃる通りです。テストを追加します。

Copy link
Member Author

Choose a reason for hiding this comment

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

戻り値の &CStr&str に戻すことが可能であることと、それをさらに SupportedDevices に戻せることをチェックするテストを書いてみました。

Copy link
Contributor

@qwerty2501 qwerty2501 left a comment

Choose a reason for hiding this comment

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

LGTM

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 1854b4a into VOICEVOX:rust Jun 7, 2022
qwerty2501 pushed a commit to qwerty2501/voicevox_core that referenced this pull request Jul 23, 2022
* add supported_devices implementation

* cargo fmt

* remove extra use

* get supported_devices string only at initialization

* implement test for supported_devices c api

* change global variable name
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