initializeで全モデルを読み込まなくても良いようにした #124

Hiroshiba · 2022-05-04T21:23:25Z

内容

最初に全モデルを読み込まなくても起動できるようにしました。
必要なキャラクターだけloadしたり、必要になったタイミングでloadしたりできるようになります。

その他

少しリファクタリングが入っているので変更行が少し多めになってしまいました。
リファクタリング箇所だけ別PRにすることも可能です。

Hiroshiba · 2022-05-04T21:23:58Z

core/src/core.cpp

-#define NOT_FOUND_ERR "No such file or directory: "
-#define FAILED_TO_OPEN_MODEL_ERR "Unable to open model files."
-#define FAILED_TO_OPEN_METAS_ERR "Unable to open metas.json."
+#define NOT_LOADED_ERR "Model is not loaded."


消したのは未使用のエラーメッセージ

Hiroshiba · 2022-05-04T21:26:41Z

core/src/core.cpp

+/**
+ * ３種類のモデルを一纏めにしたもの
+ */
+struct VVMODEL {
  embed::EMBED_RES (*YUKARIN_S)();
  embed::EMBED_RES (*YUKARIN_SA)();
  embed::EMBED_RES (*DECODE)();
-} MODELS_LIST[] = {{YUKARIN_S, YUKARIN_SA, DECODE}};
+};
+const VVMODEL VVMODEL_LIST[] = {
+    {YUKARIN_S, YUKARIN_SA, DECODE},
+};
 }  // namespace EMBED_DECL_NAMESPACE
-using EMBED_DECL_NAMESPACE::MODELS_LIST;
+using EMBED_DECL_NAMESPACE::VVMODEL_LIST;


今まではMODELS={YUKARIN_S, YUKARIN_SA, DECODE}という命名規則でしたが、話者ごとのmodelsなのかyukarin_s/saなどのmodelsなのか分かりづらかったので、VVMODEL={YUKARIN_S, YUKARIN_SA, DECODE}という形にしました。

Hiroshiba · 2022-05-04T21:27:22Z

core/src/core.cpp

+      session_options.AppendExecutionProvider_CUDA(cuda_options);
+#endif
+    }
+  }


use_gpuはSTATUSの初期化時に、cpu_num_threadsはモデルロード時に指定だったのを、どちらも初期化時に指定する形式に変更しました。

core/src/core.h

qwerty2501 · 2022-05-04T21:50:36Z

core/src/core.h

 * @return 成功したらtrue、失敗したらfalse
 * @detail
 * 何度も実行可能。use_gpuを変更して実行しなおすことも可能。
 * 最後に実行したuse_gpuに従って他の関数が実行される。
 */
-VOICEVOX_CORE_API bool initialize(bool use_gpu, int cpu_num_threads = 0);
+VOICEVOX_CORE_API bool initialize(bool use_gpu, int cpu_num_threads = 0, bool load_all_models = true);


#122 の通りCではデフォルト引数が使えないので使用は避けたほうが良いです。
次にデフォルト引数をつかっても関数のシグネチャは bool initialize(bool,int,bool) になってしまうためABI互換性が破壊されると思います。互換性を保つことを意図してるのであればinitiaize関数の宣言はこのままにし、新しい名前のinitialize関数を定義したほうが良いでしょう
互換性保たない方針にするのであればこれについてもprefix voicevox をつけたほうが良いと思います。

名前の違う似た動作をするinitialize関数が２つあると混乱しそうなので避けたいです。
引数が1つ、2つ、3つの関数をそれぞれ定義するのが良いのかなと感じました。
別PRでも良さそうですが、どうでしょう…？👀

実はCにはoverloadがないんですよ。initializeは extern "C" 内に宣言されてるのでそれをやろうとするとコンパイルエラーになるはずです

なんと･･･。
じゃあ･･･互換性を捨てますか･･･。

今回はvoicevox_のprefixを付けることで別関数を定義できますが、今後引数が増えるたびに考える必要があって開発が大変そうです。
コアを利用するニーズを加味して、引数を増やしたときの互換性は考えない（再コンパイルが必要な）ようにする方針で進めようかと思いました。

（以前もABI互換性などで議論があった記憶がありますが、もしそのときの判断と異なっていそうであればコメントいただけると助かります。）

voicevox_prefixを付けるのは別のPRで行いたいと思います！

ABI互換性は捨ててよいとは思いますが、コードをなるべく書き換えないように努めたほうが良さそうですね。リファクタリングの範疇になりますが。
リファクタリングといえば、initialize等古い関数は戻り値をboolではなくVoicevoxResultCodeに変えたい気持ちはありますね。これをすることによって last_error_message関数への依存を減らせますし

VoicevoxResultCode統一、良さそうです！

qwerty2501

local変数にOrt::SessionをmoveしてしまうとSessionの所有権がlocal変数に対して移動してしまうのでscopeを抜ける際にそのlocal変数にあるSessionが開放されてしまうので次にそのmodelのSessionを使用する際に何らかの問題が発生する恐れがあるためmoveはしないほうが良いです
おそらくOrt::Sessionがコピー不可であるためこのようにしていると思いますが、コピー不可になっているのはそれなりの理由があります。

core/src/core.cpp

qwerty2501 · 2022-05-05T01:47:10Z

core/src/core.cpp

+  Status(int model_count, bool use_gpu, int cpu_num_threads)
+      : memory_info(Ort::MemoryInfo::CreateCpu(OrtDeviceAllocator, OrtMemTypeCPU)) {
+    yukarin_s_list = std::vector<std::optional<Ort::Session>>(model_count);
+    yukarin_sa_list = std::vector<std::optional<Ort::Session>>(model_count);
+    decode_list = std::vector<std::optional<Ort::Session>>(model_count);
+
+    session_options.SetInterOpNumThreads(cpu_num_threads).SetIntraOpNumThreads(cpu_num_threads);
+    if (use_gpu) {
+#ifdef DIRECTML
+      session_options.DisableMemPattern().SetExecutionMode(ExecutionMode::ORT_SEQUENTIAL);
+      Ort::ThrowOnError(OrtSessionOptionsAppendExecutionProvider_DML(session_options, 0));
+#else
+      const OrtCUDAProviderOptions cuda_options;
+      session_options.AppendExecutionProvider_CUDA(cuda_options);
+#endif
+    }
+  }


C++ではコンストラクタ内で例外が発生するとclass及びstructのfieldで確保されているリソースが開放されないためメモリリーク等の問題が起きます。そのためC++でのコンストラクタでは基本的にfieldの初期化のみ程度に留めるべきです。
Ort系の処理では例外が発生する可能性があるためこの処理はコンストラクタで行うべきではないでしょう。
そのためこのコンストラクタの処理をstaticメンバ関数にしたほうが望ましいです

なるほどです。
まあエラーが出たらそもそもこのライブラリが動かない（のでプロセスキルするはず）ので、そこまで気にしなくても良いのかなと思いました･･･！

コンストラク内の例外でメモリリークが発生してしまうのはデストラクタが呼ばれないからで、例外処理をきちんと書いていればメモリリークは起きません。
コンストラクタ外でメモリ確保をしても例外処理を書かなければメモリリークになるので、エラーハンドリングをしっかりしないなら何処に書いても変わらないと思われます。

コンストラク内の例外でメモリリークが発生してしまうのはデストラクタが呼ばれないからで、例外処理をきちんと書いていればメモリリークは起きません。

はい。正確にはデストラクタが呼ばれないですね。

コンストラクタ外でメモリ確保をしても例外処理を書かなければメモリリークになるので、エラーハンドリングをしっかりしないなら何処に書いても変わらないと思われます。

今回のケースだとリソースは各classのデストラクタで管理されているのでコンストラクタ外で処理を書けば例外が発生した場合各classのデストラクタが呼び出されるのでメモリリークは発生しないはずですね。
これが例えばStatusのフィールドで明示的に管理するリソースがあって、Status自身のデストラクタで開放しているのであればメモリリークする可能性はありますが。今回はStatus classとして明示的にデストラクタで開放してるフィールドは無いようなので
コンストラクタ外で処理を書けばメモリリークにはならないはずですが、Hihoさんがおっしゃるとおりライブラリが動かかないのでそもそもここを気にしてもあまり意味がないというのはあります。

すみませんコンストラクタ周りの仕様についてちょっと勘違いしていたらしく、そもそもこの実装のままでも各フィールドのデストラクタは呼ばれそうです...?
https://clown.cube-soft.jp/entry/20110320/1300596620

コンストラクタで例外が発生した場合，そのクラス自体のデストラクタは実行されませんが，（初期化の終了した）メンバ変数のデストラクタは実行されます．したがって，例えば new でメモリを確保するようなメンバ変数の場合は，スマート・ポインタで保持しておく事によってその部分のリソース開放が自動的に行われ，リソースの開放忘れのかなりの部分を防ぐ事ができます．

このままにするとのことだったのであまり意味のない情報ですが

Co-authored-by: qwerty2501 <[email protected]>

Hiroshiba · 2022-05-08T18:04:34Z

@Yosshi999 さん、@Oyaki122 さん、もしよかったらレビューいただけると心強いです･･･！

Hiroshiba · 2022-05-09T15:10:34Z

多分大丈夫だと思うのでマージします！

* デフォルト引数はC言語では使えないのでC++のみに有効になるように変更した (#122) 本当はデフォルト引数を消したかったが、使ってる人がいる可能性があるためC++で使ってる場合はそのままにするように修正した * initializeで全モデルを読み込まなくても良いようにした (#124) * load_model関数を切り出す * load_modelとis_model_loadedを足した * 使われてないエラーメッセージを消去 * Update core/src/core.cpp Co-authored-by: qwerty2501 <[email protected]> * Update core/src/core.cpp Co-authored-by: qwerty2501 <[email protected]> * Update core/src/core.cpp Co-authored-by: qwerty2501 <[email protected]> * pythonのラッパーの型を変更 * load_all_models追加 * return true Co-authored-by: qwerty2501 <[email protected]> * workflow_dispatchでもビルド開始可能に (#127) * ビルド時にREADMEファイルを追加する (#131) * append unit testing framework (#121) * Catch2をプロジェクト導入して参考となる1ケースを追加した * リリース用ビルドにはテストビルドは不要のため無効化する変数を追加した * テストコードの短縮化を行った * READMEにテスト実行方法を追加 * テストをビルドするかどうかのフラグをデフォルトOFFにした * テストをビルドするかどうかのフラグを変更したことによりREADMEを修正 * MSVCに対して足りないオプションを追加 - utf8文字列でコンパイルするように指定 - C++20を明示的に指定 - __cplusplusの値を利用中のC++のバージョンに合わせるように指定(ないとC++98相当になるとか) * Configがリリースの場合のみに最適化オプションを指定するように修正 * オプションの指定を短くまとめた * coreの標準ライブラリもバージョンアップした * compile optionsの修正 * Catch2にもCXX_STANDARD 20を追加 * Windows環境でビルドできるように設定を修正 #121 (comment) * format 効いてしまっていたところを修正 * static castやnullopt比較をやめ、value関数などを使うようにする (#118) * use value and has value * remove has_value * coreのビルド時にバージョン情報がちゃんと入るようにする * hotfix イントネーションの推論をCPUで行うように (#146) * python example for 0.12, update FFI (#138) * .gitignore open_jtalk_dic in example (#148) * .gitignore open_jtalk_dic in example * modify: example/python/.gitignore * C++サンプルコードへのリンクを追加 (#142) * C++サンプルコードへのリンクを追加 * #readme * Update README.md * コード署名できるようにする (#164) * コード署名 * build_util * artifact/ * a * remove * inputのbooleanは文字列として渡ってくるので判定を修正 (#166) * inputのbooleanは文字列として渡ってくるので判定を修正 * 修正もれ Co-authored-by: Hiroshiba <[email protected]> Co-authored-by: Yuto Ashida <[email protected]> Co-authored-by: Yosshi999 <[email protected]> Co-authored-by: nebocco <[email protected]>

Hiroshiba added 3 commits May 5, 2022 05:37

load_model関数を切り出す

210b6e1

load_modelとis_model_loadedを足した

eafac33

使われてないエラーメッセージを消去

a0c3539

Hiroshiba commented May 4, 2022

View reviewed changes

qwerty2501 reviewed May 4, 2022

View reviewed changes

qwerty2501 reviewed May 5, 2022

View reviewed changes

core/src/core.cpp Outdated Show resolved Hide resolved

core/src/core.cpp Outdated Show resolved Hide resolved

core/src/core.cpp Outdated Show resolved Hide resolved

qwerty2501 reviewed May 5, 2022

View reviewed changes

Hiroshiba and others added 6 commits May 6, 2022 03:08

Update core/src/core.cpp

619e403

Co-authored-by: qwerty2501 <[email protected]>

Update core/src/core.cpp

49c8e6f

Co-authored-by: qwerty2501 <[email protected]>

Update core/src/core.cpp

deb77c9

Co-authored-by: qwerty2501 <[email protected]>

pythonのラッパーの型を変更

9daf2e4

load_all_models追加

dcfa19f

return true

662147c

Hiroshiba mentioned this pull request May 5, 2022

APIにprefixを付ける、返り値をVoicevoxResultCodeで統一する #125

Closed

Merge remote-tracking branch 'upstream/main' into モデル読み込みをlazyにする

aeb7d4e

qwerty2501 mentioned this pull request May 9, 2022

Rustへの移行 #126

Merged

Hiroshiba merged commit c66081e into VOICEVOX:main May 9, 2022

Hiroshiba deleted the モデル読み込みをlazyにする branch May 9, 2022 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initializeで全モデルを読み込まなくても良いようにした #124

initializeで全モデルを読み込まなくても良いようにした #124

Hiroshiba commented May 4, 2022

Hiroshiba May 4, 2022

Hiroshiba May 4, 2022

Hiroshiba May 4, 2022 •

edited

Loading

qwerty2501 May 4, 2022

Hiroshiba May 4, 2022 •

edited

Loading

qwerty2501 May 4, 2022

Hiroshiba May 5, 2022 •

edited

Loading

qwerty2501 May 5, 2022

Hiroshiba May 5, 2022

qwerty2501 left a comment

qwerty2501 May 5, 2022

Hiroshiba May 5, 2022 •

edited

Loading

Segu-g May 6, 2022

qwerty2501 May 6, 2022

qwerty2501 May 7, 2022

Hiroshiba commented May 8, 2022

Hiroshiba commented May 9, 2022

initializeで全モデルを読み込まなくても良いようにした #124

initializeで全モデルを読み込まなくても良いようにした #124

Conversation

Hiroshiba commented May 4, 2022

内容

関連 Issue

その他

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hiroshiba May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hiroshiba May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hiroshiba May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwerty2501 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hiroshiba May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hiroshiba commented May 8, 2022

Hiroshiba commented May 9, 2022

Hiroshiba May 4, 2022 •

edited

Loading

Hiroshiba May 4, 2022 •

edited

Loading

Hiroshiba May 5, 2022 •

edited

Loading

Hiroshiba May 5, 2022 •

edited

Loading