search

runceel / ReactiveProperty

ReactiveProperty provides MVVM and asynchronous support features under Reactive Extensions. Target frameworks are .NET 6+, .NET Framework 4.7.2 and .NET Standard 2.0.
MIT License
904 stars 101 forks source link

IFilteredReadOnlyObservableCollection のパフォーマンスを改善できないでしょうか？ #228

Closed kenichi-kobayashi closed 3 years ago

kenichi-kobayashi commented 3 years ago

Issue summary for non Japanese speakers

The performance of ToFileRedReadOnlyObservableCollection for 1,000,000 collections on UI thread is not good. Using the PropertyChanged and CollectionChanged methods to implement the same process improved the performance significantly.

Processing time: 540 sec to 5 sec Memory usage: 1,024 MB to 204 MB

When using FromEvent of Reactive Extensions, an instance of SynchronizationContextScheduler was created to automatically dispatch to the thread that subscribed, so I quit using FromEvent and replaced the process by implementing FromEvent that does not dispatch the thread.

Before:

Method Count Mean Error StdDev
ObserveProperty 1 4.662 us 0.0403 us 0.0377 us
ObserveProperty 10 46.159 us 0.2482 us 0.2322 us
ObserveProperty 100 488.395 us 3.9745 us 3.7177 us
ObserveProperty 1000 7,299.851 us 49.5364 us 41.3651 us
ObserveProperty 10000 184,803.556 us 2,443.4350 us 2,285.5906 us
ObserveProperty 100000 21,848,008.562 us 183,192.5623 us 152,974.0864 us
After: Method Count Mean Error StdDev
ObserveProperty 1 3.313 us 0.0226 us 0.0200 us
ObserveProperty 10 32.927 us 0.2260 us 0.2114 us
ObserveProperty 100 335.572 us 2.0898 us 1.9548 us
ObserveProperty 1000 4,245.384 us 82.0820 us 91.2340 us
ObserveProperty 10000 79,851.266 us 1,057.3668 us 937.3281 us
ObserveProperty 100000 779,906.493 us 8,120.6971 us 7,596.1051 us

Original (Japanese)

ReactivePropertyをいつも便利に使わせてもらっています。 とても強力なライブラリで、これが無いともう開発したくないくらい依存しています。 そんなReactivePropertyなのですが、以下の点について改善する余地がないか相談に乗ってください。

■相談内容 １００万個の配列から、特定の条件で抽出した配列を作成するために、ToFilteredReadOnlyObservableCollectionを使ってみました。自分のPCスペックでは、処理時間が９分で、使用メモリが１Ｇバイトになってしまいました。 これだと実用が難しいため、「PropertyChanged」を自前でイベント登録して、同様の処理を実現しました。すると、処理時間が５秒で、メモリが２０４ＭＢとなりました。

まとめると、以下のとおりパフォーマンス面で大きく下回ってしまいまいました。

PropertyChanged → ToFilteredReadOnlyOC

処理時間：5秒 →540秒 メモリ：204MB→1024MB

テストプログラムを添付します。

FilteredPerformanceTest.zip

何か、改善する方法はないでしょうか？

runceel commented 3 years ago

ToFilteredReadOnlyOC は内部的に元のコレクションのCollectionChangedも監視していて、中途半端な位置への要素の追加削除なども監視しているので、PropertyChangedを監視して実現してるものよりも多くの処理をやっています。 なのでPropertyChangedだけの監視で実装してみたコードよりは早くなることはありません。

ただ、それを差し引いても遅すぎるのでReactivePropertyの単体テストに処理時間がかかりすぎると失敗するテストを追加してみたのですが、100万件のコレクションを元にToFilteredReadOnlyOCを作成してみても5秒程度で終わってしまいました。

原因は不明ですが提供していただいたコードでは確かに時間がかかるのですが、現状原因はわかっていません。

okaryu commented 3 years ago

追加で調査をしてみました。

ToFilteredReadOnlyOC にて 540 秒かかっていた問題に関しては、 Task.Run で別スレッドにて実行することで 14 秒程度で実行されることがわかりました。 UIスレッドで実行していたことが影響しているのかと思いますが、詳しい検証はできていません。

元のコレクションのCollectionChangedも監視するようにして、改めて比較してみたところ以下のような結果でした。 時間の計測に関しては ToFilteredReadOnlyOC の実行時間と PropertyChanged などの設定の時間を計測しています。

PropertyChanged → ToFilteredReadOnlyOC

処理時間：0.3 秒 → 14.4 秒 メモリ：116 MB → 995 MB

上記のように、処理時間とメモリ使用量に関して大きく差が生じてしまっています。

もう少し検証を進めようと思いますが、なにか、改善のアイディアや検証すべき内容などはありますでしょうか？

runceel commented 3 years ago

@okaryu UI スレッド上での実行で時間がかかる点と、メモリ上に生成されているオブジェクトから恐らく FilteredReadOnlyObservableCollection 内部で使用している Rx 系のメソッドか、それにかかわる処理のところで大量に意図していないオブジェクトが作られていたので Rx 系のメソッドをなるべく使わないように変更しました。

プレリリース版として修正したものをリリースしました。 7.7.1-pre202102190925

既存の単体テストは全て通っているのですが、そちらでも動作確認してみて頂いてもいいですか？ 提供していただいたサンプルプロジェクトでパッケージを最新のプレビュー版に更新すると、以下のような数値になりました。

PropertyChanged: 736ms FilteredReadOnlyObservableCollection : 1381ms

okaryu commented 3 years ago

@runceel

早速、修正をしていただきありがとうございました。感激です！ こちらの開発環境で動作確認をして問題なさそうなことを確認いたしました。

どのような対応をしたのかを知るためにソースコードの差分を拝見いたしました。

今回、以下の二つのメソッドを使わないように修正されたと思います。

・ObserveElementPropertyChanged ・CollectionChangedAsObservable

上記の二つメソッドは共通して RX 提供の Observable.FromEvent を呼び出しています。 この FromEvent にパフォーマンスの課題があるという認識でいいでしょうか？ 当方も同じ箇所に問題があるのではないかと考えていました。

もし、そうであるならば、FromEvent は様々な箇所で使用されていると思います。 FilteredReadOnlyObservableCollection 以外の箇所についてはどのようにお考えでしょうか？

当方でも ReactiveExtension (https://github.com/dotnet/reactive) のコードを少し見てみましたが、 非同期処理が複雑に絡まっているように見え、当方の実力ではどこが課題なのかわかりませんでした。

お考えをお聞かせいただける幸いです。

runceel commented 3 years ago

改善したみたいでよかったです。 FromEvent(Pattern) は、ここに行き着くと思われるのですが SynchronizationContext.Current が null じゃないとスケジューラーを毎度毎度作ってるみたいです。 https://github.com/dotnet/reactive/blob/main/Rx.NET/Source/src/System.Reactive/Linq/QueryLanguage.Events.cs

今回みたいに 100 万個のインスタンスになると純粋に 100 万個スケジューラーのインスタンスが作られて、なおかつ CollectionChanged や PropertyChanged が発生するたびにスケジューラー経由で処理が実行されるので早くは無さそうです。

なので FromEvent(Pattern) を使うと、普通にイベントを購読するよりも遅いのは間違いないですが、20 万個くらいのインスタンスとかにでもしない限りは大きな問題にはならないと思います。

この動作が嫌な場合は FromEvent(Pattern) を使うときは明示的に ImmdiateScheduler (名前うろ覚え) のようなパフォーマンスがそこまで劣化しないようなスケジューラーを指定するとか、自分で何もせず即座に実行するだけのスケジューラー作ってしまうとかいうのも手としてはあると思います。

runceel commented 3 years ago

すいません。関連PRをマージしたのでクローズになりましたが続けるのは大丈夫です

runceel commented 3 years ago

FromEvent を自前のものに差し替えたバージョンの 7.8.0 のプレリリース版を公開しました。 まだ性能比較をしてない状態です。

runceel commented 3 years ago

改善前

Method Count Mean Error StdDev
ObserveProperty 1 4.662 us 0.0403 us 0.0377 us
ObserveProperty 10 46.159 us 0.2482 us 0.2322 us
ObserveProperty 100 488.395 us 3.9745 us 3.7177 us
ObserveProperty 1000 7,299.851 us 49.5364 us 41.3651 us
ObserveProperty 10000 184,803.556 us 2,443.4350 us 2,285.5906 us
ObserveProperty 100000 21,848,008.562 us 183,192.5623 us 152,974.0864 us

改善後

Method Count Mean Error StdDev
ObserveProperty 1 3.313 us 0.0226 us 0.0200 us
ObserveProperty 10 32.927 us 0.2260 us 0.2114 us
ObserveProperty 100 335.572 us 2.0898 us 1.9548 us
ObserveProperty 1000 4,245.384 us 82.0820 us 91.2340 us
ObserveProperty 10000 79,851.266 us 1,057.3668 us 937.3281 us
ObserveProperty 100000 779,906.493 us 8,120.6971 us 7,596.1051 us
kenichi-kobayashi commented 3 years ago

返信がおそくなってすいません。 処理時間、メモリ共に、大幅の改善が行われたことを確認できました。 V7.8.0のリリースありがとうございます。 ただ、別の課題が出てきたので、別Issueにて投稿させていただきますので、相談に乗ってもらえるとありがたいです。