Performance Improvements #6
Conversation
| @@ -159,7 +159,7 @@ class PURLogStoreSpec: QuickSpec { | |||
| logStore.retrieveLogsForPattern("testA.*", output: outputA, completion: { logs in | |||
| count = logs.count | |||
| }) | |||
| expect(count).toEventually(equal(300), timeout: 3) | |||
| expect(count).toEventually(equal(300), timeout: 20) | |||
There was a problem hiding this comment.
takes about 5~8 seconds.
There was a problem hiding this comment.
なるほど、retrieveLogsForPattern が非同期でログを読み込むようにはなったが、時間がかかるようにはなってしまったんですかね…
There was a problem hiding this comment.
そうですね。
この変更での一番悩みどころですが、スクロール中にログを送ることがあり、FPSが落ちていましたので…。
もしこれがネックになりそうでしたらCloseして頂いて大丈夫です。
There was a problem hiding this comment.
なるほど。時間がかかるよりも、ログが欠損したり重複したりすることがない事を気にしているので、それが守られて体験が向上するなら良いと思います。
|
|
||
| NSOperationQueue *writeChunkQueue = [[NSOperationQueue alloc] init]; | ||
| writeChunkQueue.maxConcurrentOperationCount = 1; | ||
| self.writeChunkQueue = writeChunkQueue; |
There was a problem hiding this comment.
writeChunk が複数走らないようにする意図があったんですが、何か問題がありましたか？
There was a problem hiding this comment.
変更前、writeChunkは別のスレッドで実行されることを見落としていた（ライブラリを使う側として）のと、サブスレッドで実行される理由が見当たらなかったので（メインスレッドを使うので良さそうでした）消しました。
completionの内部での処理もロックがされていなかったので、ロック処理を書くよりシンプルになって良いかなと思いますがいかがでしょうか？
There was a problem hiding this comment.
メインスレッドに変えましたので「複数走らない」というこれまでと仕様とは変わっていないつもりです。
（認識違いがありましたらご指摘お願いいたしますm(_ _)m）
There was a problem hiding this comment.
writeChunk (callWriteChunk) がメインスレッドで実行されるように変わったってことですよね？
呼び出しているところは
- flush (こちらも確実にメインスレッドで呼ばれるようにしたほうが良いのかしら)
- callWriteChunk の retry
たしかに、buffered output プラグインを継承したプラグインの実装の方で気をつければメインスレッドで問題ないはずですよね、大丈夫そうな気がしてきました。
とはいえこのPRをマージしたらメジャーバージョンあげたりしたいですね
There was a problem hiding this comment.
はい。メインスレッドで実行するように変更しました。
たしかに、buffered output プラグインを継承したプラグインの実装の方で気をつければメインスレッドで問題ないはずですよね
参考までに、どのようなことを気をつけた方がよさそうでしょうか？
メジャーアップデートの件、賛成です。
There was a problem hiding this comment.
参考までに、どのようなことを気をつけた方がよさそうでしょうか？
ライブラリを使う人がプラグインを実装する際に、writeChunk がメインスレッドで実行されるということを意識して実装してもらえれば大丈夫かなと思いました。
例えば、サーバにログを送るようなプラグインを書く場合、同期的に書かない、など。
completion ブロックでoutputプラグインの成功・失敗を指定する形になっているので、多分そんなに影響を受けるアプリはないような気がします。
|
大変長い間PRを放置してしまい、申し訳ありませんでした。 動作確認に Quick/Nimble のアップデートが必要でした。こちらは別途PRを出そうと思っていますが、このPRに含めていただいても構いません。 僕の意図がきちんと伝わらなくても困ってしまうので日本語でコメントしています。コメントしていただける際は 日本語/英語 どちらでもかまいません。よろしくお願いします。 |
|
@slightair レビューありがとうございました。返信と質問をしましたのでご確認お願い致します。返信を頂いてから修正をしようと思っています。よろしくお願いいたしますm(_ _)m |
|
@slightair コメント頂いた点を一通り修正しましたので再度ご確認をお願いいたしますm(_ _)m |
# Conflicts: # Example/Podfile.lock
|
LGTM なるべくはやめに 2.0.0 としてリリースできるようにします。 🍥 |
|
🍺 |
DBの読み書きがメインスレッドで実行されておりUIアップデート処理がブロックされていたので、非同期のメソッドに移行しました。
DB I/O is performed on the main thread which blocks UI updates, so change to use asynchronous methods.