From 0007114cf5a60779319ab8cbd0a23a0d462b8010 Mon Sep 17 00:00:00 2001
From: Richard Linden <none@none>
Date: Mon, 5 Nov 2012 16:10:57 -0800
Subject: [PATCH] SH-3499 WIP Ensure asset stats output is correct fixed copy
behavior of recordings and accumulator buffers
---
indra/llcommon/llinitparam.cpp | 2 +-
indra/llcommon/llinitparam.h | 81 +++++------------
indra/llcommon/llpredicate.h | 90 +++++--------------
indra/llcommon/lltrace.h | 15 ++--
indra/llcommon/lltracerecording.cpp | 3 +-
indra/llcommon/lltracerecording.h | 29 +++++-
indra/llcommon/lltracethreadrecorder.cpp | 5 ++
indra/newview/llappviewer.cpp | 2 +
indra/newview/llviewerassetstats.cpp | 36 +++-----
indra/newview/llviewerassetstats.h | 41 ++-------
.../newview/tests/llviewerassetstats_test.cpp | 5 ++
11 files changed, 111 insertions(+), 198 deletions(-)
diff --git a/indra/llcommon/llinitparam.cpp b/indra/llcommon/llinitparam.cpp
index d20fc032277..32d4eec6079 100644
--- a/indra/llcommon/llinitparam.cpp
+++ b/indra/llcommon/llinitparam.cpp
@@ -35,7 +35,7 @@ namespace LLInitParam
predicate_rule_t default_parse_rules()
{
- return ll_make_predicate(PROVIDED) && !ll_make_predicate(EMPTY) && !ll_make_predicate(HAS_DEFAULT_VALUE);
+ return ll_make_predicate(PROVIDED) && !ll_make_predicate(EMPTY);
}
//
diff --git a/indra/llcommon/llinitparam.h b/indra/llcommon/llinitparam.h
index 6177cc7d12b..3d4e4331c0e 100644
--- a/indra/llcommon/llinitparam.h
+++ b/indra/llcommon/llinitparam.h
@@ -919,17 +919,8 @@ namespace LLInitParam
predicate.set(HAS_DEFAULT_VALUE);
}
- if (typed_param.isValid())
- {
- predicate.set(VALID, true);
- predicate.set(PROVIDED, typed_param.anyProvided());
- }
- else
- {
- predicate.set(VALID, false);
- predicate.set(PROVIDED, false);
- }
-
+ predicate.set(VALID, typed_param.isValid());
+ predicate.set(PROVIDED, typed_param.anyProvided());
predicate.set(EMPTY, false);
if (!predicate_rule.check(predicate)) return false;
@@ -1014,15 +1005,15 @@ namespace LLInitParam
};
// parameter that is a block
- template <typename T, typename NAME_VALUE_LOOKUP>
- class TypedParam<T, NAME_VALUE_LOOKUP, false, true>
+ template <typename BLOCK_T, typename NAME_VALUE_LOOKUP>
+ class TypedParam<BLOCK_T, NAME_VALUE_LOOKUP, false, true>
: public Param,
- public ParamValue<T, NAME_VALUE_LOOKUP>
+ public ParamValue<BLOCK_T, NAME_VALUE_LOOKUP>
{
public:
- typedef ParamValue<T, NAME_VALUE_LOOKUP> param_value_t;
+ typedef ParamValue<BLOCK_T, NAME_VALUE_LOOKUP> param_value_t;
typedef typename param_value_t::value_assignment_t value_assignment_t;
- typedef TypedParam<T, NAME_VALUE_LOOKUP, false, true> self_t;
+ typedef TypedParam<BLOCK_T, NAME_VALUE_LOOKUP, false, true> self_t;
typedef NAME_VALUE_LOOKUP name_value_lookup_t;
using param_value_t::operator();
@@ -1081,16 +1072,8 @@ namespace LLInitParam
LLPredicate::Value<ESerializePredicates> predicate;
- if (typed_param.isValid())
- {
- predicate.set(VALID, true);
- predicate.set(PROVIDED, typed_param.anyProvided());
- }
- else
- {
- predicate.set(VALID, false);
- predicate.set(PROVIDED, false);
- }
+ predicate.set(VALID, typed_param.isValid());
+ predicate.set(PROVIDED, typed_param.anyProvided());
if (!predicate_rule.check(predicate)) return false;
@@ -1187,13 +1170,13 @@ namespace LLInitParam
};
// container of non-block parameters
- template <typename VALUE_TYPE, typename NAME_VALUE_LOOKUP>
- class TypedParam<VALUE_TYPE, NAME_VALUE_LOOKUP, true, false>
+ template <typename MULTI_VALUE_T, typename NAME_VALUE_LOOKUP>
+ class TypedParam<MULTI_VALUE_T, NAME_VALUE_LOOKUP, true, false>
: public Param
{
public:
- typedef TypedParam<VALUE_TYPE, NAME_VALUE_LOOKUP, true, false> self_t;
- typedef ParamValue<VALUE_TYPE, NAME_VALUE_LOOKUP> param_value_t;
+ typedef TypedParam<MULTI_VALUE_T, NAME_VALUE_LOOKUP, true, false> self_t;
+ typedef ParamValue<MULTI_VALUE_T, NAME_VALUE_LOOKUP> param_value_t;
typedef typename std::vector<param_value_t> container_t;
typedef const container_t& value_assignment_t;
@@ -1280,18 +1263,8 @@ namespace LLInitParam
LLPredicate::Value<ESerializePredicates> predicate;
predicate.set(REQUIRED, typed_param.mMinCount > 0);
-
- if (typed_param.isValid())
- {
- predicate.set(VALID, true);
- predicate.set(PROVIDED, typed_param.anyProvided());
- }
- else
- {
- predicate.set(VALID, false);
- predicate.set(PROVIDED, false);
- }
-
+ predicate.set(VALID, typed_param.isValid());
+ predicate.set(PROVIDED, typed_param.anyProvided());
predicate.set(EMPTY, typed_param.mValues.empty());
if (!predicate_rule.check(predicate)) return false;
@@ -1345,7 +1318,7 @@ namespace LLInitParam
static void inspectParam(const Param& param, Parser& parser, Parser::name_stack_t& name_stack, S32 min_count, S32 max_count)
{
- parser.inspectValue<VALUE_TYPE>(name_stack, min_count, max_count, NULL);
+ parser.inspectValue<MULTI_VALUE_T>(name_stack, min_count, max_count, NULL);
if (name_value_lookup_t::getPossibleValues())
{
parser.inspectValue<std::string>(name_stack, min_count, max_count, name_value_lookup_t::getPossibleValues());
@@ -1437,13 +1410,13 @@ namespace LLInitParam
};
// container of block parameters
- template <typename VALUE_TYPE, typename NAME_VALUE_LOOKUP>
- class TypedParam<VALUE_TYPE, NAME_VALUE_LOOKUP, true, true>
+ template <typename MULTI_BLOCK_T, typename NAME_VALUE_LOOKUP>
+ class TypedParam<MULTI_BLOCK_T, NAME_VALUE_LOOKUP, true, true>
: public Param
{
public:
- typedef TypedParam<VALUE_TYPE, NAME_VALUE_LOOKUP, true, true> self_t;
- typedef ParamValue<VALUE_TYPE, NAME_VALUE_LOOKUP> param_value_t;
+ typedef TypedParam<MULTI_BLOCK_T, NAME_VALUE_LOOKUP, true, true> self_t;
+ typedef ParamValue<MULTI_BLOCK_T, NAME_VALUE_LOOKUP> param_value_t;
typedef typename std::vector<param_value_t> container_t;
typedef const container_t& value_assignment_t;
typedef typename param_value_t::value_t value_t;
@@ -1548,17 +1521,9 @@ namespace LLInitParam
LLPredicate::Value<ESerializePredicates> predicate;
predicate.set(REQUIRED, typed_param.mMinCount > 0);
-
- if (typed_param.isValid())
- {
- predicate.set(VALID, true);
- predicate.set(PROVIDED, typed_param.anyProvided());
- }
- else
- {
- predicate.set(VALID, false);
- predicate.set(PROVIDED, false);
- }
+ predicate.set(VALID, typed_param.isValid());
+ predicate.set(PROVIDED, typed_param.anyProvided());
+ predicate.set(EMPTY, typed_param.mValues.empty());
if (!predicate_rule.check(predicate)) return false;
diff --git a/indra/llcommon/llpredicate.h b/indra/llcommon/llpredicate.h
index 6c9e5fc145b..a0e970a7997 100644
--- a/indra/llcommon/llpredicate.h
+++ b/indra/llcommon/llpredicate.h
@@ -76,41 +76,31 @@ namespace LLPredicate
void set(ENUM e, bool value = true)
{
llassert(0 <= e && e < cMaxEnum);
- mPredicateFlags = modifyPredicate(0x1 << (S32)e, cPredicateFlagsFromEnum[e], value, mPredicateFlags);
- }
-
- void set(const Value other, bool value)
- {
- predicate_flag_t predicate_flags_to_set = other.mPredicateFlags;
- predicate_flag_t cumulative_flags = 0;
- while(predicate_flags_to_set)
- {
- predicate_flag_t next_flags = clearLSB(predicate_flags_to_set);
- predicate_flag_t lsb_flag = predicate_flags_to_set ^ next_flags;
-
- predicate_flag_t mask = 0;
- predicate_flag_t cur_flags = mPredicateFlags;
- for (S32 i = 0; i < cMaxEnum; i++)
- {
- if (cPredicateFlagsFromEnum[i] & lsb_flag)
- {
- mask |= cPredicateFlagsFromEnum[i];
- cur_flags = modifyPredicate(0x1 << (0x1 << i ), cPredicateFlagsFromEnum[i], !value, cur_flags);
- }
- }
-
- cumulative_flags |= modifyPredicate(lsb_flag, mask, value, cur_flags);
-
- predicate_flags_to_set = next_flags;
+ predicate_flag_t flags_to_modify;
+ predicate_flag_t mask = cPredicateFlagsFromEnum[e];
+ if (value)
+ { // add predicate "e" to flags that don't contain it already
+ flags_to_modify = (mPredicateFlags & ~mask);
+ // clear flags not containing e
+ mPredicateFlags &= mask;
+ // add back flags shifted to contain e
+ mPredicateFlags |= flags_to_modify << (0x1 << e);
+ }
+ else
+ { // remove predicate "e" from flags that contain it
+ flags_to_modify = (mPredicateFlags & mask);
+ // clear flags containing e
+ mPredicateFlags &= ~mask;
+ // add back flags shifted to not contain e
+ mPredicateFlags |= flags_to_modify >> (0x1 << e);
}
- mPredicateFlags = cumulative_flags;
}
- void forget(const Value value)
+ void forget(ENUM e)
{
- set(value, true);
+ set(e, true);
U32 flags_with_predicate = mPredicateFlags;
- set(value, false);
+ set(e, false);
// ambiguous value is result of adding and removing predicate at the same time!
mPredicateFlags |= flags_with_predicate;
}
@@ -131,38 +121,6 @@ namespace LLPredicate
}
private:
-
- predicate_flag_t clearLSB(predicate_flag_t value)
- {
- return value & (value - 1);
- }
-
- predicate_flag_t modifyPredicate(predicate_flag_t predicate_flag, predicate_flag_t mask, predicate_flag_t value, bool set)
- {
- llassert(clearLSB(predicate_flag) == 0);
- predicate_flag_t flags_to_modify;
-
- if (set)
- {
- flags_to_modify = (mPredicateFlags & ~mask);
- // clear flags not containing predicate to be added
- value &= mask;
- // shift flags, in effect adding predicate
- flags_to_modify *= predicate_flag;
- }
- else
- {
- flags_to_modify = mPredicateFlags & mask;
- // clear flags containing predicate to be removed
- value &= ~mask;
- // shift flags, in effect removing predicate
- flags_to_modify /= predicate_flag;
- }
- // put modified flags back
- value |= flags_to_modify;
- return value;
- }
-
predicate_flag_t mPredicateFlags;
};
@@ -181,14 +139,14 @@ namespace LLPredicate
Rule()
{}
- void require(const Value<ENUM> value)
+ void require(ENUM e)
{
- mRule.set(value, require);
+ mRule.set(e, require);
}
- void allow(const Value<ENUM> value)
+ void allow(ENUM e)
{
- mRule.forget(value);
+ mRule.forget(e);
}
bool check(const Value<ENUM> value) const
diff --git a/indra/llcommon/lltrace.h b/indra/llcommon/lltrace.h
index 735c45754cb..8ad391e6e5d 100644
--- a/indra/llcommon/lltrace.h
+++ b/indra/llcommon/lltrace.h
@@ -38,9 +38,9 @@
#include <list>
-#define TOKEN_PASTE_ACTUAL(x, y) x##y
-#define TOKEN_PASTE(x, y) TOKEN_PASTE_ACTUAL(x, y)
-#define RECORD_BLOCK_TIME(block_timer) LLTrace::BlockTimer::Recorder TOKEN_PASTE(block_time_recorder, __COUNTER__)(block_timer);
+#define LL_TOKEN_PASTE_ACTUAL(x, y) x##y
+#define LL_TOKEN_PASTE(x, y) LL_TOKEN_PASTE_ACTUAL(x, y)
+#define LL_RECORD_BLOCK_TIME(block_timer) LLTrace::BlockTimer::Recorder LL_TOKEN_PASTE(block_time_recorder, __COUNTER__)(block_timer);
namespace LLTrace
{
@@ -93,13 +93,16 @@ namespace LLTrace
public:
- // copying an accumulator buffer does not copy the actual contents, but simply initializes the buffer size
- // to be identical to the other buffer
AccumulatorBuffer(const AccumulatorBuffer& other = getDefaultBuffer())
: mStorageSize(other.mStorageSize),
mStorage(new ACCUMULATOR[other.mStorageSize]),
mNextStorageSlot(other.mNextStorageSlot)
- {}
+ {
+ for (S32 i = 0; i < mNextStorageSlot; i++)
+ {
+ mStorage[i] = other.mStorage[i];
+ }
+ }
~AccumulatorBuffer()
{
diff --git a/indra/llcommon/lltracerecording.cpp b/indra/llcommon/lltracerecording.cpp
index a8e1a5eec96..9cdd89c223d 100644
--- a/indra/llcommon/lltracerecording.cpp
+++ b/indra/llcommon/lltracerecording.cpp
@@ -52,9 +52,8 @@ Recording::~Recording()
void Recording::update()
{
if (isStarted())
-{
+ {
LLTrace::get_thread_recorder()->update(this);
- mElapsedSeconds = 0.0;
mSamplingTimer.reset();
}
}
diff --git a/indra/llcommon/lltracerecording.h b/indra/llcommon/lltracerecording.h
index 3a786c13573..e9949491503 100644
--- a/indra/llcommon/lltracerecording.h
+++ b/indra/llcommon/lltracerecording.h
@@ -53,16 +53,20 @@ class LL_COMMON_API LLStopWatchControlsMixinCommon
void restart();
void reset();
- bool isStarted() { return mPlayState == STARTED; }
- bool isPaused() { return mPlayState == PAUSED; }
- bool isStopped() { return mPlayState == STOPPED; }
- EPlayState getPlayState() { return mPlayState; }
+ bool isStarted() const { return mPlayState == STARTED; }
+ bool isPaused() const { return mPlayState == PAUSED; }
+ bool isStopped() const { return mPlayState == STOPPED; }
+ EPlayState getPlayState() const { return mPlayState; }
protected:
LLStopWatchControlsMixinCommon()
: mPlayState(STOPPED)
{}
+ // derived classes can call this from their constructor in order
+ // to enforce invariants
+ void forceState(EPlayState state) { mPlayState = state; }
+
private:
// trigger data accumulation (without reset)
virtual void handleStart() = 0;
@@ -102,6 +106,23 @@ namespace LLTrace
public:
Recording();
+ Recording(const Recording& other)
+ {
+ mSamplingTimer = other.mSamplingTimer;
+ mElapsedSeconds = other.mElapsedSeconds;
+ mCountsFloat = other.mCountsFloat;
+ mMeasurementsFloat = other.mMeasurementsFloat;
+ mCounts = other.mCounts;
+ mMeasurements = other.mMeasurements;
+ mStackTimers = other.mStackTimers;
+
+ if (other.isStarted())
+ {
+ handleStart();
+ }
+
+ forceState(other.getPlayState());
+ }
~Recording();
void makePrimary();
diff --git a/indra/llcommon/lltracethreadrecorder.cpp b/indra/llcommon/lltracethreadrecorder.cpp
index 0feb3ab7af8..af66a694920 100644
--- a/indra/llcommon/lltracethreadrecorder.cpp
+++ b/indra/llcommon/lltracethreadrecorder.cpp
@@ -86,6 +86,11 @@ std::list<ThreadRecorder::ActiveRecording>::iterator ThreadRecorder::update( Rec
}
}
+ if (it == end_it)
+ {
+ llerrs << "Recording not active on this thread" << llendl;
+ }
+
return it;
}
diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp
index 2d090f0f74e..838a982cb40 100644
--- a/indra/newview/llappviewer.cpp
+++ b/indra/newview/llappviewer.cpp
@@ -5244,6 +5244,8 @@ void LLAppViewer::metricsSend(bool enable_reporting)
// Make a copy of the main stats to send into another thread.
// Receiving thread takes ownership.
LLViewerAssetStats * main_stats(new LLViewerAssetStats(*gViewerAssetStats));
+
+ main_stats->updateStats();
// Send a report request into 'thread1' to get the rest of the data
// and provide some additional parameters while here.
diff --git a/indra/newview/llviewerassetstats.cpp b/indra/newview/llviewerassetstats.cpp
index df43c4b3445..000d061bece 100755
--- a/indra/newview/llviewerassetstats.cpp
+++ b/indra/newview/llviewerassetstats.cpp
@@ -246,9 +246,11 @@ LLViewerAssetStats::LLViewerAssetStats()
LLViewerAssetStats::LLViewerAssetStats(const LLViewerAssetStats & src)
: mRegionHandle(src.mRegionHandle),
mPhaseStats(src.mPhaseStats),
- mAvatarRezStates(src.mAvatarRezStates),
- mRegionRecordings(src.mRegionRecordings)
+ mAvatarRezStates(src.mAvatarRezStates)
{
+ src.mCurRecording->update();
+ mRegionRecordings = src.mRegionRecordings;
+
mCurRecording = &mRegionRecordings[mRegionHandle];
}
@@ -294,14 +296,17 @@ void LLViewerAssetStats::recordAvatarStats()
mPhaseStats["cloud-or-gray"] = LLViewerStats::PhaseMap::getPhaseStats("cloud-or-gray");
}
-void LLViewerAssetStats::getStats(AssetStats& stats, bool compact_output)
+void LLViewerAssetStats::updateStats()
{
- using namespace LLViewerAssetStatsFF;
-
- if (mCurRecording)
+ if (mCurRecording && mCurRecording->isStarted())
{
mCurRecording->update();
}
+}
+
+void LLViewerAssetStats::getStats(AssetStats& stats, bool compact_output)
+{
+ using namespace LLViewerAssetStatsFF;
stats.regions.setProvided();
@@ -463,25 +468,6 @@ LLSD LLViewerAssetStats::asLLSD(bool compact_output)
namespace LLViewerAssetStatsFF
{
-//
-// Target thread is elaborated in the function name. This could
-// have been something 'templatey' like specializations iterated
-// over a set of constants but with so few, this is clearer I think.
-//
-// As for the threads themselves... rather than do fine-grained
-// locking as we gather statistics, this code creates a collector
-// for each thread, allocated and run independently. Logging
-// happens at relatively infrequent intervals and at that time
-// the data is sent to a single thread to be aggregated into
-// a single entity with locks, thread safety and other niceties.
-//
-// A particularly fussy implementation would distribute the
-// per-thread pointers across separate cache lines. But that should
-// be beyond current requirements.
-//
-
-// 'main' thread - initial program thread
-
void set_region(LLViewerAssetStats::region_handle_t region_handle)
{
if (! gViewerAssetStats)
diff --git a/indra/newview/llviewerassetstats.h b/indra/newview/llviewerassetstats.h
index f1ce3aeaa2f..835df891498 100755
--- a/indra/newview/llviewerassetstats.h
+++ b/indra/newview/llviewerassetstats.h
@@ -192,43 +192,12 @@ class LLViewerAssetStats
// Avatar-related statistics
void recordAvatarStats();
+ // gather latest metrics data
+ // call from main thread
+ void updateStats();
+
// Retrieve current metrics for all visited regions (NULL region UUID/handle 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
- // }
- //
- // &mmm_group = {
- // count : int,
- // min : float,
- // max : float,
- // mean : float
- // }
- //
- // {
- // duration: int
- // regions: {
- // $: { // Keys are strings of the region's handle in hex
- // duration: : int,
- // fps: : &mmm_group,
- // 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
- // }
- // }
- // }
- //
+ // Uses AssetStats structure seen above
void getStats(AssetStats& stats, bool compact_output);
LLSD asLLSD(bool compact_output);
diff --git a/indra/newview/tests/llviewerassetstats_test.cpp b/indra/newview/tests/llviewerassetstats_test.cpp
index 26a76eda2c5..1f299abe414 100755
--- a/indra/newview/tests/llviewerassetstats_test.cpp
+++ b/indra/newview/tests/llviewerassetstats_test.cpp
@@ -340,6 +340,7 @@ namespace tut
LLViewerAssetStatsFF::record_enqueue(LLViewerAssetType::AT_BODYPART, false, false);
LLViewerAssetStatsFF::record_dequeue(LLViewerAssetType::AT_BODYPART, false, false);
+ gViewerAssetStats->updateStats();
LLSD sd = gViewerAssetStats->asLLSD(false);
ensure("Correct single-key LLSD map root", is_triple_key_map(sd, "regions", "duration", "avatar"));
ensure("Correct single-slot LLSD array regions", is_single_slot_array(sd["regions"], region1_handle));
@@ -377,6 +378,7 @@ namespace tut
LLViewerAssetStatsFF::record_enqueue(LLViewerAssetType::AT_BODYPART, false, false);
LLViewerAssetStatsFF::record_dequeue(LLViewerAssetType::AT_BODYPART, false, false);
+ gViewerAssetStats->updateStats();
LLSD sd = gViewerAssetStats->asLLSD(false);
ensure("Correct single-key LLSD map root", is_triple_key_map(sd, "regions", "duration", "avatar"));
ensure("Correct single-slot LLSD array regions", is_single_slot_array(sd["regions"], region1_handle));
@@ -422,6 +424,7 @@ namespace tut
LLViewerAssetStatsFF::record_enqueue(LLViewerAssetType::AT_GESTURE, false, false);
LLViewerAssetStatsFF::record_enqueue(LLViewerAssetType::AT_GESTURE, false, false);
+ gViewerAssetStats->updateStats();
LLSD sd = gViewerAssetStats->asLLSD(false);
// std::cout << sd << std::endl;
@@ -496,6 +499,7 @@ namespace tut
LLViewerAssetStatsFF::record_enqueue(LLViewerAssetType::AT_GESTURE, false, false);
LLViewerAssetStatsFF::record_enqueue(LLViewerAssetType::AT_GESTURE, false, false);
+ gViewerAssetStats->updateStats();
LLSD sd = gViewerAssetStats->asLLSD(false);
ensure("Correct double-key LLSD map root", is_triple_key_map(sd, "duration", "regions", "avatar"));
@@ -563,6 +567,7 @@ namespace tut
LLViewerAssetStatsFF::record_enqueue(LLViewerAssetType::AT_LSL_BYTECODE, true, true);
+ gViewerAssetStats->updateStats();
LLSD sd = gViewerAssetStats->asLLSD(false);
ensure("Correct single-key LLSD map root", is_triple_key_map(sd, "regions", "duration", "avatar"));
ensure("Correct single-slot LLSD array regions", is_single_slot_array(sd["regions"], region1_handle));
--
GitLab