Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3007473002:
Make RtcEventLogImpl to use a TaskQueue instead of a helper-thread (Closed)
DescriptionMake 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. #
Messages
Total messages: 49 (19 generated)
Description was changed from ========== Make RtcEventLogImpl to use a TaskQueue instead of a helper-thread TODO: Document why this was done, and what small bugs were fixed along the way. BUG=webrtc:8142, webrtc:8143, webrtc:8145 ========== to ========== Make RtcEventLogImpl to use a TaskQueue instead of a helper-thread TODO: Document why this was done, and what small bugs were fixed along the way. BUG=webrtc:8142, webrtc:8143, webrtc:8145 ==========
eladalon@webrtc.org changed reviewers: + terelius@webrtc.org
Sneak-peek, if you'd like.
I wanted to add some unit-tests, but I am called away to a higher priority task. I guess we can add those later. PTAL? https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:710: // TODO(eladalon): !!! This time precedes the timestamps in history_. Is that Björn, what do you think? https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:729: // TODO(eladalon): !!! Do we really want to pop before the write to Björn, what do you think?
https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:77: class RtcEvent final : public rtc::QueuedTask { Maybe RtcEventTask or RtcEventLogTask to make the name less similar to rtc::Event? https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:630: task_queue_.PostTask([this, start_time, max_size_bytes, file_raw]() { Are tasks guaranteed to be executed or is there any chance that we leak the FileWrapper memory and an open file descriptor?? What happens if we delete the event log and/or taskqueue? https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:683: // Swap the event back so that we don't mix event types in the queues. I still think we should do the Swap(), but this comment is outdated. (At some point we had separate queues for each event type.) https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:710: // TODO(eladalon): !!! This time precedes the timestamps in history_. Is that On 2017/08/24 09:23:25, eladalon wrote: > Björn, what do you think? You mean that it is written to file before serializing the older events in history_? Encapsulating each log segment in a LOG_START ... LOG_END pair actually simplifies parsing in the cases where we only want to look at one segment. The next time we change the log format we should consider adding some extra information to LOG_START, to avoid this implicit dependency on the order of events in the file. Please keep this as-is for now. https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:729: // TODO(eladalon): !!! Do we really want to pop before the write to On 2017/08/24 09:23:26, eladalon wrote: > Björn, what do you think? I don't think this matters one way or the other. The only way this will have any measurable effect is if the write fails despite not reaching the file limit, and we start logging again within ~10 seconds without having received any notification that the old log has stopped, and the new log actually can write to the file. I'm not sure under what circumstances this would actually happen. Feel free to change this behavior if you see a clean way to do so. However, I'm more worried that future maintenance and refactorings will cause duplicated events in the log if we separate processing the events in history and removing the processed events.
https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:77: class RtcEvent final : public rtc::QueuedTask { On 2017/08/28 13:35:55, terelius wrote: > Maybe RtcEventTask or RtcEventLogTask to make the name less similar to > rtc::Event? Done. https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:630: task_queue_.PostTask([this, start_time, max_size_bytes, file_raw]() { On 2017/08/28 13:35:55, terelius wrote: > Are tasks guaranteed to be executed or is there any chance that we leak the > FileWrapper memory and an open file descriptor?? What happens if we delete the > event log and/or taskqueue? You're right in theory (thanks for catching!), because TaskQueue's implementation says pending tasks get deleted when the destructor is invoked. So I'll handle this in the way I'd handled the events - create a class for it. In practice, at least on my Mac, the destructor seems to block until the task queue becomes empty (which means the raw-pointer can never leak). This, however, seems like a bug that we should fix. Please see this CL for the demonstration of the bug in action: https://codereview.webrtc.org/3009763002 https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:683: // Swap the event back so that we don't mix event types in the queues. On 2017/08/28 13:35:55, terelius wrote: > I still think we should do the Swap(), but this comment is outdated. (At some > point we had separate queues for each event type.) I've freshened up the comments. https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:710: // TODO(eladalon): !!! This time precedes the timestamps in history_. Is that On 2017/08/28 13:35:55, terelius wrote: > On 2017/08/24 09:23:25, eladalon wrote: > > Björn, what do you think? > > You mean that it is written to file before serializing the older events in > history_? Encapsulating each log segment in a LOG_START ... LOG_END pair > actually simplifies parsing in the cases where we only want to look at one > segment. The next time we change the log format we should consider adding some > extra information to LOG_START, to avoid this implicit dependency on the order > of events in the file. > > Please keep this as-is for now. For posterity - we've discussed this some more offline. For a while, it sounded as if taking the oldest event's timestamp would be the way to go, but ended up deciding that would 1. not be very meaningful, 2. it wouldn't be clear if we should take the oldest event or the oldest config-event, and 3. the log's timestamps are not monotonic anyway. https://codereview.webrtc.org/3007473002/diff/40001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:729: // TODO(eladalon): !!! Do we really want to pop before the write to On 2017/08/28 13:35:54, terelius wrote: > On 2017/08/24 09:23:26, eladalon wrote: > > Björn, what do you think? > > I don't think this matters one way or the other. The only way this will have any > measurable effect is if the write fails despite not reaching the file limit, and > we start logging again within ~10 seconds without having received any > notification that the old log has stopped, and the new log actually can write to > the file. I'm not sure under what circumstances this would actually happen. > > Feel free to change this behavior if you see a clean way to do so. However, I'm > more worried that future maintenance and refactorings will cause duplicated > events in the log if we separate processing the events in history and removing > the processed events. I can document it as a known-potential-unlikely-issue. I think I'll do that, because writing code to handle this edge-case would require unit-tests simulating this edge-case, and this seems like a poor investment of our time.
lgtm https://codereview.webrtc.org/3007473002/diff/60001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/60001/webrtc/logging/rtc_event_... webrtc/logging/rtc_event_log/rtc_event_log.cc:672: void RtcEventLogImpl::LogEvent(std::unique_ptr<rtclog::Event> event) { nit: Could the name be made clearer and less similar to other functions, e.g. StoreEvent and LogRtpHeader? Maybe ProcessEventOnTaskQueue or LogEventToFileOrMemory? WDYT?
https://codereview.webrtc.org/3007473002/diff/60001/webrtc/logging/rtc_event_... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/60001/webrtc/logging/rtc_event_... 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: > nit: Could the name be made clearer and less similar to other functions, e.g. > StoreEvent and LogRtpHeader? Maybe ProcessEventOnTaskQueue or > LogEventToFileOrMemory? WDYT? I'll move this code directly into where the function is used - it's simple enough, and we'll avoid naming-related head-scratching. :-)
eladalon@webrtc.org changed reviewers: + nisse@webrtc.org
Niels, could you please also take a look?
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:77: class RtcEventLogTask final : public rtc::QueuedTask { You could templateize this and the below class, on the type the unique_ptr refers to. Not sure if it would improve readability, but in case the type can be inferred automatically when it is used, it might be worth a try. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:167: // the limitation disallows appending the event.) Suggested rephrasing of this comment: Appends an event to the output protobuf string, returning true on success. Fails and returns false in case the limit out output size prevents the event from being added; in this case, the output string is left unchanged. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:169: ProtoString* output_string) RTC_WARN_UNUSED_RESULT; How do you expect callers to handle errors? If there's nothing useful they can do, you shouldn't use RTC_WARN_UNUSED_RESULT. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:178: static std::atomic<int> log_count_; I'm not sure use of std::atomic is allowed yet. And I think log_count_ deserves a comment on its purpose. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:674: task_queue_.PostTask(std::move(task)); Now you set this up in three steps: 1. named lambda event_handler (btw, named lambda is an odd construction; C++ ought to let us declare a local function instead...) 2. Wrap the lambda in a RtcEventLogTask, with yet another local variable. 3. Pass it to PostTask. I think it will be clearer to merge the second two, something like task_queue_.PostTask(rtc::MakeUnique<RtcEventLogTask>( std::move(event), event_handler)); And I'd consider merging all three, inlining the definition of the lambda where that is used. But I guess that won't suite everyone's taste.
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:77: class RtcEventLogTask final : public rtc::QueuedTask { On 2017/08/30 12:18:35, nisse-webrtc wrote: > You could templateize this and the below class, on the type the unique_ptr > refers to. > > Not sure if it would improve readability, but in case the type can be inferred > automatically when it is used, it might be worth a try. Good suggestion - I think it would actually, oddly enough for a template, improve readability, because it would disconnect the actual type (file/event) from the fact that this class just exists to manage its lifetime. Done. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:167: // the limitation disallows appending the event.) On 2017/08/30 12:18:35, nisse-webrtc wrote: > Suggested rephrasing of this comment: > > Appends an event to the output protobuf string, returning true on > success. Fails and returns false in case the limit out output size > prevents the event from being added; in this case, the output string > is left unchanged. Done. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:169: ProtoString* output_string) RTC_WARN_UNUSED_RESULT; On 2017/08/30 12:18:36, nisse-webrtc wrote: > How do you expect callers to handle errors? > > If there's nothing useful they can do, you shouldn't use RTC_WARN_UNUSED_RESULT. This is not an error, this is an indication that an event was not written due to lack of space. Before, some call-sites of this function checked the return value and stopped on the first failure, but others failed to check the return-code, which could result in skipped events in the log. That is, given [event0, event1, event2], the log-file could end up containing only event0 and event2, because event1 happened to not fit, but event2 did. I've fixed that, and added this annotation, so as to reduce this likelihood of such bugs re-appearing. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:178: static std::atomic<int> log_count_; On 2017/08/30 12:18:35, nisse-webrtc wrote: > I'm not sure use of std::atomic is allowed yet. > > And I think log_count_ deserves a comment on its purpose. 1. I think it's allowed. I'll send you a relevant link OOB. 2. I'll add the comment. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:674: task_queue_.PostTask(std::move(task)); On 2017/08/30 12:18:35, nisse-webrtc wrote: > Now you set this up in three steps: > > 1. named lambda event_handler (btw, named lambda is an odd construction; C++ > ought to let us declare a local function instead...) > > 2. Wrap the lambda in a RtcEventLogTask, with yet another local variable. > > 3. Pass it to PostTask. > > I think it will be clearer to merge the second two, something like > > task_queue_.PostTask(rtc::MakeUnique<RtcEventLogTask>( > std::move(event), event_handler)); > > And I'd consider merging all three, inlining the definition of the lambda where > that is used. But I guess that won't suite everyone's taste. We have to have RtcEventLogTask (now named ResourceOwningTask), because a lambda cannot capture a unique_ptr. We can't use a raw pointer, because if the task is ever destructed without being executed, that would be a memory leak (but the solution I have in place would not leak memory, because the destructor would still be called and properly release memory). As for naming lambdas, I personally like this, and use it often. I'd like to keep doing that, at least unless there's a reason to suspect it might end up with less efficient machine-code. But speaking of efficiency - I used ResourceOwningTask::handler_ because I did not want to make ResourceOwningTask a friend of RtcEventLogImpl. But maybe that's too inefficient. I'll push the CL as it is, with these comments. We can then think about modifying this more.
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... 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: > On 2017/08/30 12:18:36, nisse-webrtc wrote: > > How do you expect callers to handle errors? > > > > If there's nothing useful they can do, you shouldn't use > RTC_WARN_UNUSED_RESULT. > > This is not an error, this is an indication that an event was not written due to > lack of space. Before, some call-sites of this function checked the return value > and stopped on the first failure, but others failed to check the return-code, > which could result in skipped events in the log. That is, given [event0, event1, > event2], the log-file could end up containing only event0 and event2, because > event1 happened to not fit, but event2 did. I've fixed that, and added this > annotation, so as to reduce this likelihood of such bugs re-appearing. You could move that responsibility to this class, with a flag bool log_is_full_go_away_; to ensure that if AppendEventToString ever returns false, it will continue to return false until the end of time. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:178: static std::atomic<int> log_count_; On 2017/08/30 13:01:18, eladalon wrote: > On 2017/08/30 12:18:35, nisse-webrtc wrote: > > I'm not sure use of std::atomic is allowed yet. > > > > And I think log_count_ deserves a comment on its purpose. > > 1. I think it's allowed. I'll send you a relevant link OOB. > 2. I'll add the comment. I've checked the Chromium style guide, it's allowed according to https://chromium-cpp.appspot.com/. So this is fine. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:674: task_queue_.PostTask(std::move(task)); On 2017/08/30 13:01:19, eladalon wrote: > On 2017/08/30 12:18:35, nisse-webrtc wrote: > > Now you set this up in three steps: > > > > 1. named lambda event_handler (btw, named lambda is an odd construction; C++ > > ought to let us declare a local function instead...) > > > > 2. Wrap the lambda in a RtcEventLogTask, with yet another local variable. > > > > 3. Pass it to PostTask. > > > > I think it will be clearer to merge the second two, something like > > > > task_queue_.PostTask(rtc::MakeUnique<RtcEventLogTask>( > > std::move(event), event_handler)); > > > > And I'd consider merging all three, inlining the definition of the lambda > where > > that is used. But I guess that won't suite everyone's taste. > > We have to have RtcEventLogTask (now named ResourceOwningTask), because a lambda > cannot capture a unique_ptr. We can't use a raw pointer, because if the task is > ever destructed without being executed, that would be a memory leak (but the > solution I have in place would not leak memory, because the destructor would > still be called and properly release memory). > > As for naming lambdas, I personally like this, and use it often. I'd like to > keep doing that, at least unless there's a reason to suspect it might end up > with less efficient machine-code. > > But speaking of efficiency - I used ResourceOwningTask::handler_ because I did > not want to make ResourceOwningTask a friend of RtcEventLogImpl. But maybe > that's too inefficient. I'll push the CL as it is, with these comments. We can > then think about modifying this more. ResourceOwningTask is fine, I was merely suggesting folding the expression to construct it into the expression calling to PostTask.
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... 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: > On 2017/08/30 13:01:18, eladalon wrote: > > On 2017/08/30 12:18:36, nisse-webrtc wrote: > > > How do you expect callers to handle errors? > > > > > > If there's nothing useful they can do, you shouldn't use > > RTC_WARN_UNUSED_RESULT. > > > > This is not an error, this is an indication that an event was not written due > to > > lack of space. Before, some call-sites of this function checked the return > value > > and stopped on the first failure, but others failed to check the return-code, > > which could result in skipped events in the log. That is, given [event0, > event1, > > event2], the log-file could end up containing only event0 and event2, because > > event1 happened to not fit, but event2 did. I've fixed that, and added this > > annotation, so as to reduce this likelihood of such bugs re-appearing. > > You could move that responsibility to this class, with a flag > > bool log_is_full_go_away_; > > to ensure that if AppendEventToString ever returns false, it will continue to > return false until the end of time. It would indeed be better, because now we still have the problem that future calls to StoreEvent() would not remember that a previous call has failed. But this is a bit more of a departure from the original CL, and I've already piggybacked too many changes on this CL, which was originally only intended to migrate from a helper-thread to a task queue. So I would prefer to handle this in a later CL. Wdyt? https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:674: task_queue_.PostTask(std::move(task)); On 2017/08/30 13:13:23, nisse-webrtc wrote: > On 2017/08/30 13:01:19, eladalon wrote: > > On 2017/08/30 12:18:35, nisse-webrtc wrote: > > > Now you set this up in three steps: > > > > > > 1. named lambda event_handler (btw, named lambda is an odd construction; C++ > > > ought to let us declare a local function instead...) > > > > > > 2. Wrap the lambda in a RtcEventLogTask, with yet another local variable. > > > > > > 3. Pass it to PostTask. > > > > > > I think it will be clearer to merge the second two, something like > > > > > > task_queue_.PostTask(rtc::MakeUnique<RtcEventLogTask>( > > > std::move(event), event_handler)); > > > > > > And I'd consider merging all three, inlining the definition of the lambda > > where > > > that is used. But I guess that won't suite everyone's taste. > > > > We have to have RtcEventLogTask (now named ResourceOwningTask), because a > lambda > > cannot capture a unique_ptr. We can't use a raw pointer, because if the task > is > > ever destructed without being executed, that would be a memory leak (but the > > solution I have in place would not leak memory, because the destructor would > > still be called and properly release memory). > > > > As for naming lambdas, I personally like this, and use it often. I'd like to > > keep doing that, at least unless there's a reason to suspect it might end up > > with less efficient machine-code. > > > > But speaking of efficiency - I used ResourceOwningTask::handler_ because I did > > not want to make ResourceOwningTask a friend of RtcEventLogImpl. But maybe > > that's too inefficient. I'll push the CL as it is, with these comments. We can > > then think about modifying this more. > > ResourceOwningTask is fine, I was merely suggesting folding the expression to > construct it into the expression calling to PostTask. I see two issues: 1. The nit - that the lambda is named. I prefer this style. 2. That actual issue - that the lambda is another layer of indirection, which could be avoided. I think that's preferable to the alternative of defining two classes inside of RtcEventLogImpl and having both access RtcEventLogImpl's internals.
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... 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: > On 2017/08/30 13:13:23, nisse-webrtc wrote: > > On 2017/08/30 13:01:18, eladalon wrote: > > > On 2017/08/30 12:18:36, nisse-webrtc wrote: > > > > How do you expect callers to handle errors? > > > > > > > > If there's nothing useful they can do, you shouldn't use > > > RTC_WARN_UNUSED_RESULT. > > > > > > This is not an error, this is an indication that an event was not written > due > > to > > > lack of space. Before, some call-sites of this function checked the return > > value > > > and stopped on the first failure, but others failed to check the > return-code, > > > which could result in skipped events in the log. That is, given [event0, > > event1, > > > event2], the log-file could end up containing only event0 and event2, > because > > > event1 happened to not fit, but event2 did. I've fixed that, and added this > > > annotation, so as to reduce this likelihood of such bugs re-appearing. > > > > You could move that responsibility to this class, with a flag > > > > bool log_is_full_go_away_; > > > > to ensure that if AppendEventToString ever returns false, it will continue to > > return false until the end of time. > > It would indeed be better, because now we still have the problem that future > calls to StoreEvent() would not remember that a previous call has failed. But > this is a bit more of a departure from the original CL, and I've already > piggybacked too many changes on this CL, which was originally only intended to > migrate from a helper-thread to a task queue. So I would prefer to handle this > in a later CL. Wdyt? It's just that I don't think RTC_WARN_UNUSED_RESULT should be used lightly. But I'll defer to Björn about division of work into cls. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:674: task_queue_.PostTask(std::move(task)); On 2017/08/30 14:15:44, eladalon wrote: > On 2017/08/30 13:13:23, nisse-webrtc wrote: > > On 2017/08/30 13:01:19, eladalon wrote: > > > On 2017/08/30 12:18:35, nisse-webrtc wrote: > > > > Now you set this up in three steps: > > > > > > > > 1. named lambda event_handler (btw, named lambda is an odd construction; > C++ > > > > ought to let us declare a local function instead...) > > > > > > > > 2. Wrap the lambda in a RtcEventLogTask, with yet another local variable. > > > > > > > > 3. Pass it to PostTask. > > > > > > > > I think it will be clearer to merge the second two, something like > > > > > > > > task_queue_.PostTask(rtc::MakeUnique<RtcEventLogTask>( > > > > std::move(event), event_handler)); > > > > > > > > And I'd consider merging all three, inlining the definition of the lambda > > > where > > > > that is used. But I guess that won't suite everyone's taste. > > > > > > We have to have RtcEventLogTask (now named ResourceOwningTask), because a > > lambda > > > cannot capture a unique_ptr. We can't use a raw pointer, because if the task > > is > > > ever destructed without being executed, that would be a memory leak (but the > > > solution I have in place would not leak memory, because the destructor would > > > still be called and properly release memory). > > > > > > As for naming lambdas, I personally like this, and use it often. I'd like to > > > keep doing that, at least unless there's a reason to suspect it might end up > > > with less efficient machine-code. > > > > > > But speaking of efficiency - I used ResourceOwningTask::handler_ because I > did > > > not want to make ResourceOwningTask a friend of RtcEventLogImpl. But maybe > > > that's too inefficient. I'll push the CL as it is, with these comments. We > can > > > then think about modifying this more. > > > > ResourceOwningTask is fine, I was merely suggesting folding the expression to > > construct it into the expression calling to PostTask. > > I see two issues: > 1. The nit - that the lambda is named. I prefer this style. > 2. That actual issue - that the lambda is another layer of indirection, which > could be avoided. I think that's preferable to the alternative of defining two > classes inside of RtcEventLogImpl and having both access RtcEventLogImpl's > internals. I'm not talking of either of those... I would like: 1. WrapUnique replaced by MakeUnique. 2. The local variable |task| eliminated, and it's value substituted where it is used.
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... 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: > On 2017/08/30 14:15:44, eladalon wrote: > > On 2017/08/30 13:13:23, nisse-webrtc wrote: > > > On 2017/08/30 13:01:18, eladalon wrote: > > > > On 2017/08/30 12:18:36, nisse-webrtc wrote: > > > > > How do you expect callers to handle errors? > > > > > > > > > > If there's nothing useful they can do, you shouldn't use > > > > RTC_WARN_UNUSED_RESULT. > > > > > > > > This is not an error, this is an indication that an event was not written > > due > > > to > > > > lack of space. Before, some call-sites of this function checked the return > > > value > > > > and stopped on the first failure, but others failed to check the > > return-code, > > > > which could result in skipped events in the log. That is, given [event0, > > > event1, > > > > event2], the log-file could end up containing only event0 and event2, > > because > > > > event1 happened to not fit, but event2 did. I've fixed that, and added > this > > > > annotation, so as to reduce this likelihood of such bugs re-appearing. > > > > > > You could move that responsibility to this class, with a flag > > > > > > bool log_is_full_go_away_; > > > > > > to ensure that if AppendEventToString ever returns false, it will continue > to > > > return false until the end of time. > > > > It would indeed be better, because now we still have the problem that future > > calls to StoreEvent() would not remember that a previous call has failed. But > > this is a bit more of a departure from the original CL, and I've already > > piggybacked too many changes on this CL, which was originally only intended to > > migrate from a helper-thread to a task queue. So I would prefer to handle this > > in a later CL. Wdyt? > > It's just that I don't think RTC_WARN_UNUSED_RESULT should be used lightly. But > I'll defer to Björn about division of work into cls. Nisse, I think it is fine to land this CL as-is. There are several implementation changes planned for the future and we'll keep your objection in mind for the coming CLs. Elad, I don't understand your comment about StoreEvent. This function is not used by StoreEvent. More importantly, the log stops if AppendEventToString ever returns false. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:787: if (!appended) { This is nitpicking, but you are losing the event for future logs if it doesn't fit in the file.
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... 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: > On 2017/08/30 14:27:01, nisse-webrtc wrote: > > On 2017/08/30 14:15:44, eladalon wrote: > > > On 2017/08/30 13:13:23, nisse-webrtc wrote: > > > > On 2017/08/30 13:01:18, eladalon wrote: > > > > > On 2017/08/30 12:18:36, nisse-webrtc wrote: > > > > > > How do you expect callers to handle errors? > > > > > > > > > > > > If there's nothing useful they can do, you shouldn't use > > > > > RTC_WARN_UNUSED_RESULT. > > > > > > > > > > This is not an error, this is an indication that an event was not > written > > > due > > > > to > > > > > lack of space. Before, some call-sites of this function checked the > return > > > > value > > > > > and stopped on the first failure, but others failed to check the > > > return-code, > > > > > which could result in skipped events in the log. That is, given [event0, > > > > event1, > > > > > event2], the log-file could end up containing only event0 and event2, > > > because > > > > > event1 happened to not fit, but event2 did. I've fixed that, and added > > this > > > > > annotation, so as to reduce this likelihood of such bugs re-appearing. > > > > > > > > You could move that responsibility to this class, with a flag > > > > > > > > bool log_is_full_go_away_; > > > > > > > > to ensure that if AppendEventToString ever returns false, it will continue > > to > > > > return false until the end of time. > > > > > > It would indeed be better, because now we still have the problem that future > > > calls to StoreEvent() would not remember that a previous call has failed. > But > > > this is a bit more of a departure from the original CL, and I've already > > > piggybacked too many changes on this CL, which was originally only intended > to > > > migrate from a helper-thread to a task queue. So I would prefer to handle > this > > > in a later CL. Wdyt? > > > > It's just that I don't think RTC_WARN_UNUSED_RESULT should be used lightly. > But > > I'll defer to Björn about division of work into cls. > > Nisse, I think it is fine to land this CL as-is. There are several > implementation changes planned for the future and we'll keep your objection in > mind for the coming CLs. > > Elad, I don't understand your comment about StoreEvent. This function is not > used by StoreEvent. More importantly, the log stops if AppendEventToString ever > returns false. 1. StoreEvent -> LogToFile -> AppendEventToString 2. Yes, you're right - the file is closed when this fails. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:674: task_queue_.PostTask(std::move(task)); On 2017/08/30 14:27:01, nisse-webrtc wrote: > On 2017/08/30 14:15:44, eladalon wrote: > > On 2017/08/30 13:13:23, nisse-webrtc wrote: > > > On 2017/08/30 13:01:19, eladalon wrote: > > > > On 2017/08/30 12:18:35, nisse-webrtc wrote: > > > > > Now you set this up in three steps: > > > > > > > > > > 1. named lambda event_handler (btw, named lambda is an odd construction; > > C++ > > > > > ought to let us declare a local function instead...) > > > > > > > > > > 2. Wrap the lambda in a RtcEventLogTask, with yet another local > variable. > > > > > > > > > > 3. Pass it to PostTask. > > > > > > > > > > I think it will be clearer to merge the second two, something like > > > > > > > > > > task_queue_.PostTask(rtc::MakeUnique<RtcEventLogTask>( > > > > > std::move(event), event_handler)); > > > > > > > > > > And I'd consider merging all three, inlining the definition of the > lambda > > > > where > > > > > that is used. But I guess that won't suite everyone's taste. > > > > > > > > We have to have RtcEventLogTask (now named ResourceOwningTask), because a > > > lambda > > > > cannot capture a unique_ptr. We can't use a raw pointer, because if the > task > > > is > > > > ever destructed without being executed, that would be a memory leak (but > the > > > > solution I have in place would not leak memory, because the destructor > would > > > > still be called and properly release memory). > > > > > > > > As for naming lambdas, I personally like this, and use it often. I'd like > to > > > > keep doing that, at least unless there's a reason to suspect it might end > up > > > > with less efficient machine-code. > > > > > > > > But speaking of efficiency - I used ResourceOwningTask::handler_ because I > > did > > > > not want to make ResourceOwningTask a friend of RtcEventLogImpl. But maybe > > > > that's too inefficient. I'll push the CL as it is, with these comments. We > > can > > > > then think about modifying this more. > > > > > > ResourceOwningTask is fine, I was merely suggesting folding the expression > to > > > construct it into the expression calling to PostTask. > > > > I see two issues: > > 1. The nit - that the lambda is named. I prefer this style. > > 2. That actual issue - that the lambda is another layer of indirection, which > > could be avoided. I think that's preferable to the alternative of defining two > > classes inside of RtcEventLogImpl and having both access RtcEventLogImpl's > > internals. > > I'm not talking of either of those... I would like: > > 1. WrapUnique replaced by MakeUnique. > > 2. The local variable |task| eliminated, and it's value substituted where it is > used. 1. We can do that, because unique_ptr<Derived> is not implicitly convertible to unique_ptr<Base>. 2. I think readability suffers if we do this, with no gain in efficiency to justify this. https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:787: if (!appended) { On 2017/08/30 15:06:04, terelius wrote: > This is nitpicking, but you are losing the event for future logs if it doesn't > fit in the file. Done.
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... 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 14:27:01, nisse-webrtc wrote: > > On 2017/08/30 14:15:44, eladalon wrote: > > > On 2017/08/30 13:13:23, nisse-webrtc wrote: > > > > On 2017/08/30 13:01:19, eladalon wrote: > > > > > On 2017/08/30 12:18:35, nisse-webrtc wrote: > > > > > > Now you set this up in three steps: > > > > > > > > > > > > 1. named lambda event_handler (btw, named lambda is an odd > construction; > > > C++ > > > > > > ought to let us declare a local function instead...) > > > > > > > > > > > > 2. Wrap the lambda in a RtcEventLogTask, with yet another local > > variable. > > > > > > > > > > > > 3. Pass it to PostTask. > > > > > > > > > > > > I think it will be clearer to merge the second two, something like > > > > > > > > > > > > task_queue_.PostTask(rtc::MakeUnique<RtcEventLogTask>( > > > > > > std::move(event), event_handler)); > > > > > > > > > > > > And I'd consider merging all three, inlining the definition of the > > lambda > > > > > where > > > > > > that is used. But I guess that won't suite everyone's taste. > > > > > > > > > > We have to have RtcEventLogTask (now named ResourceOwningTask), because > a > > > > lambda > > > > > cannot capture a unique_ptr. We can't use a raw pointer, because if the > > task > > > > is > > > > > ever destructed without being executed, that would be a memory leak (but > > the > > > > > solution I have in place would not leak memory, because the destructor > > would > > > > > still be called and properly release memory). > > > > > > > > > > As for naming lambdas, I personally like this, and use it often. I'd > like > > to > > > > > keep doing that, at least unless there's a reason to suspect it might > end > > up > > > > > with less efficient machine-code. > > > > > > > > > > But speaking of efficiency - I used ResourceOwningTask::handler_ because > I > > > did > > > > > not want to make ResourceOwningTask a friend of RtcEventLogImpl. But > maybe > > > > > that's too inefficient. I'll push the CL as it is, with these comments. > We > > > can > > > > > then think about modifying this more. > > > > > > > > ResourceOwningTask is fine, I was merely suggesting folding the expression > > to > > > > construct it into the expression calling to PostTask. > > > > > > I see two issues: > > > 1. The nit - that the lambda is named. I prefer this style. > > > 2. That actual issue - that the lambda is another layer of indirection, > which > > > could be avoided. I think that's preferable to the alternative of defining > two > > > classes inside of RtcEventLogImpl and having both access RtcEventLogImpl's > > > internals. > > > > I'm not talking of either of those... I would like: > > > > 1. WrapUnique replaced by MakeUnique. > > > > 2. The local variable |task| eliminated, and it's value substituted where it > is > > used. > > 1. We can do that, because unique_ptr<Derived> is not implicitly convertible to > unique_ptr<Base>. I'd expect the unique_ptr to support implicit conversion whenever the underlying raw pointers do. Otherwise it's not a smart pointer, but a stupid pointer... From http://en.cppreference.com/w/cpp/memory/unique_ptr: "If T is a derived class of some base B, then std::unique_ptr<T> is implicitly convertible to std::unique_ptr<B>. " WrapUnique is intended only for rare cases, like working with old raw-pointer apis, or with private constructors. I think it's fairly important to not introduce needlessly. > 2. I think readability suffers if we do this, with no gain in efficiency to > justify this. I disagree on readability, but I won't insist on this one.
https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/100001/webrtc/logging/rtc_event... 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 15:19:20, eladalon wrote: > > On 2017/08/30 14:27:01, nisse-webrtc wrote: > > > On 2017/08/30 14:15:44, eladalon wrote: > > > > On 2017/08/30 13:13:23, nisse-webrtc wrote: > > > > > On 2017/08/30 13:01:19, eladalon wrote: > > > > > > On 2017/08/30 12:18:35, nisse-webrtc wrote: > > > > > > > Now you set this up in three steps: > > > > > > > > > > > > > > 1. named lambda event_handler (btw, named lambda is an odd > > construction; > > > > C++ > > > > > > > ought to let us declare a local function instead...) > > > > > > > > > > > > > > 2. Wrap the lambda in a RtcEventLogTask, with yet another local > > > variable. > > > > > > > > > > > > > > 3. Pass it to PostTask. > > > > > > > > > > > > > > I think it will be clearer to merge the second two, something like > > > > > > > > > > > > > > task_queue_.PostTask(rtc::MakeUnique<RtcEventLogTask>( > > > > > > > std::move(event), event_handler)); > > > > > > > > > > > > > > And I'd consider merging all three, inlining the definition of the > > > lambda > > > > > > where > > > > > > > that is used. But I guess that won't suite everyone's taste. > > > > > > > > > > > > We have to have RtcEventLogTask (now named ResourceOwningTask), > because > > a > > > > > lambda > > > > > > cannot capture a unique_ptr. We can't use a raw pointer, because if > the > > > task > > > > > is > > > > > > ever destructed without being executed, that would be a memory leak > (but > > > the > > > > > > solution I have in place would not leak memory, because the destructor > > > would > > > > > > still be called and properly release memory). > > > > > > > > > > > > As for naming lambdas, I personally like this, and use it often. I'd > > like > > > to > > > > > > keep doing that, at least unless there's a reason to suspect it might > > end > > > up > > > > > > with less efficient machine-code. > > > > > > > > > > > > But speaking of efficiency - I used ResourceOwningTask::handler_ > because > > I > > > > did > > > > > > not want to make ResourceOwningTask a friend of RtcEventLogImpl. But > > maybe > > > > > > that's too inefficient. I'll push the CL as it is, with these > comments. > > We > > > > can > > > > > > then think about modifying this more. > > > > > > > > > > ResourceOwningTask is fine, I was merely suggesting folding the > expression > > > to > > > > > construct it into the expression calling to PostTask. > > > > > > > > I see two issues: > > > > 1. The nit - that the lambda is named. I prefer this style. > > > > 2. That actual issue - that the lambda is another layer of indirection, > > which > > > > could be avoided. I think that's preferable to the alternative of defining > > two > > > > classes inside of RtcEventLogImpl and having both access RtcEventLogImpl's > > > > internals. > > > > > > I'm not talking of either of those... I would like: > > > > > > 1. WrapUnique replaced by MakeUnique. > > > > > > 2. The local variable |task| eliminated, and it's value substituted where it > > is > > > used. > > > > 1. We can do that, because unique_ptr<Derived> is not implicitly convertible > to > > unique_ptr<Base>. > > I'd expect the unique_ptr to support implicit conversion whenever the > underlying raw pointers do. Otherwise it's not a smart pointer, but a stupid > pointer... > > From http://en.cppreference.com/w/cpp/memory/unique_ptr: "If T is a derived > class of some base B, then std::unique_ptr<T> is implicitly convertible to > std::unique_ptr<B>. " > > WrapUnique is intended only for rare cases, like working with old raw-pointer > apis, or with private constructors. I think it's fairly important to not > introduce needlessly. > > > 2. I think readability suffers if we do this, with no gain in efficiency to > > justify this. > > I disagree on readability, but I won't insist on this one. You're right, I skimmed through the compiler's error message too quickly - std::unique_ptr converts implicitly from derived-class to base-class. The problem was that PostTask(const Closure&) was a better match than PostTask(std::unique_ptr<QueuedTask>) when posting a sub-class of QueuedTask.
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/3007473002/#ps260001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Make RtcEventLogImpl to use a TaskQueue instead of a helper-thread TODO: Document why this was done, and what small bugs were fixed along the way. BUG=webrtc:8142, webrtc:8143, webrtc:8145 ========== to ========== 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 ==========
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... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:76: // a lambda (a copy constructor is required). Could be formulated as a TODO item to remove the class as soon as we move to C++14. https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:148: // Fails and returns false in case the limit out output size prevents the Sorry if that was my typo, but it should be "on", not "out". https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:275: auto file = rtc::WrapUnique<FileWrapper>(FileWrapper::Create()); I take it this a correct use of WrapUnique, working around the old FileWrapper interface of returning a raw pointer to be owned by the caller? https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:654: task_queue_.PostTask(rtc::MakeUnique<ResourceOwningTask<rtclog::Event>>( I think this looks nicer. Thanks. https://codereview.webrtc.org/3007473002/diff/260001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/260001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:304: RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_); In the case of the worker queue, since we own it, we could replace all the corresponding RTC_CHECK_CALLED_SEQUENTIALLY with RTC_DCHECK(task_queue_.IsCurrent()); If we also replace the corresponding GUARDED_BY with ACCESS_ON, I think the worker_sequence_checker_ can be deleted. What do you think?
The CQ bit was unchecked by eladalon@webrtc.org
https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:76: // a lambda (a copy constructor is required). On 2017/08/31 12:09:24, nisse-webrtc wrote: > Could be formulated as a TODO item to remove the class as soon as we move to > C++14. Done. https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:148: // Fails and returns false in case the limit out output size prevents the On 2017/08/31 12:09:24, nisse-webrtc wrote: > Sorry if that was my typo, but it should be "on", not "out". Done. https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:275: auto file = rtc::WrapUnique<FileWrapper>(FileWrapper::Create()); On 2017/08/31 12:09:24, nisse-webrtc wrote: > I take it this a correct use of WrapUnique, working around the old FileWrapper > interface of returning a raw pointer to be owned by the caller? Yes - unlike the other use of WrapUnique, which was debatable, this case is unavoidable. :-) https://codereview.webrtc.org/3007473002/diff/240001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:654: task_queue_.PostTask(rtc::MakeUnique<ResourceOwningTask<rtclog::Event>>( On 2017/08/31 12:09:24, nisse-webrtc wrote: > I think this looks nicer. Thanks. Once we've added the version of PostTask() that allows us to post a std::unique_ptr<> to a class derived of QueuedTask, WrapUnique became avoidable, this line became shorter, and the version with the intermediary variable became less appealing in comparison; consensus was reached. :-) https://codereview.webrtc.org/3007473002/diff/260001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/260001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:304: RTC_DCHECK_CALLED_SEQUENTIALLY(&worker_sequence_checker_); On 2017/08/31 12:09:24, nisse-webrtc wrote: > In the case of the worker queue, since we own it, we could replace all the > corresponding RTC_CHECK_CALLED_SEQUENTIALLY with > > RTC_DCHECK(task_queue_.IsCurrent()); > > If we also replace the corresponding GUARDED_BY with ACCESS_ON, I think the > worker_sequence_checker_ can be deleted. What do you think? Done (% RTC_DCHECK_RUN_ON).
lgtm https://codereview.webrtc.org/3007473002/diff/280001/webrtc/logging/rtc_event... File webrtc/logging/rtc_event_log/rtc_event_log.cc (right): https://codereview.webrtc.org/3007473002/diff/280001/webrtc/logging/rtc_event... webrtc/logging/rtc_event_log/rtc_event_log.cc:77: // rid of this ocne we've moved to C++14. Nit: "once".
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/3007473002/#ps300001 (title: "Fix typo; change phrasing.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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/bui...)
The CQ bit was checked by eladalon@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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/buil...)
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, nisse@webrtc.org Link to the patchset: https://codereview.webrtc.org/3007473002/#ps320001 (title: "Rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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/bui...)
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, nisse@webrtc.org Link to the patchset: https://codereview.webrtc.org/3007473002/#ps340001 (title: "Relax access' threading constraints.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1504527755315940,
"parent_rev": "a8ae6f2acaef86e63f349442dbde8e81a9bd041e", "commit_rev":
"f33cee7534f0469330b6b7aa57f84eec4b082598"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/f33cee7534f0469330b6b7aa5... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/external/webrtc/+/f33cee7534f0469330b6b7aa5...
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.webrtc.org/3010143002/ by charujain@webrtc.org. The reason for reverting is: Breaks google3 project..
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.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
