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

Performance Improvements #6

Merged
merged 9 commits into from Feb 23, 2016

Conversation

Projects
None yet
2 participants
@chuganzy
Contributor

chuganzy commented Oct 13, 2015

DBの読み書きがメインスレッドで実行されておりUIアップデート処理がブロックされていたので、非同期のメソッドに移行しました。

DB I/O is performed on the main thread which blocks UI updates, so change to use asynchronous methods.

@@ -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)

This comment has been minimized.

@chuganzy

chuganzy Oct 13, 2015

Contributor

takes about 5~8 seconds.

@chuganzy

chuganzy Oct 13, 2015

Contributor

takes about 5~8 seconds.

This comment has been minimized.

@slightair

slightair Feb 22, 2016

Member

なるほど、retrieveLogsForPattern が非同期でログを読み込むようにはなったが、時間がかかるようにはなってしまったんですかね…

@slightair

slightair Feb 22, 2016

Member

なるほど、retrieveLogsForPattern が非同期でログを読み込むようにはなったが、時間がかかるようにはなってしまったんですかね…

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

そうですね。
この変更での一番悩みどころですが、スクロール中にログを送ることがあり、FPSが落ちていましたので…。
もしこれがネックになりそうでしたらCloseして頂いて大丈夫です。

@chuganzy

chuganzy Feb 22, 2016

Contributor

そうですね。
この変更での一番悩みどころですが、スクロール中にログを送ることがあり、FPSが落ちていましたので…。
もしこれがネックになりそうでしたらCloseして頂いて大丈夫です。

This comment has been minimized.

@slightair

slightair Feb 22, 2016

Member

なるほど。時間がかかるよりも、ログが欠損したり重複したりすることがない事を気にしているので、それが守られて体験が向上するなら良いと思います。

@slightair

slightair Feb 22, 2016

Member

なるほど。時間がかかるよりも、ログが欠損したり重複したりすることがない事を気にしているので、それが守られて体験が向上するなら良いと思います。

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

分かりました!

@chuganzy

chuganzy Feb 22, 2016

Contributor

分かりました!

@@ -158,7 +158,7 @@ class PURLoggerSpec: QuickSpec {
"buffered.c-aaa=1",
"buffered.c-aaa=2",
"buffered.d-aaa=3"
), timeout: 1)
), timeout: 10)

This comment has been minimized.

@chuganzy

chuganzy Oct 13, 2015

Contributor

about 3~5 seconds.

@chuganzy

chuganzy Oct 13, 2015

Contributor

about 3~5 seconds.

NSOperationQueue *writeChunkQueue = [[NSOperationQueue alloc] init];
writeChunkQueue.maxConcurrentOperationCount = 1;
self.writeChunkQueue = writeChunkQueue;

This comment has been minimized.

@slightair

slightair Feb 22, 2016

Member

writeChunk が複数走らないようにする意図があったんですが、何か問題がありましたか?

@slightair

slightair Feb 22, 2016

Member

writeChunk が複数走らないようにする意図があったんですが、何か問題がありましたか?

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

変更前、writeChunkは別のスレッドで実行されることを見落としていた(ライブラリを使う側として)のと、サブスレッドで実行される理由が見当たらなかったので(メインスレッドを使うので良さそうでした)消しました。
completionの内部での処理もロックがされていなかったので、ロック処理を書くよりシンプルになって良いかなと思いますがいかがでしょうか?

@chuganzy

chuganzy Feb 22, 2016

Contributor

変更前、writeChunkは別のスレッドで実行されることを見落としていた(ライブラリを使う側として)のと、サブスレッドで実行される理由が見当たらなかったので(メインスレッドを使うので良さそうでした)消しました。
completionの内部での処理もロックがされていなかったので、ロック処理を書くよりシンプルになって良いかなと思いますがいかがでしょうか?

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

メインスレッドに変えましたので「複数走らない」というこれまでと仕様とは変わっていないつもりです。
(認識違いがありましたらご指摘お願いいたしますm(_ _)m)

@chuganzy

chuganzy Feb 22, 2016

Contributor

メインスレッドに変えましたので「複数走らない」というこれまでと仕様とは変わっていないつもりです。
(認識違いがありましたらご指摘お願いいたしますm(_ _)m)

This comment has been minimized.

@slightair

slightair Feb 22, 2016

Member

writeChunk (callWriteChunk) がメインスレッドで実行されるように変わったってことですよね?

呼び出しているところは

  • flush (こちらも確実にメインスレッドで呼ばれるようにしたほうが良いのかしら)
  • callWriteChunk の retry

たしかに、buffered output プラグインを継承したプラグインの実装の方で気をつければメインスレッドで問題ないはずですよね、大丈夫そうな気がしてきました。
とはいえこのPRをマージしたらメジャーバージョンあげたりしたいですね

@slightair

slightair Feb 22, 2016

Member

writeChunk (callWriteChunk) がメインスレッドで実行されるように変わったってことですよね?

呼び出しているところは

  • flush (こちらも確実にメインスレッドで呼ばれるようにしたほうが良いのかしら)
  • callWriteChunk の retry

たしかに、buffered output プラグインを継承したプラグインの実装の方で気をつければメインスレッドで問題ないはずですよね、大丈夫そうな気がしてきました。
とはいえこのPRをマージしたらメジャーバージョンあげたりしたいですね

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

はい。メインスレッドで実行するように変更しました。

たしかに、buffered output プラグインを継承したプラグインの実装の方で気をつければメインスレッドで問題ないはずですよね

参考までに、どのようなことを気をつけた方がよさそうでしょうか?

メジャーアップデートの件、賛成です。

@chuganzy

chuganzy Feb 22, 2016

Contributor

はい。メインスレッドで実行するように変更しました。

たしかに、buffered output プラグインを継承したプラグインの実装の方で気をつければメインスレッドで問題ないはずですよね

参考までに、どのようなことを気をつけた方がよさそうでしょうか?

メジャーアップデートの件、賛成です。

This comment has been minimized.

@slightair

slightair Feb 22, 2016

Member

参考までに、どのようなことを気をつけた方がよさそうでしょうか?

ライブラリを使う人がプラグインを実装する際に、writeChunk がメインスレッドで実行されるということを意識して実装してもらえれば大丈夫かなと思いました。
例えば、サーバにログを送るようなプラグインを書く場合、同期的に書かない、など。
completion ブロックでoutputプラグインの成功・失敗を指定する形になっているので、多分そんなに影響を受けるアプリはないような気がします。

@slightair

slightair Feb 22, 2016

Member

参考までに、どのようなことを気をつけた方がよさそうでしょうか?

ライブラリを使う人がプラグインを実装する際に、writeChunk がメインスレッドで実行されるということを意識して実装してもらえれば大丈夫かなと思いました。
例えば、サーバにログを送るようなプラグインを書く場合、同期的に書かない、など。
completion ブロックでoutputプラグインの成功・失敗を指定する形になっているので、多分そんなに影響を受けるアプリはないような気がします。

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

かしこまりました

@chuganzy

chuganzy Feb 22, 2016

Contributor

かしこまりました

return self.database && self.databaseConnection;
if (self.databaseConnection.database != database) {
self.databaseConnection = [database newConnection];
}

This comment has been minimized.

@slightair

slightair Feb 22, 2016

Member

logstore の prepare は一度しか呼ばないつもりだったんですが、この変更は必要ですか?

@slightair

slightair Feb 22, 2016

Member

logstore の prepare は一度しか呼ばないつもりだったんですが、この変更は必要ですか?

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

詳しく覚えていませんが(すみません)、テスト内で呼ばれていました。
DBは同じなのにconnectionが違い、connectionのメンバー変数のキューがあり、それぞれが変わってしまい処理順がバラバラになってしまうため必要でした。

@chuganzy

chuganzy Feb 22, 2016

Contributor

詳しく覚えていませんが(すみません)、テスト内で呼ばれていました。
DBは同じなのにconnectionが違い、connectionのメンバー変数のキューがあり、それぞれが変わってしまい処理順がバラバラになってしまうため必要でした。

This comment has been minimized.

@slightair

slightair Feb 22, 2016

Member

ふーむ、テストで呼んではいますが同じLogStoreのインスタンスにprepareを呼ぶケースはあるのかな…
timeout の件も関係ありそうですが、テストも非同期前提では書かれていないのでテストのほうがよくないかもしれませんね、すみません

@slightair

slightair Feb 22, 2016

Member

ふーむ、テストで呼んではいますが同じLogStoreのインスタンスにprepareを呼ぶケースはあるのかな…
timeout の件も関係ありそうですが、テストも非同期前提では書かれていないのでテストのほうがよくないかもしれませんね、すみません

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

こちら修正と一緒にどのテストか調査いたします

@chuganzy

chuganzy Feb 22, 2016

Contributor

こちら修正と一緒にどのテストか調査いたします

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

https://github.com/cookpad/puree-ios/blob/master/Example/Tests/PURLoggerSpec.swift#L149
この行でした。このようにconfigurationを使いまわした場合ですね🤔
(少なくとも私たちはこのように使いまわしたりはしていません)

@chuganzy

chuganzy Feb 22, 2016

Contributor

https://github.com/cookpad/puree-ios/blob/master/Example/Tests/PURLoggerSpec.swift#L149
この行でした。このようにconfigurationを使いまわした場合ですね🤔
(少なくとも私たちはこのように使いまわしたりはしていません)

This comment has been minimized.

@slightair

slightair Feb 23, 2016

Member

なるほどー
実際に使う場合と違ってテストのやり方がよくなさそうですね。
このまま取り込ませていただいて、あとでテストを見直してみたいと思います

@slightair

slightair Feb 23, 2016

Member

なるほどー
実際に使う場合と違ってテストのやり方がよくなさそうですね。
このまま取り込ませていただいて、あとでテストを見直してみたいと思います

return @{
LogMetadataKeyOutput: NSStringFromClass([output class]),
};
}

This comment has been minimized.

@slightair

slightair Feb 22, 2016

Member

不要な関数が残っていましたね、すみません。
定数 LogMetadataKeyOutput も不要かと思います、一緒に消していただけると嬉しいです

@slightair

slightair Feb 22, 2016

Member

不要な関数が残っていましたね、すみません。
定数 LogMetadataKeyOutput も不要かと思います、一緒に消していただけると嬉しいです

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

かしこまりました 👍

@chuganzy

chuganzy Feb 22, 2016

Contributor

かしこまりました 👍

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

ついでにSystemDataCollectionNamePrefixも消しました

@chuganzy

chuganzy Feb 22, 2016

Contributor

ついでにSystemDataCollectionNamePrefixも消しました

This comment has been minimized.

@slightair

slightair Feb 23, 2016

Member

ありがとうございますー

@slightair

slightair Feb 23, 2016

Member

ありがとうございますー

Show outdated Hide outdated Pod/Classes/PURBufferedOutput.m
[self.buffer addObjectsFromArray:logs];
if ([self.timer isValid]) {
[self.buffer addObjectsFromArray:logs];
[self flush];

This comment has been minimized.

@slightair

slightair Feb 22, 2016

Member

logStore の再読み込みと書き出しは別にしたいので、 flush はここで呼ばず、元のままにしてもらえませんか?

@slightair

slightair Feb 22, 2016

Member

logStore の再読み込みと書き出しは別にしたいので、 flush はここで呼ばず、元のままにしてもらえませんか?

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

resumestart時にreloadLogStoreしてそのままflushしていたのですが、非同期になった影響でこのようになっています。

確かにおっしゃるとおりここに持たせるのはイケてないかなと思います 😭
reloadLogStoreの引数にhandlerを追加し実行するようにしたらよろしいでしょうか?

@chuganzy

chuganzy Feb 22, 2016

Contributor

resumestart時にreloadLogStoreしてそのままflushしていたのですが、非同期になった影響でこのようになっています。

確かにおっしゃるとおりここに持たせるのはイケてないかなと思います 😭
reloadLogStoreの引数にhandlerを追加し実行するようにしたらよろしいでしょうか?

This comment has been minimized.

@slightair

slightair Feb 22, 2016

Member

あーたしかに。元の実装もダメですね…
handler を追加するのが良さそうです。 🙇

@slightair

slightair Feb 22, 2016

Member

あーたしかに。元の実装もダメですね…
handler を追加するのが良さそうです。 🙇

This comment has been minimized.

@chuganzy

chuganzy Feb 22, 2016

Contributor

かしこまりました 👍

@chuganzy

chuganzy Feb 22, 2016

Contributor

かしこまりました 👍

@slightair

This comment has been minimized.

Show comment
Hide comment
@slightair

slightair Feb 22, 2016

Member

大変長い間PRを放置してしまい、申し訳ありませんでした。
いくつか気になった点をコメントしましたのでよろしくお願いします。

動作確認に Quick/Nimble のアップデートが必要でした。こちらは別途PRを出そうと思っていますが、このPRに含めていただいても構いません。

僕の意図がきちんと伝わらなくても困ってしまうので日本語でコメントしています。コメントしていただける際は 日本語/英語 どちらでもかまいません。よろしくお願いします。

Member

slightair commented Feb 22, 2016

大変長い間PRを放置してしまい、申し訳ありませんでした。
いくつか気になった点をコメントしましたのでよろしくお願いします。

動作確認に Quick/Nimble のアップデートが必要でした。こちらは別途PRを出そうと思っていますが、このPRに含めていただいても構いません。

僕の意図がきちんと伝わらなくても困ってしまうので日本語でコメントしています。コメントしていただける際は 日本語/英語 どちらでもかまいません。よろしくお願いします。

@chuganzy

This comment has been minimized.

Show comment
Hide comment
@chuganzy

chuganzy Feb 22, 2016

Contributor

@slightair レビューありがとうございました。返信と質問をしましたのでご確認お願い致します。返信を頂いてから修正をしようと思っています。よろしくお願いいたしますm(_ _)m

Contributor

chuganzy commented Feb 22, 2016

@slightair レビューありがとうございました。返信と質問をしましたのでご確認お願い致します。返信を頂いてから修正をしようと思っています。よろしくお願いいたしますm(_ _)m

@chuganzy

This comment has been minimized.

Show comment
Hide comment
@chuganzy

chuganzy Feb 22, 2016

Contributor

@slightair コメント頂いた点を一通り修正しましたので再度ご確認をお願いいたしますm(_ _)m

Contributor

chuganzy commented Feb 22, 2016

@slightair コメント頂いた点を一通り修正しましたので再度ご確認をお願いいたしますm(_ _)m

Takeru Chuganji added some commits Feb 22, 2016

@slightair

This comment has been minimized.

Show comment
Hide comment
@slightair

slightair Feb 23, 2016

Member

LGTM
長々と対応ありがとうございました、マージしますね。

なるべくはやめに 2.0.0 としてリリースできるようにします。 🍥

Member

slightair commented Feb 23, 2016

LGTM
長々と対応ありがとうございました、マージしますね。

なるべくはやめに 2.0.0 としてリリースできるようにします。 🍥

slightair added a commit that referenced this pull request Feb 23, 2016

@slightair slightair merged commit ab7ebd2 into cookpad:master Feb 23, 2016

@chuganzy

This comment has been minimized.

Show comment
Hide comment
@chuganzy

chuganzy Feb 23, 2016

Contributor

🍺

Contributor

chuganzy commented Feb 23, 2016

🍺

@chuganzy chuganzy referenced this pull request Mar 15, 2016

Closed

Update YapDatabase #8

@slightair slightair referenced this pull request Nov 1, 2016

Merged

Xcode8 support + #12

@chuganzy chuganzy deleted the chuganzy:performance_fix branch Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment