Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(45)

Issue 3007473002: Make RtcEventLogImpl to use a TaskQueue instead of a helper-thread (Closed)

Created:
3 years, 4 months ago by eladalon
Modified:
3 years, 3 months ago
Reviewers:
terelius, nisse-webrtc
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Make RtcEventLogImpl to use a TaskQueue instead of a helper-thread Make RtcEventLogImpl to use a TaskQueue instead of a helper-thread. This will eventually allow us to run multiple log sessions on a single task-queue. BUG=webrtc:8142, webrtc:8143, webrtc:8145 Review-Url: https://codereview.webrtc.org/3007473002 Cr-Commit-Position: refs/heads/master@{#19666} Committed: https://chromium.googlesource.com/external/webrtc/+/f33cee7534f0469330b6b7aa57f84eec4b082598

Patch Set 1 #

Patch Set 2 : std::atomic_fetch_x returns old value; prepare accordingly. #

Patch Set 3 : Annotate AppendEventToString() with RTC_WARN_UNUSED_RESULT. #

Total comments: 12

Patch Set 4 : Response to comments. #

Total comments: 2

Patch Set 5 : . #

Patch Set 6 : s/ph/pb #

Total comments: 24

Patch Set 7 : Response to comments. #

Patch Set 8 : Fix deps. #

Patch Set 9 : Manual rebase necessary for some reason. #

Patch Set 10 : Update the deps of the right target, actually. #

Patch Set 11 : . #

Patch Set 12 : Well, this was an embarassing omission. #

Patch Set 13 : . #

Total comments: 8

Patch Set 14 : . #

Total comments: 2

Patch Set 15 : . #

Total comments: 1

Patch Set 16 : Fix typo; change phrasing. #

Patch Set 17 : Rebased #

Patch Set 18 : Relax access' threading constraints. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+328 lines, -531 lines) Patch
M webrtc/logging/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +2 lines, -2 lines 0 comments Download
M webrtc/logging/rtc_event_log/rtc_event_log.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +4 lines, -3 lines 0 comments Download
M webrtc/logging/rtc_event_log/rtc_event_log.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 7 chunks +322 lines, -83 lines 0 comments Download
D webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.h View 1 chunk +0 lines, -128 lines 0 comments Download
D webrtc/logging/rtc_event_log/rtc_event_log_helper_thread.cc View 1 chunk +0 lines, -315 lines 0 comments Download

Messages

Total messages: 49 (19 generated)
eladalon
Sneak-peek, if you'd like.
3 years, 4 months ago (2017-08-23 16:05:24 UTC) #3
eladalon
I wanted to add some unit-tests, but I am called away to a higher priority ...
3 years, 4 months ago (2017-08-24 09:23:26 UTC) #4
terelius
https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode77 webrtc/logging/rtc_event_log/rtc_event_log.cc:77: class RtcEvent final : public rtc::QueuedTask { Maybe RtcEventTask ...
3 years, 3 months ago (2017-08-28 13:35:55 UTC) #5
eladalon
https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode77 webrtc/logging/rtc_event_log/rtc_event_log.cc:77: class RtcEvent final : public rtc::QueuedTask { On 2017/08/28 ...
3 years, 3 months ago (2017-08-29 13:36:55 UTC) #6
terelius
lgtm https://codereview.webrtc.org/3007473002/diff/60001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/60001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode672 webrtc/logging/rtc_event_log/rtc_event_log.cc:672: void RtcEventLogImpl::LogEvent(std::unique_ptr<rtclog::Event> event) { nit: Could the name ...
3 years, 3 months ago (2017-08-30 11:26:19 UTC) #7
eladalon
https://codereview.webrtc.org/3007473002/diff/60001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/60001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode672 webrtc/logging/rtc_event_log/rtc_event_log.cc:672: void RtcEventLogImpl::LogEvent(std::unique_ptr<rtclog::Event> event) { On 2017/08/30 11:26:19, terelius wrote: ...
3 years, 3 months ago (2017-08-30 11:43:33 UTC) #8
eladalon
Niels, could you please also take a look?
3 years, 3 months ago (2017-08-30 11:43:59 UTC) #10
nisse-webrtc
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode77 webrtc/logging/rtc_event_log/rtc_event_log.cc:77: class RtcEventLogTask final : public rtc::QueuedTask { You could ...
3 years, 3 months ago (2017-08-30 12:18:36 UTC) #11
eladalon
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode77 webrtc/logging/rtc_event_log/rtc_event_log.cc:77: class RtcEventLogTask final : public rtc::QueuedTask { On 2017/08/30 ...
3 years, 3 months ago (2017-08-30 13:01:19 UTC) #12
nisse-webrtc
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode169 webrtc/logging/rtc_event_log/rtc_event_log.cc:169: ProtoString* output_string) RTC_WARN_UNUSED_RESULT; On 2017/08/30 13:01:18, eladalon wrote: > ...
3 years, 3 months ago (2017-08-30 13:13:23 UTC) #13
eladalon
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode169 webrtc/logging/rtc_event_log/rtc_event_log.cc:169: ProtoString* output_string) RTC_WARN_UNUSED_RESULT; On 2017/08/30 13:13:23, nisse-webrtc wrote: > ...
3 years, 3 months ago (2017-08-30 14:15:45 UTC) #14
nisse-webrtc
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode169 webrtc/logging/rtc_event_log/rtc_event_log.cc:169: ProtoString* output_string) RTC_WARN_UNUSED_RESULT; On 2017/08/30 14:15:44, eladalon wrote: > ...
3 years, 3 months ago (2017-08-30 14:27:01 UTC) #15
terelius
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode169 webrtc/logging/rtc_event_log/rtc_event_log.cc:169: ProtoString* output_string) RTC_WARN_UNUSED_RESULT; On 2017/08/30 14:27:01, nisse-webrtc wrote: > ...
3 years, 3 months ago (2017-08-30 15:06:04 UTC) #16
eladalon
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode169 webrtc/logging/rtc_event_log/rtc_event_log.cc:169: ProtoString* output_string) RTC_WARN_UNUSED_RESULT; On 2017/08/30 15:06:04, terelius wrote: > ...
3 years, 3 months ago (2017-08-30 15:19:20 UTC) #17
nisse-webrtc
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode674 webrtc/logging/rtc_event_log/rtc_event_log.cc:674: task_queue_.PostTask(std::move(task)); On 2017/08/30 15:19:20, eladalon wrote: > On 2017/08/30 ...
3 years, 3 months ago (2017-08-31 06:53:15 UTC) #18
eladalon
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode674 webrtc/logging/rtc_event_log/rtc_event_log.cc:674: task_queue_.PostTask(std::move(task)); On 2017/08/31 06:53:15, nisse-webrtc wrote: > On 2017/08/30 ...
3 years, 3 months ago (2017-08-31 09:05:49 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3007473002/260001
3 years, 3 months ago (2017-08-31 11:54:55 UTC) #22
nisse-webrtc
Looks pretty good now. I'm afraid I have one more suggestion this round. https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event_log/rtc_event_log.cc File ...
3 years, 3 months ago (2017-08-31 12:09:24 UTC) #24
eladalon
https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode76 webrtc/logging/rtc_event_log/rtc_event_log.cc:76: // a lambda (a copy constructor is required). On ...
3 years, 3 months ago (2017-08-31 12:31:32 UTC) #26
nisse-webrtc
lgtm https://codereview.webrtc.org/3007473002/diff/280001/webrtc/logging/rtc_event_log/rtc_event_log.cc File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/280001/webrtc/logging/rtc_event_log/rtc_event_log.cc#newcode77 webrtc/logging/rtc_event_log/rtc_event_log.cc:77: // rid of this ocne we've moved to ...
3 years, 3 months ago (2017-08-31 12:33:44 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3007473002/300001
3 years, 3 months ago (2017-08-31 12:34:25 UTC) #30
commit-bot: I haz the power
Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/builds/7466)
3 years, 3 months ago (2017-08-31 13:00:15 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3007473002/300001
3 years, 3 months ago (2017-08-31 13:35:06 UTC) #34
commit-bot: I haz the power
Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/builds/7572)
3 years, 3 months ago (2017-08-31 14:12:16 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3007473002/320001
3 years, 3 months ago (2017-09-04 09:22:18 UTC) #39
commit-bot: I haz the power
Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/builds/7573)
3 years, 3 months ago (2017-09-04 10:08:08 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3007473002/340001
3 years, 3 months ago (2017-09-04 12:22:42 UTC) #44
commit-bot: I haz the power
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/external/webrtc/+/f33cee7534f0469330b6b7aa57f84eec4b082598
3 years, 3 months ago (2017-09-04 14:26:22 UTC) #47
charujain
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.webrtc.org/3010143002/ by charujain@webrtc.org. ...
3 years, 3 months ago (2017-09-04 18:06:07 UTC) #48
eladalon
3 years, 3 months ago (2017-09-05 12:11:02 UTC) #49
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.webrtc.org/3010163002/ by eladalon@webrtc.org.

The reason for reverting is: I will fix and reland..

Powered by Google App Engine
This is Rietveld 408576698