Hibernate the thread if there are no events in the queue. Wake it up when an event is added to the …

webrtc/call/rtc_event_log.cc

Index: webrtc/call/rtc_event_log.cc
diff --git a/webrtc/call/rtc_event_log.cc b/webrtc/call/rtc_event_log.cc
index 3c602b772188dc80738e075104805cb8facfe471..90145735eddf90eb720b10a02f1e30f6b2b66196 100644
--- a/webrtc/call/rtc_event_log.cc
+++ b/webrtc/call/rtc_event_log.cc
@@ -103,15 +103,14 @@ class RtcEventLogImpl final : public RtcEventLog {
                              int32_t total_packets) override;
 
  private:
+  void StoreEvent(std::unique_ptr<rtclog::Event>* event);
+
   // Message queue for passing control messages to the logging thread.
   SwapQueue<RtcEventLogHelperThread::ControlMessage> message_queue_;
 
   // Message queue for passing events to the logging thread.
   SwapQueue<std::unique_ptr<rtclog::Event> > event_queue_;
 
-  rtc::Event wake_up_;
-  rtc::Event stopped_;
-
   const Clock* const clock_;
 
   RtcEventLogHelperThread helper_thread_;
@@ -165,13 +164,9 @@ RtcEventLogImpl::RtcEventLogImpl(const Clock* clock)
     // Allocate buffers for roughly one second of history.
     : message_queue_(kControlMessagesPerSecond),
       event_queue_(kEventsPerSecond),
-      wake_up_(false, false),
-      stopped_(false, false),
       clock_(clock),
       helper_thread_(&message_queue_,
                      &event_queue_,
-                     &wake_up_,
-                     &stopped_,
                      clock),
       thread_checker_() {
   thread_checker_.DetachFromThread();
@@ -201,6 +196,7 @@ bool RtcEventLogImpl::StartLogging(const std::string& file_name,
     LOG(LS_ERROR) << "Message queue full. Can't start logging.";
     return false;
   }
+  helper_thread_.SignalNewEvent();
   LOG(LS_INFO) << "Starting WebRTC event log.";
   return true;
 }
@@ -234,6 +230,7 @@ bool RtcEventLogImpl::StartLogging(rtc::PlatformFile platform_file,
     LOG(LS_ERROR) << "Message queue full. Can't start logging.";
     return false;
   }
+  helper_thread_.SignalNewEvent();
   LOG(LS_INFO) << "Starting WebRTC event log.";
   return true;
 }
@@ -255,8 +252,7 @@ void RtcEventLogImpl::StopLogging() {
     message_queue_.Clear();
   }
   LOG(LS_INFO) << "Stopping WebRTC event log.";
-  wake_up_.Set();                       // Request the output thread to wake up.
-  stopped_.Wait(rtc::Event::kForever);  // Wait for the log to stop.
+  helper_thread_.WaitForFileFinished();
 }
 
 void RtcEventLogImpl::LogVideoReceiveStreamConfig(
@@ -292,9 +288,7 @@ void RtcEventLogImpl::LogVideoReceiveStreamConfig(
     decoder->set_name(d.payload_name);
     decoder->set_payload_type(d.payload_type);
   }
-  if (!event_queue_.Insert(&event)) {
-    LOG(LS_ERROR) << "Config queue full. Not logging config event.";
-  }
+  StoreEvent(&event);

[tommi, 2016/06/08 15:26:40]

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.

[terelius, 2016/06/08 17:52:17]

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.
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.
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.
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.

 }
 
 void RtcEventLogImpl::LogVideoSendStreamConfig(
@@ -324,9 +318,7 @@ void RtcEventLogImpl::LogVideoSendStreamConfig(
   rtclog::EncoderConfig* encoder = sender_config->mutable_encoder();
   encoder->set_name(config.encoder_settings.payload_name);
   encoder->set_payload_type(config.encoder_settings.payload_type);
-  if (!event_queue_.Insert(&event)) {
-    LOG(LS_ERROR) << "Config queue full. Not logging config event.";
-  }
+  StoreEvent(&event);
 }
 
 void RtcEventLogImpl::LogRtpHeader(PacketDirection direction,
@@ -356,9 +348,7 @@ void RtcEventLogImpl::LogRtpHeader(PacketDirection direction,
   rtp_event->mutable_rtp_packet()->set_type(ConvertMediaType(media_type));
   rtp_event->mutable_rtp_packet()->set_packet_length(packet_length);
   rtp_event->mutable_rtp_packet()->set_header(header, header_length);
-  if (!event_queue_.Insert(&rtp_event)) {
-    LOG(LS_ERROR) << "RTP queue full. Not logging RTP packet.";
-  }
+  StoreEvent(&rtp_event);
 }
 
 void RtcEventLogImpl::LogRtcpPacket(PacketDirection direction,
@@ -416,9 +406,7 @@ void RtcEventLogImpl::LogRtcpPacket(PacketDirection direction,
     block_begin += block_size;
   }
   rtcp_event->mutable_rtcp_packet()->set_packet_data(buffer, buffer_length);
-  if (!event_queue_.Insert(&rtcp_event)) {
-    LOG(LS_ERROR) << "RTCP queue full. Not logging RTCP packet.";
-  }
+  StoreEvent(&rtcp_event);
 }
 
 void RtcEventLogImpl::LogAudioPlayout(uint32_t ssrc) {
@@ -427,9 +415,7 @@ void RtcEventLogImpl::LogAudioPlayout(uint32_t ssrc) {
   event->set_type(rtclog::Event::AUDIO_PLAYOUT_EVENT);
   auto playout_event = event->mutable_audio_playout_event();
   playout_event->set_local_ssrc(ssrc);
-  if (!event_queue_.Insert(&event)) {
-    LOG(LS_ERROR) << "Playout queue full. Not logging ACM playout.";
-  }
+  StoreEvent(&event);
 }
 
 void RtcEventLogImpl::LogBwePacketLossEvent(int32_t bitrate,
@@ -442,9 +428,14 @@ void RtcEventLogImpl::LogBwePacketLossEvent(int32_t bitrate,
   bwe_event->set_bitrate(bitrate);
   bwe_event->set_fraction_loss(fraction_loss);
   bwe_event->set_total_packets(total_packets);
-  if (!event_queue_.Insert(&event)) {
-    LOG(LS_ERROR) << "BWE loss queue full. Not logging BWE update.";
+  StoreEvent(&event);
+}
+
+void RtcEventLogImpl::StoreEvent(std::unique_ptr<rtclog::Event>* event) {
+  if (!event_queue_.Insert(event)) {
+    LOG(LS_ERROR) << "WebRTC event log queue full. Dropping event.";
   }
+  helper_thread_.SignalNewEvent();
 }
 
 bool RtcEventLog::ParseRtcEventLog(const std::string& file_name,