From 242068d58cc01640aa9f733fa67f078fc65c4ae5 Mon Sep 17 00:00:00 2001 From: "tommi@webrtc.org" Date: Mon, 14 Jul 2014 20:19:56 +0000 Subject: [PATCH] A step towards changing StatsReport::Value::name to an enum. The stats reporting code does a lot of unnecessary string copying. This is a step in the direction of removing that and forcing use of only known constants. This is a reland of an already reviewed cl that got reverted by mistake. TBR=xians@google.com Review URL: https://webrtc-codereview.appspot.com/12989004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6678 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/app/webrtc/objc/RTCStatsReport.mm | 2 +- talk/app/webrtc/statscollector.cc | 18 ++++---- talk/app/webrtc/statscollector_unittest.cc | 53 ++++++++++++++++------ talk/app/webrtc/statstypes.h | 41 ++++++++++++++--- 4 files changed, 84 insertions(+), 30 deletions(-) diff --git a/talk/app/webrtc/objc/RTCStatsReport.mm b/talk/app/webrtc/objc/RTCStatsReport.mm index 8da4b469a0..98cf6548e3 100644 --- a/talk/app/webrtc/objc/RTCStatsReport.mm +++ b/talk/app/webrtc/objc/RTCStatsReport.mm @@ -57,7 +57,7 @@ [NSMutableArray arrayWithCapacity:statsReport.values.size()]; webrtc::StatsReport::Values::const_iterator it = statsReport.values.begin(); for (; it != statsReport.values.end(); ++it) { - RTCPair* pair = [[RTCPair alloc] initWithKey:@(it->name.c_str()) + RTCPair* pair = [[RTCPair alloc] initWithKey:@(it->display_name()) value:@(it->value.c_str())]; [values addObject:pair]; } diff --git a/talk/app/webrtc/statscollector.cc b/talk/app/webrtc/statscollector.cc index b80da23036..dd615ab89e 100644 --- a/talk/app/webrtc/statscollector.cc +++ b/talk/app/webrtc/statscollector.cc @@ -193,19 +193,17 @@ const char StatsReport::kStatsReportTypeCertificate[] = "googCertificate"; const char StatsReport::kStatsReportVideoBweId[] = "bweforvideo"; // Implementations of functions in statstypes.h -void StatsReport::AddValue(const std::string& name, const std::string& value) { - Value temp; - temp.name = name; - temp.value = value; - values.push_back(temp); +void StatsReport::AddValue(StatsReport::StatsValueName name, + const std::string& value) { + values.push_back(Value(name, value)); } -void StatsReport::AddValue(const std::string& name, int64 value) { +void StatsReport::AddValue(StatsReport::StatsValueName name, int64 value) { AddValue(name, talk_base::ToString(value)); } template -void StatsReport::AddValue(const std::string& name, +void StatsReport::AddValue(StatsReport::StatsValueName name, const std::vector& value) { std::ostringstream oss; oss << "["; @@ -218,11 +216,11 @@ void StatsReport::AddValue(const std::string& name, AddValue(name, oss.str()); } -void StatsReport::AddBoolean(const std::string& name, bool value) { +void StatsReport::AddBoolean(StatsReport::StatsValueName name, bool value) { AddValue(name, value ? "true" : "false"); } -void StatsReport::ReplaceValue(const std::string& name, +void StatsReport::ReplaceValue(StatsReport::StatsValueName name, const std::string& value) { for (Values::iterator it = values.begin(); it != values.end(); ++it) { if ((*it).name == name) { @@ -258,7 +256,7 @@ std::string StatsId(const std::string& type, const std::string& id, bool ExtractValueFromReport( const StatsReport& report, - const std::string& name, + StatsReport::StatsValueName name, std::string* value) { StatsReport::Values::const_iterator it = report.values.begin(); for (; it != report.values.end(); ++it) { diff --git a/talk/app/webrtc/statscollector_unittest.cc b/talk/app/webrtc/statscollector_unittest.cc index 76251e2515..fc0ca0ed79 100644 --- a/talk/app/webrtc/statscollector_unittest.cc +++ b/talk/app/webrtc/statscollector_unittest.cc @@ -149,7 +149,7 @@ class FakeAudioTrack }; bool GetValue(const StatsReport* report, - const std::string& name, + StatsReport::StatsValueName name, std::string* value) { StatsReport::Values::const_iterator it = report->values.begin(); for (; it != report->values.end(); ++it) { @@ -163,7 +163,7 @@ bool GetValue(const StatsReport* report, std::string ExtractStatsValue(const std::string& type, const StatsReports& reports, - const std::string name) { + StatsReport::StatsValueName name) { if (reports.empty()) { return kNoReports; } @@ -204,13 +204,13 @@ const StatsReport* FindReportById(const StatsReports& reports, } std::string ExtractSsrcStatsValue(StatsReports reports, - const std::string& name) { + StatsReport::StatsValueName name) { return ExtractStatsValue( StatsReport::kStatsReportTypeSsrc, reports, name); } std::string ExtractBweStatsValue(StatsReports reports, - const std::string& name) { + StatsReport::StatsValueName name) { return ExtractStatsValue( StatsReport::kStatsReportTypeBwe, reports, name); } @@ -446,7 +446,7 @@ class StatsCollectorTest : public testing::Test { // This creates a standard setup with a transport called "trspname" // having one transport channel // and the specified virtual connection name. - void InitSessionStats(const std::string vc_name) { + void InitSessionStats(const std::string& vc_name) { const std::string kTransportName("trspname"); cricket::TransportStats transport_stats; cricket::TransportChannelStats channel_stats; @@ -687,11 +687,12 @@ TEST_F(StatsCollectorTest, BytesCounterHandles64Bits) { EXPECT_CALL(session_, video_channel()).WillRepeatedly(Return(&video_channel)); EXPECT_CALL(session_, voice_channel()).WillRepeatedly(ReturnNull()); EXPECT_CALL(*media_channel, GetStats(_, _)) - .WillOnce(DoAll(SetArgPointee<1>(stats_read), - Return(true))); + .WillOnce(DoAll(SetArgPointee<1>(stats_read), + Return(true))); stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard); stats.GetStats(NULL, &reports); - std::string result = ExtractSsrcStatsValue(reports, "bytesSent"); + std::string result = ExtractSsrcStatsValue(reports, + StatsReport::kStatsValueNameBytesSent); EXPECT_EQ(kBytesSentString, result); } @@ -730,9 +731,11 @@ TEST_F(StatsCollectorTest, BandwidthEstimationInfoIsReported) { stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard); stats.GetStats(NULL, &reports); - std::string result = ExtractSsrcStatsValue(reports, "bytesSent"); + std::string result = ExtractSsrcStatsValue(reports, + StatsReport::kStatsValueNameBytesSent); EXPECT_EQ(kBytesSentString, result); - result = ExtractBweStatsValue(reports, "googTargetEncBitrate"); + result = ExtractBweStatsValue(reports, + StatsReport::kStatsValueNameTargetEncBitrate); EXPECT_EQ(kTargetEncBitrateString, result); } @@ -850,6 +853,9 @@ TEST_F(StatsCollectorTest, TrackAndSsrcObjectExistAfterUpdateSsrcStats) { // stats object, and that this transport stats object exists in stats. TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) { webrtc::StatsCollector stats(&session_); // Implementation under test. + // Ignore unused callback (logspam). + EXPECT_CALL(session_, GetTransport(_)) + .WillOnce(Return(static_cast(NULL))); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); // The content_name known by the video channel. const std::string kVcName("vcname"); @@ -919,6 +925,9 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsAbsent) { // an outgoing SSRC where stats are returned. TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) { webrtc::StatsCollector stats(&session_); // Implementation under test. + // Ignore unused callback (logspam). + EXPECT_CALL(session_, GetTransport(_)) + .WillOnce(Return(static_cast(NULL))); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); // The content_name known by the video channel. const std::string kVcName("vcname"); @@ -1190,13 +1199,15 @@ TEST_F(StatsCollectorTest, StatsOutputLevelVerbose) { StatsReports reports; // returned values. stats.GetStats(NULL, &reports); std::string result = ExtractBweStatsValue( - reports, "googReceivedPacketGroupPropagationDeltaSumDebug"); + reports, + StatsReport::kStatsValueNameRecvPacketGroupPropagationDeltaSumDebug); EXPECT_EQ("10", result); result = ExtractBweStatsValue( - reports, "googReceivedPacketGroupPropagationDeltaDebug"); + reports, + StatsReport::kStatsValueNameRecvPacketGroupPropagationDeltaDebug); EXPECT_EQ("[100, 200]", result); result = ExtractBweStatsValue( - reports, "googReceivedPacketGroupArrivalTimeDebug"); + reports, StatsReport::kStatsValueNameRecvPacketGroupArrivalTimeDebug); EXPECT_EQ("[1000, 2000]", result); } @@ -1204,6 +1215,10 @@ TEST_F(StatsCollectorTest, StatsOutputLevelVerbose) { // AudioTrackInterface::GetStats() method. TEST_F(StatsCollectorTest, GetStatsFromLocalAudioTrack) { webrtc::StatsCollector stats(&session_); // Implementation under test. + // Ignore unused callback (logspam). + EXPECT_CALL(session_, GetTransport(_)) + .WillOnce(Return(static_cast(NULL))); + MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The content_name known by the voice channel. const std::string kVcName("vcname"); @@ -1233,6 +1248,9 @@ TEST_F(StatsCollectorTest, GetStatsFromLocalAudioTrack) { // correctly. TEST_F(StatsCollectorTest, GetStatsFromRemoteStream) { webrtc::StatsCollector stats(&session_); // Implementation under test. + // Ignore unused callback (logspam). + EXPECT_CALL(session_, GetTransport(_)) + .WillOnce(Return(static_cast(NULL))); MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The content_name known by the voice channel. const std::string kVcName("vcname"); @@ -1256,6 +1274,9 @@ TEST_F(StatsCollectorTest, GetStatsFromRemoteStream) { // after a RemoveLocalAudioTrack() call. TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) { webrtc::StatsCollector stats(&session_); // Implementation under test. + // Ignore unused callback (logspam). + EXPECT_CALL(session_, GetTransport(_)) + .WillOnce(Return(static_cast(NULL))); MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The content_name known by the voice channel. const std::string kVcName("vcname"); @@ -1310,6 +1331,9 @@ TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) { // the same ssrc, they populate stats reports correctly. TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) { webrtc::StatsCollector stats(&session_); // Implementation under test. + // Ignore unused callback (logspam). + EXPECT_CALL(session_, GetTransport(_)) + .WillOnce(Return(static_cast(NULL))); MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The content_name known by the voice channel. const std::string kVcName("vcname"); @@ -1388,6 +1412,9 @@ TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) { // avoid duplication of code in test cases. TEST_F(StatsCollectorTest, TwoLocalTracksWithSameSsrc) { webrtc::StatsCollector stats(&session_); // Implementation under test. + // Ignore unused callback (logspam). + EXPECT_CALL(session_, GetTransport(_)) + .WillRepeatedly(Return(static_cast(NULL))); MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The content_name known by the voice channel. const std::string kVcName("vcname"); diff --git a/talk/app/webrtc/statstypes.h b/talk/app/webrtc/statstypes.h index 0a2aa03a5c..828b9f5f49 100644 --- a/talk/app/webrtc/statstypes.h +++ b/talk/app/webrtc/statstypes.h @@ -43,26 +43,55 @@ class StatsReport { public: StatsReport() : timestamp(0) { } + // TODO(tommi): Change this to be an enum type that holds all the + // kStatsValueName constants. + typedef const char* StatsValueName; + std::string id; // See below for contents. std::string type; // See below for contents. struct Value { - std::string name; + Value() : name(NULL) {} + // The copy ctor can't be declared as explicit due to problems with STL. + Value(const Value& other) : name(other.name), value(other.value) {} + explicit Value(StatsValueName name) : name(name) {} + Value(StatsValueName name, const std::string& value) + : name(name), value(value) { + } + + // TODO(tommi): Remove this operator once we don't need it. + // The operator is provided for compatibility with STL containers. + // The public |name| member variable is otherwise meant to be read-only. + Value& operator=(const Value& other) { + const_cast(name) = other.name; + value = other.value; + return *this; + } + + // TODO(tommi): Change implementation to do a simple enum value-to-static- + // string conversion when client code has been updated to use this method + // instead of the |name| member variable. + const char* display_name() const { return name; } + + const StatsValueName name; + std::string value; }; - void AddValue(const std::string& name, const std::string& value); - void AddValue(const std::string& name, int64 value); + void AddValue(StatsValueName name, const std::string& value); + void AddValue(StatsValueName name, int64 value); template - void AddValue(const std::string& name, const std::vector& value); - void AddBoolean(const std::string& name, bool value); + void AddValue(StatsValueName name, const std::vector& value); + void AddBoolean(StatsValueName name, bool value); - void ReplaceValue(const std::string& name, const std::string& value); + void ReplaceValue(StatsValueName name, const std::string& value); double timestamp; // Time since 1970-01-01T00:00:00Z in milliseconds. typedef std::vector Values; Values values; + // TODO(tommi): These should all be enum values. + // StatsReport types. // A StatsReport of |type| = "googSession" contains overall information // about the thing libjingle calls a session (which may contain one