Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 6 months ago by terelius Modified:
4 years, 6 months ago Reviewers:
ivoc, pbos-webrtc, stefan-webrtc, Stefan, tommi CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionHibernate the thread if there are no events in the queue. Wake it up when an event is added to the queue.
BUG=614192
Committed: https://crrev.com/bea8959687abe97585a70b43152d8c90d8f7aa11
Cr-Commit-Position: refs/heads/master@{#13070}
Patch Set 1 #Patch Set 2 : Nit #
Total comments: 10
Patch Set 3 : Rename signaling events and move to helper class. Change int to bool. #
Total comments: 2
Patch Set 4 : Rename SignalStopFile() -> WaitForFileFinished() #
Total comments: 2
Created: 4 years, 6 months ago
Messages
Total messages: 26 (9 generated)
Description was changed from ========== Hibernate the thread if there are no events in the queue. Wake it up when an event is added to the queue. BUG=614192 ========== to ========== Hibernate the thread if there are no events in the queue. Wake it up when an event is added to the queue. BUG=614192 ==========
terelius@webrtc.org changed reviewers: + holmer@chromium.org, ivoc@webrtc.org
I went with Stefan's suggestion, which is that if the output thread wakes up only to find both queues empty, then we sleep until an element is inserted rather than for a fixed duration. It would be more efficient to store the elements directly to the history and only run the thread if we're writing to file, but that would require larger changes. Please take a look.
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_helper_thread.h (right): https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.h:111: rtc::Event* end_hibernation_; Shouldn't the wake_up_ event be sufficient to signal that we should wake up? Or is the wake_up_ event really just a timer that can be awoken in the destructor? If this is the case then wake_up_ should preferably be renamed something like batch_timer_ or something.
https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log.cc:265: end_hibernation_.Set(); Should the last three lines here be replaced with something like logging_thread_.Stop() which would do the appropriate sets and waits? I think some shared events between the classes could be avoidable.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
lgtm % nits, and please consider pbos comments too. https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_helper_thread.cc (right): https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.cc:123: message_received++; ++message_received; https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.cc:194: message_received++; ++...;
https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_helper_thread.h (right): https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.h:80: int LogToMemory(); size_t or preferably just a bool for whether it succeeds or not.
Please take another look. https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log.cc:265: end_hibernation_.Set(); On 2016/06/03 08:54:25, pbos-webrtc wrote: > Should the last three lines here be replaced with something like > logging_thread_.Stop() which would do the appropriate sets and waits? I think > some shared events between the classes could be avoidable. Done. Note that this only reduces the number of shared events between the classes. It does not change the number of shared events between the threads. https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_helper_thread.cc (right): https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.cc:123: message_received++; On 2016/06/03 08:55:44, stefan-webrtc (holmer) wrote: > ++message_received; Done. https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.cc:194: message_received++; On 2016/06/03 08:55:44, stefan-webrtc (holmer) wrote: > ++...; Done. https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_helper_thread.h (right): https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.h:80: int LogToMemory(); On 2016/06/03 08:56:15, pbos-webrtc wrote: > size_t or preferably just a bool for whether it succeeds or not. Changed to bool. https://codereview.webrtc.org/2035483003/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.h:111: rtc::Event* end_hibernation_; On 2016/06/03 08:51:06, pbos-webrtc wrote: > Shouldn't the wake_up_ event be sufficient to signal that we should wake up? Or > is the wake_up_ event really just a timer that can be awoken in the destructor? > If this is the case then wake_up_ should preferably be renamed something like > batch_timer_ or something. Both are needed. I renamed them wake_periodically_ and wake_from_hibernation_.
https://codereview.webrtc.org/2035483003/diff/40001/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/2035483003/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log.cc:255: helper_thread_.SignalStopFile(); // Wake up thread and wait for it to finish. Rename the method so that you don't need the comment.
lgtm.
On 2016/06/08 12:29:32, ivoc wrote: > lgtm. lgtm
https://codereview.webrtc.org/2035483003/diff/40001/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/2035483003/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log.cc:255: helper_thread_.SignalStopFile(); // Wake up thread and wait for it to finish. On 2016/06/08 12:25:42, pbos-webrtc wrote: > Rename the method so that you don't need the comment. Done.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2035483003/#ps60001 (title: "Rename SignalStopFile() -> WaitForFileFinished()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035483003/60001
Message was sent while issue was closed.
Description was changed from ========== Hibernate the thread if there are no events in the queue. Wake it up when an event is added to the queue. BUG=614192 ========== to ========== Hibernate the thread if there are no events in the queue. Wake it up when an event is added to the queue. BUG=614192 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Hibernate the thread if there are no events in the queue. Wake it up when an event is added to the queue. BUG=614192 ========== to ========== Hibernate the thread if there are no events in the queue. Wake it up when an event is added to the queue. BUG=614192 Committed: https://crrev.com/bea8959687abe97585a70b43152d8c90d8f7aa11 Cr-Commit-Position: refs/heads/master@{#13070} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bea8959687abe97585a70b43152d8c90d8f7aa11 Cr-Commit-Position: refs/heads/master@{#13070}
Message was sent while issue was closed.
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
Message was sent while issue was closed.
drive by observations https://codereview.webrtc.org/2035483003/diff/60001/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/2035483003/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log.cc:291: StoreEvent(&event); This pattern looked strange to me (passing a |std:unique_pt<>r*|), so I guessed that what you really wanted to do was to move the object (std::move), but then I saw you're using SwapQueue for the events. I wonder if that's what you really want since SwapQueue has a limit and it returns a previous object - but with StoreEvent, that always gets deleted anyhow and we always new() events that get inserted. It also doesn't appear that we handle the error situation when we can't add to that queue and even in that case, we signal a new event, both of which seem like a bug. At a glanse, the implementation seems to be very complex for what I think (possibly naively) should be fairly straight forward. The RtcEventLogHelperThread and the synchronization with it could be to a large extent be replaced by TaskQueue, using FileWrapper looks like overkill to me (check out the implementation), we seem to be using 3 separate event objects and have several complex in-memory container structures. Also, are the Start/Stop methods necessary? Could we simply start in the constructor and stop in the destructor? That way it would be simple at the call site to be sure when an object should not be eating up any resources such as threads, when logging isn't active.
Message was sent while issue was closed.
https://codereview.webrtc.org/2035483003/diff/60001/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/2035483003/diff/60001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log.cc:291: StoreEvent(&event); On 2016/06/08 15:26:40, tommi-webrtc wrote: > This pattern looked strange to me (passing a |std:unique_pt<>r*|), so I guessed > that what you really wanted to do was to move the object (std::move), but then I > saw you're using SwapQueue for the events. I wonder if that's what you really > want since SwapQueue has a limit and it returns a previous object - but with > StoreEvent, that always gets deleted anyhow and we always new() events that get > inserted. You're right. I thought about the argument type too, but decided to pass a unique_ptr<>* anyway, because that's what the SwapQueue uses. As we discussed before, there is no good way to reuse the old events without having one queue per event type. The reason is that different events allocate memory for protobuf objects of different types. When you want to reuse an event, there is a fairly high chance that the new event type doesn't match the event type of the old event you have available. One way around this would be to serialize the event and store the string representation in the ringbuffer. This would avoid extra allocations / memory fragmentation and would be significantly more compact in terms of memory usage. The downside is that we would need a slightly more complicated ringbuffer, and we would be doing the serialization on the logging thread. > It also doesn't appear that we handle the error situation when we can't add to > that queue and even in that case, we signal a new event, both of which seem like > a bug. The queue size is intended to be significantly larger than the expected number of events between each Process() cycle, so we shouldn't normally fail to insert. However, if we do, we print an error message and we drop the event. I think this is the best we can do with a fixed size queue. If the queue is full, then it is definitely not a bug to signal that there are new (i.e. unprocessed) events. Perhaps the function name could be chosen better, but I wanted a name that makes it clear that the function should be called after every inserted event. > At a glanse, the implementation seems to be very complex for what I think > (possibly naively) should be fairly straight forward. The > RtcEventLogHelperThread and the synchronization with it could be to a large > extent be replaced by TaskQueue, using FileWrapper looks like overkill to me > (check out the implementation), we seem to be using 3 separate event objects and > have several complex in-memory container structures. Much of the complexity comes from keeping a history of old events in memory. This was decided in the one of the first design meetings as a way to let users start logging when they experience problems but at the same time make the log contain the cause of the problem. It would be significantly easier if we didn't have to do that extra buffering. I agree about the FileWrapper. TaskQueue might be useful, but it did not exist at the time. > Also, are the Start/Stop methods necessary? Could we simply start in the > constructor and stop in the destructor? That way it would be simple at the call > site to be sure when an object should not be eating up any resources such as > threads, when logging isn't active. The Start and Stop methods are used to control whether we log to file. In chromium we get the filename from the user, so we can't easily start logging automatically. Note that calling Stop in the destructor wouldn't help at all; the problem isn't that we somehow "forget" to stop, the problem is that the destructors never get called in the first place.
Message was sent while issue was closed.
On 2016/06/08 17:52:17, terelius wrote: > https://codereview.webrtc.org/2035483003/diff/60001/webrtc/call/rtc_event_log.cc > File webrtc/call/rtc_event_log.cc (right): > > https://codereview.webrtc.org/2035483003/diff/60001/webrtc/call/rtc_event_log... > webrtc/call/rtc_event_log.cc:291: StoreEvent(&event); > On 2016/06/08 15:26:40, tommi-webrtc wrote: > > This pattern looked strange to me (passing a |std:unique_pt<>r*|), so I > guessed > > that what you really wanted to do was to move the object (std::move), but then > I > > saw you're using SwapQueue for the events. I wonder if that's what you really > > want since SwapQueue has a limit and it returns a previous object - but with > > StoreEvent, that always gets deleted anyhow and we always new() events that > get > > inserted. > > You're right. I thought about the argument type too, but decided to pass a > unique_ptr<>* anyway, because that's what the SwapQueue uses. > > As we discussed before, there is no good way to reuse the old events without > having one queue per event type. The reason is that different events allocate > memory for protobuf objects of different types. When you want to reuse an event, > there is a fairly high chance that the new event type doesn't match the event > type of the old event you have available. > > One way around this would be to serialize the event and store the string > representation in the ringbuffer. This would avoid extra allocations / memory > fragmentation and would be significantly more compact in terms of memory usage. > The downside is that we would need a slightly more complicated ringbuffer, and > we would be doing the serialization on the logging thread. > > > It also doesn't appear that we handle the error situation when we can't add to > > that queue and even in that case, we signal a new event, both of which seem > like > > a bug. > > The queue size is intended to be significantly larger than the expected number > of events between each Process() cycle, so we shouldn't normally fail to insert. > However, if we do, we print an error message and we drop the event. I think this > is the best we can do with a fixed size queue. > > If the queue is full, then it is definitely not a bug to signal that there are > new (i.e. unprocessed) events. Perhaps the function name could be chosen better, > but I wanted a name that makes it clear that the function should be called after > every inserted event. My point was that if the queue is full and an event is not inserted, we still signal. That's one signal for no event. > > > At a glanse, the implementation seems to be very complex for what I think > > (possibly naively) should be fairly straight forward. The > > RtcEventLogHelperThread and the synchronization with it could be to a large > > extent be replaced by TaskQueue, using FileWrapper looks like overkill to me > > (check out the implementation), we seem to be using 3 separate event objects > and > > have several complex in-memory container structures. > > Much of the complexity comes from keeping a history of old events in memory. > This was decided in the one of the first design meetings as a way to let users > start logging when they experience problems but at the same time make the log > contain the cause of the problem. It would be significantly easier if we didn't > have to do that extra buffering. > > I agree about the FileWrapper. TaskQueue might be useful, but it did not exist > at the time. > > > Also, are the Start/Stop methods necessary? Could we simply start in the > > constructor and stop in the destructor? That way it would be simple at the > call > > site to be sure when an object should not be eating up any resources such as > > threads, when logging isn't active. > > The Start and Stop methods are used to control whether we log to file. In > chromium we get the filename from the user, so we can't easily start logging > automatically. Note that calling Stop in the destructor wouldn't help at all; > the problem isn't that we somehow "forget" to stop, the problem is that the > destructors never get called in the first place. If Stop() gets called, couldn't that be the place where we'd delete the object? (similarly, match Start() with the ctor?)
Message was sent while issue was closed.
On 2016/06/09 14:06:34, tommi-webrtc wrote: > On 2016/06/08 17:52:17, terelius wrote: > > > https://codereview.webrtc.org/2035483003/diff/60001/webrtc/call/rtc_event_log.cc > > File webrtc/call/rtc_event_log.cc (right): > > > https://codereview.webrtc.org/2035483003/diff/60001/webrtc/call/rtc_event_log... > > webrtc/call/rtc_event_log.cc:291: StoreEvent(&event); > > > > If the queue is full, then it is definitely not a bug to signal that there are > > new (i.e. unprocessed) events. Perhaps the function name could be chosen > better, > > but I wanted a name that makes it clear that the function should be called > after > > every inserted event. > > My point was that if the queue is full and an event is not inserted, we still > signal. That's one signal for no event. I understand what you mean (and it should be redundant to signal if no event was added), but it is not a bug. The signal we send to the thread means "there are events in the queues". If the queue is full, it is perfectly reasonable to wake up the thread and inform it that there are events to be processed. > > The Start and Stop methods are used to control whether we log to file. In > > chromium we get the filename from the user, so we can't easily start logging > > automatically. Note that calling Stop in the destructor wouldn't help at all; > > the problem isn't that we somehow "forget" to stop, the problem is that the > > destructors never get called in the first place. > > If Stop() gets called, couldn't that be the place where we'd delete the object? > (similarly, match Start() with the ctor?) Which object do you want to delete? We can't delete the RtcEventLog object while the Call exists since we have pointers to the event log in multiple components. Once ivoc@ moves the event log ownership to Call, it will be deleted automatically when the Call ends. (This will also terminate the helper thread.) We could delete the RtcEventLogHelperThread when we are no longer writing to file, but that would require larger changes than this CL. Also, previous reviewers preferred a solution where the RtcEventLog can be oblivious to how the events are handled; the decision between logging to memory and logging to file is done by the helper thread. This design is also similar to the Process() functions that are used all over WebRTC. If we want to only run the thread when writing to file (and still keep a short history in memory when not logging to file), then the RtcEventLog would have to store the events in the right memory buffer which would potentially move some extra work from the helper thread to the real-time thread. TL;DR: I don't think this is a bad idea, but it would take more time to implement and verify. Maintaining a history on the real-time thread would also interfere with some ideas we have for reducing the memory usage, and we need to understand that space-time trade-off. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
