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

Programクラスのパフォーマンス改善 #55

Merged
merged 7 commits into from
May 12, 2019
Merged

Programクラスのパフォーマンス改善 #55

merged 7 commits into from
May 12, 2019

Conversation

masnagam
Copy link
Contributor

getおよびexistsメソッドの計算量をO(N)からO(1)に改善.

これにより,以下のパフォーマンスが改善します.

  • 起動時のprogram.jsonの読み込み時間
  • findByNetworkIdAndReplaceのブロック時間

addのたびに,_itemを線形検索していましたが,ほとんどの場合IDがマッチすることはないのでかなりの時間を消費していました.

この修正により,findByNetworkIdAndReplaceのブロック時間は問題にならない程度に速くなったので(Rock64でも1秒以下),findByNetworkIdAndReplaceの非同期化は不要となりました.そのため,asyncおよびawaitのみ削除しました.

コメントにあるように,_items_itemMapに置き換えてしまうことも可能でしたが,そうしてしまうと,Programクラスの大部分を書き換えることになるため,置き換えはしませんでした.置き換えることで,spliceしなくて良くなるのでもう少し速くなるかもしれませんが,すでに十分速くなっているので,必要性は低いと考えています.

_items_itemMapという似たものが2つあるのはよくないということでしたら,_itemsを削除しますので,指摘をお願いします.

Improve the time complexity of `get` and `exists` methods from O(N) to
O(1).

This change affects performance of the following functions:

* Load time of program.json
* findByNetworkIdAndReplace

New private member variable `_itemMap` has been introduced, which maps
program IDs to their associated ProgramItem objects.
@masnagam
Copy link
Contributor Author

masnagam commented Apr 26, 2019

ちなみに,この修正は #56 とは無関係です

@kanreisa
Copy link
Member

kanreisa commented May 3, 2019

対応が遅くなりました。

_itemMap なのですが、Map はいかがでしょうか?
Map 使った上で、もしパフォーマンス問題なさそうであれば、_items を消してしまってもよいと思います。

@masnagam
Copy link
Contributor Author

masnagam commented May 3, 2019

_itemsを削除するかどうかは,パフォーマンスの良し悪しで判断するのではなく,修正による影響を考慮した上で判断すべきだと考えます.

書き換えてもいいのですが,以下について問題がないか判断をお願いします.

  • 最初のコメントにも書きましたが,_itemsを現在のArrayからObjectまたはMapに置き換えた場合,Program.tsを大幅に書き換えることになります
    • 恐らく,diffを確認する意味がなくなります
  • Program.all()およびProgram.itemsの動作が変わります
    • 連想配列からプログラム一覧を取得するので,毎回違うJS Objectが返されますし,要素の順番も保証されません
    • 上記のようにProgram.all()およびProgram.itemsの動作が変わると,問題が発生するようなコードはないですか?

問題ないのであれば書き換えます.

ObjectMapもV8ではハッシュテーブルのようなものを使って実装されているようです.つまり,要素数に対して大雑把に考えてO(1)です.そのため,どちらを使っても大して変わらないと考えています.最初のコメントにあるように,要素を追加するたびに_itemsを線形検索(O(N))していて,ほとんど毎回全要素をサーチしていることが,速度が遅いことの原因だからです.

@kanreisa
Copy link
Member

kanreisa commented May 3, 2019

まさに

似たものが2つあるのはよくない

と考えていますから、提案していただいたパフォーマンス改善の意図をそのまま生かす形で Map 使えたら Map 使ってみましょう

私もテストしますので、ぜひ書き換えてみてください。

src/Mirakurun/Program.ts Outdated Show resolved Hide resolved
@masnagam
Copy link
Contributor Author

masnagam commented May 4, 2019

了解です.念の為,以下の順番でコミットします.

  1. Object -> Mapへの変更のみ
  2. _itemsを削除
  3. _itemMap -> _itemsに改名

2, 3の修正がまずかったら簡単に戻せるようにしておきます.

The `Map` class keeps insertion order.  So, we expect that the order
of program items in new `Program.items` is the same as the old one,
hopefully...
@masnagam
Copy link
Contributor Author

masnagam commented May 4, 2019

コミットログにも書きましたが,Map挿入順を記憶しているみたいです.上手くいけば,Program.itemsの順番は以前と同じはず..

@masnagam
Copy link
Contributor Author

masnagam commented May 4, 2019

_itemMap_itemsに改名すると,名前が似ているものと混同してしまうので止めておきました.

Dockerfile has been updated because it didn't work.  In addition, the
inconsistent line endings in Dockerfile have been fixed.

docker-compose.yml has been added for test purpose.  A data volume
which mounted at /usr/local/var/db/mirakurun is created automatically
when running `docker-compose up`.  Use `docker-compose down` with the
`-v` option if you want to remove the data volume at the shutdown.  Or
remove it manually using `docker voluem rm`.
@masnagam
Copy link
Contributor Author

masnagam commented May 4, 2019

単体テストなどがないため,とりあえず暫く動かしてみます

@masnagam
Copy link
Contributor Author

masnagam commented May 5, 2019

一日程度動かしましたが,Map変更・_items削除前と比べて遅くなったりはしていないようでした.

Copy link
Member

@kanreisa kanreisa left a comment

Choose a reason for hiding this comment

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

不要な変更とファイルが混じってしまっているようです

@masnagam
Copy link
Contributor Author

masnagam commented May 12, 2019

fbc67e1 で行ったDockerfileの変更とdocker-compose.ymlの追加でしょうか?

fbc67e1 をrevertし,別のPRとして出し直します.

@masnagam
Copy link
Contributor Author

1c7acea でrevertしました.

Dockerfileの修正とdocker-compose.ymlの追加だったので,revertしても本件の修正には影響を与えません.

@masnagam
Copy link
Contributor Author

分離して #59 を作成しました.

@kanreisa kanreisa merged commit bc1ca61 into Chinachu:master May 12, 2019
@kanreisa
Copy link
Member

ありがとうございます。マージしました。
(https://github.com/Chinachu/Mirakurun/blob/master/CHANGELOG.md

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.

None yet

2 participants