ESC-154 ESC-156 Metrics integration into viewer's threads

Removed declared but undefined interfaces from LLTextureFetch family.
Had inserted the cross-thread command processor into some of
LLTextureFetchWorker's processing which was unnatural and
probably wrong.  Moved it to LLTextureFetch which turned out to
be far, far more natural.  Better documentation on the asLLSD()
format.  Refined LLSD stats merger logic and enhanced unit
tests to verify same.

5 files changed, 91 insertions, 69 deletions

diff --git a/indra/newview/lltexturefetch.cpp b/indra/newview/lltexturefetch.cpp
index d303d425c8..e574a35479 100644
--- a/indra/newview/lltexturefetch.cpp
+++ b/indra/newview/lltexturefetch.cpp
@@ -405,7 +405,7 @@ namespace
  * into the respective collector unconcerned with locking and
  * the state of any other thread.  But when the agent moves into
  * a different region or the metrics timer expires and a report
- * needs to be sent back to the grid, messaging across grids
+ * needs to be sent back to the grid, messaging across threads
  * is required to distribute data and perform global actions.
  * In pseudo-UML, it looks like:
  *
@@ -484,7 +484,11 @@ public:
 	virtual ~TFRequest()
 		{}
 
-	virtual bool doWork(LLTextureFetchWorker * worker) = 0;
+	// Patterned after QueuedRequest's method but expected behavior
+	// is different.  Always expected to complete on the first call
+	// and work dispatcher will assume the same and delete the
+	// request after invocation.
+	virtual bool doWork(LLTextureFetch * fetcher) = 0;
 };
 
 
@@ -511,7 +515,7 @@ public:
 	virtual ~TFReqSetRegion()
 		{}
 
-	virtual bool doWork(LLTextureFetchWorker * worker);
+	virtual bool doWork(LLTextureFetch * fetcher);
 		
 public:
 	const LLUUID mRegionID;
@@ -557,7 +561,7 @@ public:
 
 	virtual ~TFReqSendMetrics();
 
-	virtual bool doWork(LLTextureFetchWorker * worker);
+	virtual bool doWork(LLTextureFetch * fetcher);
 		
 public:
 	const std::string mCapsURL;
@@ -808,9 +812,6 @@ bool LLTextureFetchWorker::doWork(S32 param)
 		}
 	}
 
-	// Run a cross-thread command, if any.
-	mFetcher->cmdDoWork(this);
-	
 	if(mImagePriority < F_ALMOST_ZERO)
 	{
 		if (mState == INIT || mState == LOAD_FROM_NETWORK || mState == LOAD_FROM_SIMULATOR)
@@ -2188,6 +2189,9 @@ void LLTextureFetch::threadedUpdate()
 	}
 	process_timer.reset();
 	
+	// Run a cross-thread command, if any.
+	cmdDoWork();
+	
 	// Update Curl on same thread as mCurlGetRequest was constructed
 	S32 processed = mCurlGetRequest->process();
 	if (processed > 0)
@@ -2695,22 +2699,22 @@ TFRequest * LLTextureFetch::cmdDequeue()
 	return ret;
 }
 
-void LLTextureFetch::cmdDoWork(LLTextureFetchWorker * worker)
+void LLTextureFetch::cmdDoWork()
 {
-	// Queue is expected to be locked here.
-
 	if (mDebugPause)
 	{
 		return;  // debug: don't do any work
 	}
 
+	lockQueue();
 	TFRequest * req = cmdDequeue();
 	if (req)
 	{
 		// One request per pass should really be enough for this.
-		req->doWork(worker);
+		req->doWork(this);
 		delete req;
 	}
+	unlockQueue();
 }
 
 
@@ -2727,7 +2731,7 @@ namespace
  * Thread:  Thread1 (TextureFetch)
  */
 bool
-TFReqSetRegion::doWork(LLTextureFetchWorker *)
+TFReqSetRegion::doWork(LLTextureFetch *)
 {
 	LLViewerAssetStatsFF::set_region_thread1(mRegionID);
 
@@ -2749,7 +2753,7 @@ TFReqSendMetrics::~TFReqSendMetrics()
  * Thread:  Thread1 (TextureFetch)
  */
 bool
-TFReqSendMetrics::doWork(LLTextureFetchWorker * fetch_worker)
+TFReqSendMetrics::doWork(LLTextureFetch * fetcher)
 {
 	/*
 	 * HTTP POST responder.  Doesn't do much but tries to
@@ -2818,11 +2822,11 @@ TFReqSendMetrics::doWork(LLTextureFetchWorker * fetch_worker)
 	if (! mCapsURL.empty())
 	{
 		LLCurlRequest::headers_t headers;
-		fetch_worker->getFetcher().getCurlRequest().post(mCapsURL,
-														 headers,
-														 thread1_stats,
-														 new lcl_responder(LLTextureFetch::svMetricsDataBreak,
-																		   reporting_started));
+		fetcher->getCurlRequest().post(mCapsURL,
+									   headers,
+									   thread1_stats,
+									   new lcl_responder(LLTextureFetch::svMetricsDataBreak,
+														 reporting_started));
 	}
 	else
 	{
diff --git a/indra/newview/lltexturefetch.h b/indra/newview/lltexturefetch.h
index 220305d881..88b7e4a16b 100644
--- a/indra/newview/lltexturefetch.h
+++ b/indra/newview/lltexturefetch.h
@@ -85,7 +85,7 @@ public:
 
 	LLTextureInfo* getTextureInfo() { return &mTextureInfo; }
 
-	// Commands available to other threads.
+	// Commands available to other threads to control metrics gathering operations.
 	void commandSetRegion(const LLUUID & region_id);
 	void commandSendMetrics(const std::string & caps_url, LLSD * report_main);
 	void commandDataBreak();
@@ -98,8 +98,6 @@ protected:
 	void addToHTTPQueue(const LLUUID& id);
 	void removeFromHTTPQueue(const LLUUID& id, S32 received_size = 0);
 	void removeRequest(LLTextureFetchWorker* worker, bool cancel);
-	// Called from worker thread (during doWork)
-	void processCurlRequests();
 
 	// Overrides from the LLThread tree
 	bool runCondition();
@@ -110,10 +108,10 @@ private:
 	/*virtual*/ void endThread(void);
 	/*virtual*/ void threadedUpdate(void);
 
-	// command helpers
+	// Metrics command helpers
 	void cmdEnqueue(TFRequest *);
 	TFRequest * cmdDequeue();
-	void cmdDoWork(LLTextureFetchWorker* worker);
+	void cmdDoWork();
 	
 public:
 	LLUUID mDebugID;
@@ -146,7 +144,7 @@ private:
 
 	U32 mHTTPTextureBits;
 
-	// Special cross-thread command queue.  This command queue
+	// Out-of-band cross-thread command queue.  This command queue
 	// is logically tied to LLQueuedThread's list of
 	// QueuedRequest instances and so must be covered by the
 	// same locks.
diff --git a/indra/newview/llviewerassetstats.cpp b/indra/newview/llviewerassetstats.cpp
index c0287863f6..c3e58cdd56 100644
--- a/indra/newview/llviewerassetstats.cpp
+++ b/indra/newview/llviewerassetstats.cpp
@@ -230,7 +230,7 @@ LLViewerAssetStats::asLLSD()
 			LLSD::String("get_other")
 		};
 
-	// Sub-tags.  If you add or delete from this list, mergeLLSD() must be updated.
+	// Sub-tags.  If you add or delete from this list, mergeRegionsLLSD() must be updated.
 	static const LLSD::String enq_tag("enqueued");
 	static const LLSD::String deq_tag("dequeued");
 	static const LLSD::String rcnt_tag("resp_count");
@@ -281,7 +281,7 @@ LLViewerAssetStats::asLLSD()
 }
 
 /* static */ void
-LLViewerAssetStats::mergeLLSD(const LLSD & src, LLSD & dst)
+LLViewerAssetStats::mergeRegionsLLSD(const LLSD & src, LLSD & dst)
 {
 	// Merge operator definitions
 	static const int MOP_ADD_INT(0);
@@ -290,11 +290,6 @@ LLViewerAssetStats::mergeLLSD(const LLSD & src, LLSD & dst)
 	static const int MOP_MEAN_REAL(3);	// Requires a 'mMergeOpArg' to weight the input terms
 
 	static const LLSD::String regions_key("regions");
-	static const LLSD::String root_key_list[] =
-		{
-			"duration",
-			regions_key
-		};
 	
 	static const struct
 		{
@@ -318,35 +313,29 @@ LLViewerAssetStats::mergeLLSD(const LLSD & src, LLSD & dst)
 			{ "resp_max", MOP_MAX_REAL, "" }
 		};
 
-	// First normalized the root keys but remember if we need to do full merge
-	const bool needs_deep_merge(src.has(regions_key) && dst.has(regions_key));
-	
-	for (int root_index(0); root_index < LL_ARRAY_SIZE(root_key_list); ++root_index)
+	// Trivial checks
+	if (! src.has(regions_key))
 	{
-		const LLSD::String & key_name(root_key_list[root_index]);
-						
-		if ((! src.has(key_name)) || dst.has(key_name))
-			continue;
-
-		// key present in source, not in dst here
-		dst[key_name] = src[key_name];
+		return;
 	}
 
-	if (! needs_deep_merge)
+	if (! dst.has(regions_key))
+	{
+		dst[regions_key] = src[regions_key];
 		return;
-
-	// Okay, had both src and dst 'regions' section, do the deep merge
-
+	}
+	
+	// Non-trivial cases requiring a deep merge.
 	const LLSD & root_src(src[regions_key]);
 	LLSD & root_dst(dst[regions_key]);
 	
-	const LLSD::map_const_iterator it_end(root_src.endMap());
-	for (LLSD::map_const_iterator it(root_src.beginMap()); it_end != it; ++it)
+	const LLSD::map_const_iterator it_uuid_end(root_src.endMap());
+	for (LLSD::map_const_iterator it_uuid(root_src.beginMap()); it_uuid_end != it_uuid; ++it_uuid)
 	{
-		if (! root_dst.has(it->first))
+		if (! root_dst.has(it_uuid->first))
 		{
 			// src[<region>] without matching dst[<region>]
-			root_dst[it->first] = it->second;
+			root_dst[it_uuid->first] = it_uuid->second;
 		}
 		else
 		{
@@ -354,30 +343,30 @@ LLViewerAssetStats::mergeLLSD(const LLSD & src, LLSD & dst)
 			// We have matching source and destination regions.
 			// Now iterate over each asset bin in the region status.  Could iterate over
 			// an explicit list but this will do as well.
-			LLSD & reg_dst(root_dst[it->first]);
-			const LLSD & reg_src(root_src[it->first]);
+			LLSD & reg_dst(root_dst[it_uuid->first]);
+			const LLSD & reg_src(root_src[it_uuid->first]);
 
-			const LLSD::map_const_iterator it_src_bin_end(reg_src.endMap());
-			for (LLSD::map_const_iterator it_src_bin(reg_src.beginMap()); it_src_bin_end != it_src_bin; ++it_src_bin)
+			const LLSD::map_const_iterator it_sets_end(reg_src.endMap());
+			for (LLSD::map_const_iterator it_sets(reg_src.beginMap()); it_sets_end != it_sets; ++it_sets)
 			{
 				static const LLSD::String no_touch_1("duration");
 
-				if (no_touch_1 == it_src_bin->first)
+				if (no_touch_1 == it_sets->first)
 				{
 					continue;
 				}
-				else if (! reg_dst.has(it_src_bin->first))
+				else if (! reg_dst.has(it_sets->first))
 				{
 					// src[<region>][<asset>] without matching dst[<region>][<asset>]
-					reg_dst[it_src_bin->first] = it_src_bin->second;
+					reg_dst[it_sets->first] = it_sets->second;
 				}
 				else
 				{
 					// src[<region>][<asset>] with matching dst[<region>][<asset>]
 					// Matching stats bin in both source and destination regions.
 					// Iterate over those bin keys we know how to merge, leave the remainder untouched.
-					LLSD & bin_dst(reg_dst[it_src_bin->first]);
-					const LLSD & bin_src(reg_src[it_src_bin->first]);
+					LLSD & bin_dst(reg_dst[it_sets->first]);
+					const LLSD & bin_src(reg_src[it_sets->first]);
 
 					for (int key_index(0); key_index < LL_ARRAY_SIZE(key_list); ++key_index)
 					{
@@ -577,7 +566,6 @@ void
 merge_stats(const LLSD & src, LLSD & dst)
 {
 	static const LLSD::String regions_key("regions");
-	static const LLSD::String dur_key("duration");
 
 	// Trivial cases first
 	if (! src.isMap())
@@ -592,14 +580,14 @@ merge_stats(const LLSD & src, LLSD & dst)
 	}
 	
 	// Okay, both src and dst are maps at this point.
-	// Collector class know how to merge it's part
-	LLViewerAssetStats::mergeLLSD(src, dst);
+	// Collector class know how to merge the regions part.
+	LLViewerAssetStats::mergeRegionsLLSD(src, dst);
 
-	// Now merge non-collector bits manually.
+	// Now merge non-regions bits manually.
 	const LLSD::map_const_iterator it_end(src.endMap());
 	for (LLSD::map_const_iterator it(src.beginMap()); it_end != it; ++it)
 	{
-		if (regions_key == it->first || dur_key == it->first)
+		if (regions_key == it->first)
 			continue;
 
 		if (dst.has(it->first))
diff --git a/indra/newview/llviewerassetstats.h b/indra/newview/llviewerassetstats.h
index 65ecdca4a0..cb63b9c511 100644
--- a/indra/newview/llviewerassetstats.h
+++ b/indra/newview/llviewerassetstats.h
@@ -159,11 +159,43 @@ public:
 	void recordGetServiced(LLViewerAssetType::EType at, bool with_http, bool is_temp, duration_t duration);
 
 	// Retrieve current metrics for all visited regions (NULL region UUID excluded)
+    // Returned LLSD is structured as follows:
+	//
+	// &stats_group = {
+	//   enqueued   : int,
+	//   dequeued   : int,
+	//   resp_count : int,
+	//   resp_min   : float,
+	//   resp_max   : float,
+	//   resp_mean  : float
+	// }
+	//
+	// {
+	//   duration: int
+	//   regions: {
+	//     $: {
+	//       duration:                 : int,
+	//       get_texture_temp_http     : &stats_group,
+	//       get_texture_temp_udp      : &stats_group,
+	//       get_texture_non_temp_http : &stats_group,
+	//       get_texture_non_temp_udp  : &stats_group,
+	//       get_wearable_udp          : &stats_group,
+	//       get_sound_udp             : &stats_group,
+	//       get_gesture_udp           : &stats_group,
+	//       get_other                 : &stats_group
+	//     }
+	//   }
+	// }
 	LLSD asLLSD();
 
-	// Merge two LLSD's structured as per asLLSD().  If inputs are not
-	// correctly formed, result is undefined (little defensive action).
-	static void mergeLLSD(const LLSD & src, LLSD & dst);
+	// Merges the "regions" maps in two LLSDs structured as per asLLSD().
+	// This takes two LLSDs as returned by asLLSD() and intelligently
+	// merges the metrics contained in the maps indexed by "regions".
+	// The remainder of the top-level map of the LLSDs is left unchanged
+	// in expectation that callers will add other information at this
+	// level.  The "regions" information must be correctly formed or the
+	// final result is undefined (little defensive action).
+	static void mergeRegionsLLSD(const LLSD & src, LLSD & dst);
 	
 protected:
 	typedef std::map<LLUUID, LLPointer<PerRegionStats> > PerRegionContainer;
diff --git a/indra/newview/tests/llviewerassetstats_test.cpp b/indra/newview/tests/llviewerassetstats_test.cpp
index e8cde5fc5d..a44712e8ad 100644
--- a/indra/newview/tests/llviewerassetstats_test.cpp
+++ b/indra/newview/tests/llviewerassetstats_test.cpp
@@ -492,7 +492,7 @@ namespace tut
 			dst["regions"][reg2_name] = reg2_stats;
 			dst["duration"] = 36;
 
-			LLViewerAssetStats::mergeLLSD(src, dst);
+			LLViewerAssetStats::mergeRegionsLLSD(src, dst);
 		
 			ensure("region 1 in merged stats", llsd_equals(reg1_stats, dst["regions"][reg1_name]));
 			ensure("region 2 still in merged stats", llsd_equals(reg2_stats, dst["regions"][reg2_name]));
@@ -507,7 +507,7 @@ namespace tut
 			dst["regions"][reg1_name] = reg2_stats;
 			dst["duration"] = 36;
 
-			LLViewerAssetStats::mergeLLSD(src, dst);
+			LLViewerAssetStats::mergeRegionsLLSD(src, dst);
 
 			ensure("src not ruined", llsd_equals(reg1_stats, src["regions"][reg1_name]));
 			ensure_equals("added enqueued counts", dst["regions"][reg1_name]["get_other"]["enqueued"].asInteger(), 12);