diff options
| -rwxr-xr-x | indra/newview/llmediadataclient.cpp | 137 | ||||
| -rwxr-xr-x | indra/newview/llmediadataclient.h | 8 | ||||
| -rw-r--r-- | indra/newview/llvovolume.cpp | 6 | ||||
| -rw-r--r-- | indra/newview/tests/llmediadataclient_test.cpp | 16 |
4 files changed, 111 insertions, 56 deletions
diff --git a/indra/newview/llmediadataclient.cpp b/indra/newview/llmediadataclient.cpp index b8da368bd7..8f5290bac2 100755 --- a/indra/newview/llmediadataclient.cpp +++ b/indra/newview/llmediadataclient.cpp @@ -58,6 +58,32 @@ // - Any request that gets a 503 still goes through the retry logic // +/*************************************************************************************************************** + What's up with this queueing code? + + First, a bit of background: + + Media on a prim was added into the system in the Viewer 2.0 timeframe. In order to avoid changing the + network format of objects, an unused field in the object (the "MediaURL" string) was repurposed to + indicate that the object had media data, and also hold a sequence number and the UUID of the agent + who last updated the data. The actual media data for objects is accessed via the "ObjectMedia" capability. + Due to concerns about sim performance, requests to this capability are rate-limited to 5 requests every + 5 seconds per agent. + + The initial implementation of LLMediaDataClient used a single queue to manage requests to the "ObjectMedia" cap. + Requests to the cap were queued so that objects closer to the avatar were loaded in first, since they were most + likely to be the ones the media performance manager would load. + + This worked in some cases, but we found that it was possible for a scripted object that constantly updated its + media data to starve other objects, since the same queue contained both requests to load previously unseen media + data and requests to fetch media data in response to object updates. + + The solution for this we came up with was to have two queues. The sorted queue contains requests to fetch media + data for objects that don't have it yet, and the round-robin queue contains requests to update media data for + objects that have already completed their initial load. When both queues are non-empty, the code ping-pongs + between them so that updates can't completely block initial load-in. +**************************************************************************************************************/ + // // Forward decls // @@ -149,7 +175,7 @@ void LLMediaDataClient::request(const LLMediaDataClientObject::ptr_t &object, co enqueue(new Request(getCapabilityName(), payload, object, this)); } -void LLMediaDataClient::enqueue(const Request *request) +void LLMediaDataClient::enqueue(Request *request) { if (request->isNew()) { @@ -161,8 +187,7 @@ void LLMediaDataClient::enqueue(const Request *request) LL_DEBUGS("LLMediaDataClient") << "Queuing SORTED request for " << *request << LL_ENDL; - // Sadly, we have to const-cast because items put into the queue are not const - mSortedQueue.push_back(const_cast<LLMediaDataClient::Request*>(request)); + mSortedQueue.push_back(request); LL_DEBUGS("LLMediaDataClientQueue") << "SORTED queue:" << mSortedQueue << LL_ENDL; } @@ -184,8 +209,7 @@ void LLMediaDataClient::enqueue(const Request *request) { LL_DEBUGS("LLMediaDataClient") << "Queuing RR request for " << *request << LL_ENDL; // Push the request on the pending queue - // Sadly, we have to const-cast because items put into the queue are not const - mRoundRobinQueue.push_front(const_cast<LLMediaDataClient::Request*>(request)); + mRoundRobinQueue.push_front(request); LL_DEBUGS("LLMediaDataClientQueue") << "RR queue:" << mRoundRobinQueue << LL_ENDL; } @@ -240,17 +264,64 @@ bool LLMediaDataClient::processQueueTimer() return isEmpty(); } +bool LLMediaDataClient::request_needs_purge(LLMediaDataClient::request_ptr_t request) +{ + // Check for conditions that should cause a request to get removed from the queue + if(request.isNull()) + { + LL_WARNS("LLMediaDataClient") << "removing NULL request" << LL_ENDL; + return true; + } + + // For some reason, the existing code put sent items onto the back of the round-robin queue and let them come through again + // before stripping them off the front. Was there a good reason for this? +// if(request->isMarkedSent()) +// { +// LL_INFOS("LLMediaDataClient") << "removing : " << *request << " request is marked sent" << LL_ENDL; +// return true; +// } + + const LLMediaDataClientObject *object = request->getObject(); + + if(object == NULL) + { + LL_INFOS("LLMediaDataClient") << "removing : " << *request << " object is NULL" << LL_ENDL; + return true; + } + + if(object->isDead()) + { + LL_INFOS("LLMediaDataClient") << "removing : " << *request << " object is dead" << LL_ENDL; + return true; + } + + if(!object->hasMedia()) + { + LL_INFOS("LLMediaDataClient") << "removing : " << *request << " object has no media" << LL_ENDL; + return true; + } + + return false; +} + void LLMediaDataClient::sortQueue() { if(!mSortedQueue.empty()) { - // Score all items first + // Cull dead objects and score all remaining items first request_queue_t::iterator iter = mSortedQueue.begin(); request_queue_t::iterator end = mSortedQueue.end(); while (iter != end) { - (*iter)->updateScore(); - iter++; + if(request_needs_purge(*iter)) + { + iter = mSortedQueue.erase(iter); + } + else + { + (*iter)->updateScore(); + iter++; + } } // Re-sort the list... @@ -270,6 +341,19 @@ void LLMediaDataClient::sortQueue() } } } + + // Cull dead items from the round robin queue as well. + for(request_queue_t::iterator iter = mRoundRobinQueue.begin(); iter != mRoundRobinQueue.end();) + { + if(request_needs_purge(*iter)) + { + iter = mRoundRobinQueue.erase(iter); + } + else + { + iter++; + } + } } // static @@ -298,26 +382,14 @@ void LLMediaDataClient::serviceQueue() llassert(!request.isNull()); const LLMediaDataClientObject *object = (request.isNull()) ? NULL : request->getObject(); llassert(NULL != object); - + // Check for conditions that would make us just pop and rapidly loop through // the queue. - if(request.isNull() || - request->isMarkedSent() || - NULL == object || - object->isDead() || - !object->hasMedia()) + if(request->isMarkedSent()) { - if (request.isNull()) - { - LL_WARNS("LLMediaDataClient") << "Skipping NULL request" << LL_ENDL; - } - else { - LL_INFOS("LLMediaDataClient") << "Skipping : " << *request << " " - << ((request->isMarkedSent()) ? " request is marked sent" : - ((NULL == object) ? " object is NULL " : - ((object->isDead()) ? "object is dead" : - ((!object->hasMedia()) ? "object has no media!" : "BADNESS!")))) << LL_ENDL; - } + // Sent items that make it back to the head of the queue need to be skipped. + LL_INFOS("LLMediaDataClient") << "removing : " << *request << " request is marked sent" << LL_ENDL; + queue_p->pop_front(); continue; // jump back to the start of the quick retry loop } @@ -448,24 +520,15 @@ LLMediaDataClient::Responder::RetryTimer::RetryTimer(F32 time, Responder *mdr) { } -// virtual -LLMediaDataClient::Responder::RetryTimer::~RetryTimer() +// virtual +BOOL LLMediaDataClient::Responder::RetryTimer::tick() { - LL_DEBUGS("LLMediaDataClient") << "~RetryTimer" << *(mResponder->getRequest()) << LL_ENDL; - - // XXX This is weird: Instead of doing the work in tick() (which re-schedules - // a timer, which might be risky), do it here, in the destructor. Yes, it is very odd. - // Instead of retrying, we just put the request back onto the queue LL_INFOS("LLMediaDataClient") << "RetryTimer fired for: " << *(mResponder->getRequest()) << " retrying" << LL_ENDL; mResponder->getRequest()->reEnqueue(); // Release the ref to the responder. mResponder = NULL; -} -// virtual -BOOL LLMediaDataClient::Responder::RetryTimer::tick() -{ // Don't fire again return TRUE; } @@ -552,7 +615,7 @@ const char *LLMediaDataClient::Request::getTypeAsString() const } -void LLMediaDataClient::Request::reEnqueue() const +void LLMediaDataClient::Request::reEnqueue() { // I sure hope this doesn't deref a bad pointer: mMDC->enqueue(this); diff --git a/indra/newview/llmediadataclient.h b/indra/newview/llmediadataclient.h index 8dd72cb595..3e86c52c63 100755 --- a/indra/newview/llmediadataclient.h +++ b/indra/newview/llmediadataclient.h @@ -142,7 +142,7 @@ protected: const char *getTypeAsString() const; // Re-enqueue thyself - void reEnqueue() const; + void reEnqueue(); F32 getRetryTimerDelay() const; U32 getMaxNumRetries() const; @@ -185,7 +185,7 @@ protected: //If we get back a normal response, handle it here. Default just logs it. virtual void result(const LLSD& content); - const request_ptr_t &getRequest() const { return mRequest; } + request_ptr_t &getRequest() { return mRequest; } protected: virtual ~Responder(); @@ -196,7 +196,6 @@ protected: { public: RetryTimer(F32 time, Responder *); - virtual ~RetryTimer(); virtual BOOL tick(); private: // back-pointer @@ -214,13 +213,14 @@ protected: // Subclasses must override to return a cap name virtual const char *getCapabilityName() const = 0; + virtual bool request_needs_purge(request_ptr_t request); virtual void sortQueue(); virtual void serviceQueue(); private: typedef std::list<request_ptr_t> request_queue_t; - void enqueue(const Request*); + void enqueue(Request*); // Return whether the given object is/was in the queue static LLMediaDataClient::request_ptr_t findOrRemove(request_queue_t &queue, const LLMediaDataClientObject::ptr_t &obj, bool remove, Request::Type type); diff --git a/indra/newview/llvovolume.cpp b/indra/newview/llvovolume.cpp index 5644e02134..c838e9a28d 100644 --- a/indra/newview/llvovolume.cpp +++ b/indra/newview/llvovolume.cpp @@ -2025,12 +2025,12 @@ void LLVOVolume::mediaNavigated(LLViewerMediaImpl *impl, LLPluginClassMedia* plu } else { - llwarns << "Couldn't find media entry!" << llendl; + LL_WARNS("MediaOnAPrim") << "Couldn't find media entry!" << LL_ENDL; } if(block_navigation) { - llinfos << "blocking navigate to URI " << new_location << llendl; + LL_INFOS("MediaOnAPrim") << "blocking navigate to URI " << new_location << LL_ENDL; // "bounce back" to the current URL from the media entry mediaNavigateBounceBack(face_index); @@ -2038,7 +2038,7 @@ void LLVOVolume::mediaNavigated(LLViewerMediaImpl *impl, LLPluginClassMedia* plu else if (sObjectMediaNavigateClient) { - llinfos << "broadcasting navigate with URI " << new_location << llendl; + LL_DEBUGS("MediaOnAPrim") << "broadcasting navigate with URI " << new_location << LL_ENDL; sObjectMediaNavigateClient->navigate(new LLMediaDataClientObjectImpl(this, false), face_index, new_location); } diff --git a/indra/newview/tests/llmediadataclient_test.cpp b/indra/newview/tests/llmediadataclient_test.cpp index 33d413bd21..d73ea24768 100644 --- a/indra/newview/tests/llmediadataclient_test.cpp +++ b/indra/newview/tests/llmediadataclient_test.cpp @@ -580,25 +580,17 @@ namespace tut ::pump_timers(); - // The first tick should remove the first one + // The first tick should remove the second and fourth ones, and process the first one ensure("is not in queue 1", !mdc->isInQueue(o1)); - ensure("is in queue 2", mdc->isInQueue(o2)); + ensure("is not in queue 2", !mdc->isInQueue(o2)); ensure("is in queue 3", mdc->isInQueue(o3)); - ensure("is in queue 4", mdc->isInQueue(o4)); + ensure("is not in queue 4", !mdc->isInQueue(o4)); ensure("post records", gPostRecords->size(), 1); ::pump_timers(); - // The second tick should skip the second and remove the third - ensure("is not in queue 2", !mdc->isInQueue(o2)); + // The second tick should process the third, emptying the queue ensure("is not in queue 3", !mdc->isInQueue(o3)); - ensure("is in queue 4", mdc->isInQueue(o4)); - ensure("post records", gPostRecords->size(), 2); - - ::pump_timers(); - - // The third tick should skip the fourth one and empty the queue. - ensure("is not in queue 4", !mdc->isInQueue(o4)); ensure("post records", gPostRecords->size(), 2); ensure("queue empty", mdc->isEmpty()); |