From fdda86251cc597dff3d2bd6b80a7a880db21dd96 Mon Sep 17 00:00:00 2001 From: Rye Date: Tue, 3 Nov 2026 03:19:44 -0500 Subject: [PATCH 1/4] A region's object cache flush leaves nothing pointing at what it frees An update whose texture entry runs past its end flushes the region's object cache through LLVOCache::removeEntry, which since LL's #1340 also clears the region's map of cached objects. The map holds the only references to them, and they were freed while still in use. The update had been read from one of them, by cacheFullUpdate's decodeBoundingInfo or by createVisibleObjects's addNewObject, whose waiting list it was walking; and the region's active, waiting and visible sets, the cache's tree, the drawables' octree entries and the eviction walk's cursor kept bare pointers to every one. On develop the update went on to read its packer, which was the freed entry's. The update now asks for the removal (LLVOCache::requestRemoval), and LLWorld::updateRegions carries it out before the regions' updates, outside any update of an object. Clearing a region's map takes every entry out of what points at it first (LLVOCacheEntry::releaseAll): the sets and the waiting list are emptied, each entry leaves the cache's tree and its octree entry and is marked invalid, and the references go to the release saveObjectCache already hands its entries to. The same clear is reached from a failed write in saveObjectCache on a region still in the world, at retirement or a forced disconnect. The request and the release are tested; the region, LLVOVolume and LLWorld, which newview_tests does not build, are not. Nothing has been run in world or under a sanitizer. Co-Authored-By: Claude Opus 5.5 --- indra/newview/llviewerregion.cpp | 10 +++- indra/newview/llvocache.cpp | 44 +++++++++++++++++ indra/newview/llvocache.h | 15 ++++++ indra/newview/llvovolume.cpp | 7 ++- indra/newview/llworld.cpp | 7 +++ indra/newview/tests/llvieweroctree_stub.cpp | 1 + indra/newview/tests/llvocache_test.cpp | 53 +++++++++++++++++++++ 7 files changed, 134 insertions(+), 3 deletions(-) diff --git a/indra/newview/llviewerregion.cpp b/indra/newview/llviewerregion.cpp index 93a2be6c94f..128dc06cc0b 100644 --- a/indra/newview/llviewerregion.cpp +++ b/indra/newview/llviewerregion.cpp @@ -3256,7 +3256,15 @@ void LLViewerRegion::dumpCache() void LLViewerRegion::clearVOCacheFromMemory() { - mImpl->mCacheMap.clear(); + // The map holds the only references to its entries, and the sets, the + // waiting list, the cache's tree, the drawables' octree entries and the + // eviction walk's cursor hold bare pointers to them. + std::vector> released; + LLVOCacheEntry::releaseAll(mImpl->mCacheMap, { &mImpl->mActiveSet, &mImpl->mWaitingSet, &mImpl->mVisibleEntries }, + [this](LLVOCacheEntry* entry) { removeFromVOCacheTree(entry); }, + LLAppViewer::instance()->isQuitting() ? released : sRegionCacheCleanup); + mImpl->mWaitingList.clear(); + mLastVisitedEntry = NULL; } void LLViewerRegion::unpackRegionHandshake() diff --git a/indra/newview/llvocache.cpp b/indra/newview/llvocache.cpp index a7d950cf7e5..179a712da3a 100644 --- a/indra/newview/llvocache.cpp +++ b/indra/newview/llvocache.cpp @@ -39,6 +39,7 @@ #include +#include #include #include @@ -273,6 +274,31 @@ LLVOCacheEntry::~LLVOCacheEntry() mDP.freeBuffer(); } +// static +void LLVOCacheEntry::releaseAll(vocache_entry_map_t& entries, std::initializer_list sets, + const std::function& leave_tree, std::vector>& keep) +{ + keep.reserve(keep.size() + entries.size()); + for (auto& [local_id, entry] : entries) + { + if (entry.isNull()) + { + continue; + } + leave_tree(entry); + entry->removeOctreeEntry(); + entry->setValid(false); + keep.push_back(std::move(entry)); + } + entries.clear(); + + // Every entry a set can hold is one of the map's. + for (vocache_entry_set_t* set : sets) + { + set->clear(); + } +} + void LLVOCacheEntry::updateEntry(U32 crc, LLDataPackerBinaryBuffer &dp) { if(mCRC != crc) @@ -1426,6 +1452,24 @@ void LLVOCache::removeEntry(U64 handle) removeEntry(entry) ; } +void LLVOCache::requestRemoval(U64 handle) +{ + if (std::find(mRequestedRemovals.begin(), mRequestedRemovals.end(), handle) == mRequestedRemovals.end()) + { + mRequestedRemovals.push_back(handle); + } +} + +void LLVOCache::removeRequestedEntries() +{ + std::vector handles; + handles.swap(mRequestedRemovals); + for (U64 handle : handles) + { + removeEntry(handle); + } +} + void LLVOCache::clearCacheInMemory() { if(!mHeaderEntryQueue.empty()) diff --git a/indra/newview/llvocache.h b/indra/newview/llvocache.h index 4bb2116401e..12700cc2271 100644 --- a/indra/newview/llvocache.h +++ b/indra/newview/llvocache.h @@ -36,6 +36,10 @@ #include #include +#include +#include +#include + //--------------------------------------------------------------------------- // Cache entries class LLCamera; @@ -210,6 +214,12 @@ class LLVOCacheEntry typedef boost::unordered_map vocache_gltf_overrides_map_t; + // Lets every entry of a region's map go, leaving nothing that points at one: each leaves the cache's tree + // through `leave_tree` and the octree entry it may share with a drawable, and is marked invalid, and `sets`, + // which hold bare pointers to the map's entries, are emptied. The map's references go to `keep`. + static void releaseAll(vocache_entry_map_t& entries, std::initializer_list sets, + const std::function& leave_tree, std::vector>& keep); + // [SL:KB] - Patch: World-Derender | Checked: 2014-08-10 (Catznip-3.7) vocache_entry_set_t::const_iterator getChildrenBegin() const { return mChildrenList.begin(); } vocache_entry_set_t::const_iterator getChildrenEnd() const { return mChildrenList.end(); } @@ -347,6 +357,10 @@ class LLVOCache : public LLParamSingleton void writeToCache(U64 handle, const LLUUID& id, const LLVOCacheEntry::vocache_entry_map_t& cache_entry_map, bool dirty_cache, bool removal_enabled); void writeGenericExtrasToCache(U64 handle, const LLUUID& id, const LLVOCacheEntry::vocache_gltf_overrides_map_t& cache_extras_entry_map, bool dirty_cache, bool removal_enabled); void removeEntry(U64 handle) ; + // Removes a region's entry as removeEntry does, once removeRequestedEntries runs: the removal lets go of the + // region's objects in memory, one of which an object update asking for it is read from. + void requestRemoval(U64 handle); + void removeRequestedEntries(); void removeGenericExtrasForHandle(U64 handle); U32 getCacheEntries() { return mNumEntries; } @@ -378,6 +392,7 @@ class LLVOCache : public LLParamSingleton std::filesystem::path mObjectCacheDirPath; header_entry_queue_t mHeaderEntryQueue; handle_entry_map_t mHandleEntryMap; + std::vector mRequestedRemovals; }; #endif diff --git a/indra/newview/llvovolume.cpp b/indra/newview/llvovolume.cpp index 5ac98499dc8..e1e90e208b1 100644 --- a/indra/newview/llvovolume.cpp +++ b/indra/newview/llvovolume.cpp @@ -451,13 +451,16 @@ U32 LLVOVolume::processUpdateMessage(LLMessageSystem *mesgsys, if (cut != ALCompressedVolume::ECut::TEXTURE_ANIM) { // An update that ends before the end of its texture - // entry is bogus data, which flushes the region's cache. + // entry is bogus data, which flushes the region's cache + // once the update is done with: the flush lets go of the + // region's cached objects, and the update may be read + // from one of them. dp->dumpBufferToLog(); LL_WARNS() << "Flushing cache files" << LL_ENDL; if(LLVOCache::instanceExists() && getRegion()) { - LLVOCache::getInstance()->removeEntry(getRegion()->getHandle()) ; + LLVOCache::getInstance()->requestRemoval(getRegion()->getHandle()); } } LL_WARNS("UpdateFail") << (cut == ALCompressedVolume::ECut::VOLUME_PARAMS ? "Volume parameters" diff --git a/indra/newview/llworld.cpp b/indra/newview/llworld.cpp index 1c87c7c0ffc..7a6c5d7bf8a 100644 --- a/indra/newview/llworld.cpp +++ b/indra/newview/llworld.cpp @@ -695,6 +695,13 @@ void LLWorld::updateRegions(F32 max_update_time) // Before the regions' updates, since it may remove one. mRegionRetirement.update(); + // The cache removals object updates asked for, which let go of regions' + // cached objects, before the regions' updates walk them. + if (LLVOCache::instanceExists()) + { + LLVOCache::getInstance()->removeRequestedEntries(); + } + LLViewerRegion::calcNewObjectCreationThrottle(); if(LLViewerRegion::isNewObjectCreationThrottleDisabled()) { diff --git a/indra/newview/tests/llvieweroctree_stub.cpp b/indra/newview/tests/llvieweroctree_stub.cpp index e850efd9ac2..b2d1de60983 100644 --- a/indra/newview/tests/llvieweroctree_stub.cpp +++ b/indra/newview/tests/llvieweroctree_stub.cpp @@ -75,6 +75,7 @@ void LLViewerOctreeEntryData::setSpatialExtents(const LLVector4a& min, const LLV void LLViewerOctreeEntryData::setPositionGroup(const LLVector4a& pos) {} void LLViewerOctreeEntryData::setGroup(LLViewerOctreeGroup* group) {} void LLViewerOctreeEntryData::setOctreeEntry(LLViewerOctreeEntry* entry) {} +void LLViewerOctreeEntryData::removeOctreeEntry() {} U32 LLViewerOctreeEntryData::sCurVisible{}; LLViewerOctreePartition::LLViewerOctreePartition() = default; diff --git a/indra/newview/tests/llvocache_test.cpp b/indra/newview/tests/llvocache_test.cpp index 1673b3fa32f..e4fb3dc2388 100644 --- a/indra/newview/tests/llvocache_test.cpp +++ b/indra/newview/tests/llvocache_test.cpp @@ -42,6 +42,7 @@ #include "llvieweroctree_stub.cpp" +#include #include #include @@ -807,6 +808,58 @@ namespace CHECK_FALSE(std::filesystem::exists(objectFile(handle, "_extras.slec"))); } + // Removing a region's entry lets go of its objects in memory, and an + // object update that finds the cache bogus is read from one of them. + TEST_CASE_METHOD(vocacheTest, "LLVOCache: a removal asked for waits for the removals asked for to be carried out", "[LLVOCache]") + { + const U64 handle = to_region_handle(1023 * 256, 1000 * 256); + const LLUUID id = LLUUID::generateNewID(); + LLVOCache::instance().writeToCache(handle, id, makeEntries(3), true, false); + REQUIRE(LLVOCache::instance().getCacheEntries() == 1u); + + LLVOCache::instance().requestRemoval(handle); + LLVOCache::instance().requestRemoval(handle); + UNSCOPED_INFO("the region is still cached once its removal is asked for"); + CHECK(LLVOCache::instance().getCacheEntries() == 1u); + CHECK(std::filesystem::exists(objectFile(handle, ".slc"))); + + LLVOCache::instance().removeRequestedEntries(); + UNSCOPED_INFO("and gone once the removals are carried out"); + CHECK(LLVOCache::instance().getCacheEntries() == 0u); + CHECK_FALSE(std::filesystem::exists(objectFile(handle, ".slc"))); + + UNSCOPED_INFO("which leaves none asked for"); + LLVOCache::instance().writeToCache(handle, id, makeEntries(3), true, false); + LLVOCache::instance().removeRequestedEntries(); + CHECK(LLVOCache::instance().getCacheEntries() == 1u); + } + + TEST_CASE("LLVOCacheEntry: releaseAll leaves no set holding an entry it let go", "[LLVOCache]") + { + LLVOCacheEntry::vocache_entry_map_t entries = vocacheTest::makeEntries(6); + const LLPointer watched = entries[2]; + LLVOCacheEntry::vocache_entry_set_t active; + LLVOCacheEntry::vocache_entry_set_t visible; + active.insert(entries[1].get()); + active.insert(watched.get()); + visible.insert(entries[5].get()); + + std::vector left_tree; + std::vector> keep; + LLVOCacheEntry::releaseAll(entries, { &active, &visible }, [&](LLVOCacheEntry* entry) { left_tree.push_back(entry); }, keep); + + CHECK(entries.empty()); + CHECK(active.empty()); + CHECK(visible.empty()); + UNSCOPED_INFO("every entry left the tree"); + CHECK(left_tree.size() == 6u); + CHECK(std::find(left_tree.begin(), left_tree.end(), watched.get()) != left_tree.end()); + UNSCOPED_INFO("and is held by keep alone besides the test"); + CHECK(keep.size() == 6u); + CHECK(watched->getNumRefs() == 2); + CHECK_FALSE(watched->isValid()); + } + TEST_CASE_METHOD(vocacheTest, "LLVOCache: staysInMemory keeps entries seen lately or inside the rear sphere", "[LLVOCache]") { // A stand-in for an entry that nothing has looked at in a while: its From 670b85ad16a5aba0f51e5d40ad881874e5f11a78 Mon Sep 17 00:00:00 2001 From: Rye Date: Tue, 3 Nov 2026 03:19:47 -0500 Subject: [PATCH 2/4] A full inbound packet queue leaves a reliable packet out unacked Since LL's a1b68fb8ed a reliable packet taken into the inbound queue is acked as it comes off the socket, before it is queued. Once its priority ring was full at its ceiling of 8192 packets, LLPacketRing::pushPacket overwrote the oldest packet queued: one acked as it came in, which its sender never sent again, and whose number was recorded, so that a copy would have been taken for a duplicate. The ring now refuses a packet it has no room for, and bufferInboundPacket acks a reliable packet and records its number only where its ring has room. The packet left out is dropped as the network might have dropped it, and its sender sends it again. An unreliable packet is lost as one was before, the newest now rather than the oldest. A packet the network impairment holds back is acked as it arrives, and is still lost where the impairment releases it into a full ring. The test fills the low priority ring with reliable packets and sends one more. With the ring overwriting, that one was acked, and the first was never read, even once the last was sent again. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/llpacketring.cpp | 33 +++++-------- indra/llmessage/llpacketring.h | 16 +++--- indra/llmessage/message.cpp | 7 ++- indra/llmessage/tests/message_test.cpp | 68 ++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 27 deletions(-) diff --git a/indra/llmessage/llpacketring.cpp b/indra/llmessage/llpacketring.cpp index bade413e61e..3fc6b2e9ff1 100644 --- a/indra/llmessage/llpacketring.cpp +++ b/indra/llmessage/llpacketring.cpp @@ -59,35 +59,28 @@ LLPacketRing::~LLPacketRing() mHeadIndex = 0; } -void LLPacketRing::pushPacket(const LLPacketBuffer& packet) +bool LLPacketRing::hasRoom() const { - S16 ring_size = (S16)mRing.size(); - if (mNumBufferedPackets >= ring_size && ring_size < MAX_BUFFER_RING_SIZE) + return mNumBufferedPackets < (S16)mRing.size() || (S16)mRing.size() < MAX_BUFFER_RING_SIZE; +} + +bool LLPacketRing::pushPacket(const LLPacketBuffer& packet) +{ + if (mNumBufferedPackets >= (S16)mRing.size() && !expandRing()) { - expandRing(); - ring_size = (S16)mRing.size(); + LL_WARNS("PacketRing") << "buffer full at " << mNumBufferedPackets << " packets, leaving one out" << LL_ENDL; + return false; } + const S16 ring_size = (S16)mRing.size(); LLPacketBuffer* slot = mRing[mHeadIndex]; - S32 old_size = slot->getSize(); - *slot = packet; mHeadIndex = (mHeadIndex + 1) % ring_size; - if (mNumBufferedPackets < ring_size) - { - ++mNumBufferedPackets; - mNumBufferedBytes += packet.getSize(); - } - else - { - // Ring is at maximum capacity; oldest packet was overwritten. - // This is VERY BAD because we've already ACKed the packet we're loosing - // (if it was "reliable"). - LL_WARNS("PacketRing") << "buffer overflow at " << mNumBufferedPackets << " packets" << LL_ENDL; - mNumBufferedBytes += packet.getSize() - old_size; - } + ++mNumBufferedPackets; + mNumBufferedBytes += packet.getSize(); + return true; } bool LLPacketRing::popPacket(LLPacketBuffer& packet) diff --git a/indra/llmessage/llpacketring.h b/indra/llmessage/llpacketring.h index 5315e5a5bda..0081a3cf17e 100644 --- a/indra/llmessage/llpacketring.h +++ b/indra/llmessage/llpacketring.h @@ -8,9 +8,8 @@ * popPacket(). * * The ring starts at DEFAULT_BUFFER_RING_SIZE slots and grows in increments - * of BUFFER_RING_EXPANSION up to MAX_BUFFER_RING_SIZE. Once at the ceiling, - * pushPacket() silently overwrites the oldest queued packet to make room for - * the incoming one. + * of BUFFER_RING_EXPANSION up to MAX_BUFFER_RING_SIZE. Once full at the + * ceiling, pushPacket() refuses the incoming packet. * * $LicenseInfo:firstyear=2001&license=viewerlgpl$ * Second Life Viewer Source Code @@ -47,9 +46,14 @@ class LLPacketRing LLPacketRing(); ~LLPacketRing(); - // Copy 'packet' onto the tail of the ring, growing the ring or - // overwriting the oldest entry when the ring is at capacity. - void pushPacket(const LLPacketBuffer& packet); + // Copy 'packet' onto the tail of the ring, growing the ring when it is + // full. A ring full at its ceiling leaves the packet out and returns + // false: the oldest packet it holds may be a reliable one already acked, + // which its sender would never send again. + bool pushPacket(const LLPacketBuffer& packet); + + // Whether pushPacket() would take a packet now. + bool hasRoom() const; // Copy the head (oldest) packet into 'packet'. // Returns true if a packet was available, false if the ring was empty. diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp index c27cd9ed599..0d73ee571a3 100644 --- a/indra/llmessage/message.cpp +++ b/indra/llmessage/message.cpp @@ -1159,7 +1159,12 @@ S32 LLMessageSystem::bufferInboundPacket() } } - if (cdp) + // A queue full at its ceiling has no room for the packet, which is + // left out as the network might have dropped it: neither acked nor + // its number recorded, so that its sender sends it again. + const bool has_room = (isHighPriorityMessage(pkt) ? mHighPriorityInbound : mLowPriorityInbound).hasRoom(); + + if (cdp && has_room) { // ACK inbound reliable packet ASAP, so its sender stops resending // it while it waits in the queue, whatever checkMessages() makes diff --git a/indra/llmessage/tests/message_test.cpp b/indra/llmessage/tests/message_test.cpp index fd889d9ba95..dcbc32487eb 100644 --- a/indra/llmessage/tests/message_test.cpp +++ b/indra/llmessage/tests/message_test.cpp @@ -752,5 +752,73 @@ namespace } CHECK(ll_test::CircuitProbe::acksWaiting(*circuit) == 5); } + + // The packets a full queue holds were acked as they came in, so taking + // the place of the oldest would lose a packet its sender never sends + // again. The packet the queue has no room for is left out unacked. + TEST_CASE_METHOD(LLMessageSystemTestData, "LLMessageSystem: a full inbound queue leaves a reliable packet out unacked", "[LLMessageSystem]") + { + const std::string template_path = mTestConfigDir + mSep + "message_template.msg"; + const TemplateFile message_template(template_path, DUPLICATE_TEST_TEMPLATE); + LLMessageSystem system(template_path, NET_USE_OS_ASSIGNED_PORT, 1, 0, 0, false, 5.f, 100.f); + REQUIRE(system.isOK()); + const UseMessageSystem use(system); + system.setHandlerFunc("DuplicateTest", handle_duplicate_test); + sDuplicateTestsHandled = 0; + + const SenderSocket sender; + REQUIRE(sender.mResult == 0); + const LLHost sender_host("127.0.0.1", sender.mPort); + system.enableCircuit(sender_host, true); + const LLCircuitData* circuit = system.mCircuitInfo.findCircuit(sender_host); + REQUIRE(circuit != nullptr); + const LLHost receiver("127.0.0.1", system.getListenPort()); + + // DuplicateTest numbered `id`, a low priority message. + auto send = [&](U32 id, U8 flags) + { + const U8 packet[] = { flags, (U8)(id >> 24), (U8)(id >> 16), (U8)(id >> 8), (U8)id, 0, 0xff, 0xff, 0x00, 0x01, 1, 2, 3, 4 }; + REQUIRE(send_packet(sender.mSocket, (const char*)packet, (int)sizeof(packet), receiver.getAddress(), receiver.getPort())); + }; + + // The ring's ceiling, filled a few at a time so that the socket's + // buffer holds every one until it is queued. + constexpr S32 FULL = 8192; + U32 id = 0; + while (system.getNumBufferedPackets() < FULL) + { + const S32 batch = llmin(128, FULL - system.getNumBufferedPackets()); + for (S32 i = 0; i < batch; ++i) + { + send(++id, LL_RELIABLE_FLAG); + } + queue_until(system, (S32)id); + REQUIRE(system.getNumBufferedPackets() == (S32)id); + } + REQUIRE(ll_test::CircuitProbe::acksWaiting(*circuit) == FULL); + + const U32 left_out = id + 1; + send(left_out, LL_RELIABLE_FLAG); + const F64 give_up = LLTimer::getTotalSeconds() + 1.0; + while (system.getTotalNumDroppedPackets() < 1 && LLTimer::getTotalSeconds() < give_up) + { + system.drainUdpSocket(); + ms_sleep(1); + } + REQUIRE(system.getTotalNumDroppedPackets() == 1); + CHECK(system.getNumBufferedPackets() == FULL); + CHECK(ll_test::CircuitProbe::acksWaitingFor(*circuit, left_out) == 0); + CHECK(ll_test::CircuitProbe::acksWaiting(*circuit) == FULL); + + UNSCOPED_INFO("every packet queued is read, the first among them"); + read_queued(system); + CHECK(sDuplicateTestsHandled == FULL); + + UNSCOPED_INFO("and the one left out is taken when it comes again"); + send(left_out, LL_RELIABLE_FLAG | LL_RESENT_FLAG); + read_until(system, FULL + 1); + CHECK(sDuplicateTestsHandled == FULL + 1); + CHECK(ll_test::CircuitProbe::acksWaitingFor(*circuit, left_out) == 1); + } } From 010f7371f464a311d0ceedc6cf66770f8cefc03d Mon Sep 17 00:00:00 2001 From: Rye Date: Tue, 3 Nov 2026 04:27:56 -0500 Subject: [PATCH 3/4] A full inbound queue acks a copy it would drop, and an impaired packet on release A copy of a reliable packet already read is acked when its queue has no room for it, as it is when there is room. It was left out unacked, and its sender went on sending it until the queue drained. The duplicate check, which records nothing for a packet it has not seen, now runs before the check for room. A packet the network impairment holds back is no longer acked, nor its number checked, as it arrives. checkMessages does both as it reads the packet once it is released, and a queue with no room for it then leaves it out unacked, rather than losing a packet already acked. Its number is checked out of order, as the impairment means it to arrive. The warning for a packet left out is given once a second at the most, with how many were left out since the last. The full-queue test now fills either ring, checks the circuit's next number after the packet left out, and sends a copy of a packet already read into the full ring. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/llpacketring.cpp | 14 ++- indra/llmessage/llpacketring.h | 4 + indra/llmessage/message.cpp | 75 +++++++++------- indra/llmessage/tests/message_test.cpp | 113 +++++++++++++++++-------- 4 files changed, 140 insertions(+), 66 deletions(-) diff --git a/indra/llmessage/llpacketring.cpp b/indra/llmessage/llpacketring.cpp index 3fc6b2e9ff1..0c02ca14539 100644 --- a/indra/llmessage/llpacketring.cpp +++ b/indra/llmessage/llpacketring.cpp @@ -29,6 +29,7 @@ #include "llpacketring.h" #include "llerror.h" +#include "llframetimer.h" constexpr S16 MAX_BUFFER_RING_SIZE = 8192; @@ -68,7 +69,18 @@ bool LLPacketRing::pushPacket(const LLPacketBuffer& packet) { if (mNumBufferedPackets >= (S16)mRing.size() && !expandRing()) { - LL_WARNS("PacketRing") << "buffer full at " << mNumBufferedPackets << " packets, leaving one out" << LL_ENDL; + // A flood that fills the ring leaves a packet out with every one + // that comes in, so it is told of a second at a time. + ++mLeftOutSinceWarning; + const F64 now = LLFrameTimer::getTotalSeconds(); + if (!mWarnedLeftOut || now - mLastLeftOutWarning >= 1.0) + { + LL_WARNS("PacketRing") << "buffer full at " << mNumBufferedPackets << " packets, " << mLeftOutSinceWarning + << " left out" << LL_ENDL; + mWarnedLeftOut = true; + mLastLeftOutWarning = now; + mLeftOutSinceWarning = 0; + } return false; } const S16 ring_size = (S16)mRing.size(); diff --git a/indra/llmessage/llpacketring.h b/indra/llmessage/llpacketring.h index 0081a3cf17e..934abd0adce 100644 --- a/indra/llmessage/llpacketring.h +++ b/indra/llmessage/llpacketring.h @@ -73,4 +73,8 @@ class LLPacketRing S16 mHeadIndex { 0 }; S16 mNumBufferedPackets { 0 }; S32 mNumBufferedBytes { 0 }; + + S32 mLeftOutSinceWarning { 0 }; + F64 mLastLeftOutWarning { 0.0 }; + bool mWarnedLeftOut { false }; }; diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp index 0d73ee571a3..68ddb339fc9 100644 --- a/indra/llmessage/message.cpp +++ b/indra/llmessage/message.cpp @@ -1159,36 +1159,6 @@ S32 LLMessageSystem::bufferInboundPacket() } } - // A queue full at its ceiling has no room for the packet, which is - // left out as the network might have dropped it: neither acked nor - // its number recorded, so that its sender sends it again. - const bool has_room = (isHighPriorityMessage(pkt) ? mHighPriorityInbound : mLowPriorityInbound).hasRoom(); - - if (cdp && has_room) - { - // ACK inbound reliable packet ASAP, so its sender stops resending - // it while it waits in the queue, whatever checkMessages() makes - // of it, which does not ack it again. - if ((data[0] & LL_RELIABLE_FLAG)) - { - cdp->collectRAck(recv_packet_id); - pkt.setAcked(true); - } - - // Check packet sequencing here, in true socket-arrival order, before - // this packet is sorted into the high/low priority inbound queue. - // Skip duplicates of reliable packets already handled, marked resent - // or not, same as checkMessages()/logValidMsg() would do further - // downstream. - bool recv_resent = (data[0] & LL_RESENT_FLAG) != 0; - bool recv_reliable = (data[0] & LL_RELIABLE_FLAG) != 0; - if (!recv_reliable || !cdp->checkDuplicate(recv_packet_id, recv_resent)) - { - cdp->checkPacketInID(recv_packet_id, recv_resent); - pkt.setPacketIDChecked(true); - } - } - auto enqueue = [this](const LLPacketBuffer& buffered) { if (isHighPriorityMessage(buffered)) @@ -1201,13 +1171,54 @@ S32 LLMessageSystem::bufferInboundPacket() } }; + auto take = [&](LLPacketBuffer& arrived) + { + // A queue full at its ceiling has no room for the packet, which is + // left out as the network might have dropped it: neither acked nor + // its number recorded, so that its sender sends it again. + const bool has_room = (isHighPriorityMessage(arrived) ? mHighPriorityInbound : mLowPriorityInbound).hasRoom(); + + if (cdp) + { + // Skip duplicates of reliable packets already handled, marked resent + // or not, same as checkMessages()/logValidMsg() would do further + // downstream. + const bool recv_resent = (data[0] & LL_RESENT_FLAG) != 0; + const bool recv_reliable = (data[0] & LL_RELIABLE_FLAG) != 0; + const bool duplicate = recv_reliable && cdp->checkDuplicate(recv_packet_id, recv_resent); + + // ACK inbound reliable packet ASAP, so its sender stops resending + // it while it waits in the queue, whatever checkMessages() makes + // of it, which does not ack it again. A copy of a packet already + // taken is acked with no room for it, as it would be with room. + if (recv_reliable && (has_room || duplicate)) + { + cdp->collectRAck(recv_packet_id); + arrived.setAcked(true); + } + + // Check packet sequencing here, in true socket-arrival order, before + // this packet is sorted into the high/low priority inbound queue. + if (has_room && !duplicate) + { + cdp->checkPacketInID(recv_packet_id, recv_resent); + arrived.setPacketIDChecked(true); + } + } + + enqueue(arrived); + }; + #if AL_NET_IMPAIRMENT std::vector released; mNetImpairment.advance(released); + // A packet held back arrives when it is released: checkMessages() acks + // it and checks its number as it reads it, and a queue with no room for + // it then leaves it out unacked. if (!mNetImpairment.hold(pkt)) { - enqueue(pkt); + take(pkt); } // Anything released rejoins *behind* the packet that just arrived, which is what puts it @@ -1217,7 +1228,7 @@ S32 LLMessageSystem::bufferInboundPacket() enqueue(late); } #else - enqueue(pkt); + take(pkt); #endif } diff --git a/indra/llmessage/tests/message_test.cpp b/indra/llmessage/tests/message_test.cpp index dcbc32487eb..3d2ceee1ab7 100644 --- a/indra/llmessage/tests/message_test.cpp +++ b/indra/llmessage/tests/message_test.cpp @@ -36,6 +36,7 @@ #include "net.h" #include +#include #if !LL_WINDOWS #include @@ -67,6 +68,12 @@ namespace ll_test { return std::count(circuit.mAcks.begin(), circuit.mAcks.end(), id); } + + // The number the circuit takes the next packet in order to have. + static TPACKETID packetsInID(const LLCircuitData& circuit) + { + return circuit.mPacketsInID; + } }; } @@ -774,51 +781,91 @@ namespace REQUIRE(circuit != nullptr); const LLHost receiver("127.0.0.1", system.getListenPort()); - // DuplicateTest numbered `id`, a low priority message. - auto send = [&](U32 id, U8 flags) + // A packet numbered `id` carrying message number `message`. + using MessageNumber = std::array; + auto send = [&](U32 id, U8 flags, const MessageNumber& message) { - const U8 packet[] = { flags, (U8)(id >> 24), (U8)(id >> 16), (U8)(id >> 8), (U8)id, 0, 0xff, 0xff, 0x00, 0x01, 1, 2, 3, 4 }; + const U8 packet[] = { flags, (U8)(id >> 24), (U8)(id >> 16), (U8)(id >> 8), (U8)id, 0, + message[0], message[1], message[2], message[3], 1, 2, 3, 4 }; REQUIRE(send_packet(sender.mSocket, (const char*)packet, (int)sizeof(packet), receiver.getAddress(), receiver.getPort())); }; - // The ring's ceiling, filled a few at a time so that the socket's - // buffer holds every one until it is queued. + // Waits until the system has left `packets` packets out in all. + auto drain_until_left_out = [&](S32 packets) + { + const F64 give_up = LLTimer::getTotalSeconds() + 1.0; + while (system.getTotalNumDroppedPackets() < packets && LLTimer::getTotalSeconds() < give_up) + { + system.drainUdpSocket(); + ms_sleep(1); + } + REQUIRE(system.getTotalNumDroppedPackets() == packets); + }; + + // Fills the ring `message` goes to up to its ceiling with reliable + // packets numbered from `first`, a few at a time so that the socket's + // buffer holds every one until it is queued, and sends one more, + // which is left out. Returns its number. constexpr S32 FULL = 8192; - U32 id = 0; - while (system.getNumBufferedPackets() < FULL) + auto overflow = [&](U32 first, const MessageNumber& message) -> U32 { - const S32 batch = llmin(128, FULL - system.getNumBufferedPackets()); - for (S32 i = 0; i < batch; ++i) + const size_t acks_before = ll_test::CircuitProbe::acksWaiting(*circuit); + U32 id = first; + while (system.getNumBufferedPackets() < FULL) { - send(++id, LL_RELIABLE_FLAG); + const S32 batch = llmin(128, FULL - system.getNumBufferedPackets()); + for (S32 i = 0; i < batch; ++i) + { + send(id++, LL_RELIABLE_FLAG, message); + } + queue_until(system, (S32)(id - first)); + REQUIRE(system.getNumBufferedPackets() == (S32)(id - first)); } - queue_until(system, (S32)id); - REQUIRE(system.getNumBufferedPackets() == (S32)id); - } - REQUIRE(ll_test::CircuitProbe::acksWaiting(*circuit) == FULL); + REQUIRE(ll_test::CircuitProbe::acksWaiting(*circuit) == acks_before + FULL); + + const U32 left_out = id; + send(left_out, LL_RELIABLE_FLAG, message); + drain_until_left_out(1); + CHECK(system.getNumBufferedPackets() == FULL); + CHECK(ll_test::CircuitProbe::acksWaitingFor(*circuit, left_out) == 0); + CHECK(ll_test::CircuitProbe::acksWaiting(*circuit) == acks_before + FULL); + UNSCOPED_INFO("nor is its number taken as come"); + CHECK(ll_test::CircuitProbe::packetsInID(*circuit) == left_out); + return left_out; + }; - const U32 left_out = id + 1; - send(left_out, LL_RELIABLE_FLAG); - const F64 give_up = LLTimer::getTotalSeconds() + 1.0; - while (system.getTotalNumDroppedPackets() < 1 && LLTimer::getTotalSeconds() < give_up) + SECTION("the low priority ring") { - system.drainUdpSocket(); - ms_sleep(1); - } - REQUIRE(system.getTotalNumDroppedPackets() == 1); - CHECK(system.getNumBufferedPackets() == FULL); - CHECK(ll_test::CircuitProbe::acksWaitingFor(*circuit, left_out) == 0); - CHECK(ll_test::CircuitProbe::acksWaiting(*circuit) == FULL); + // DuplicateTest, Low 1. + const MessageNumber duplicate_test = { 0xff, 0xff, 0x00, 0x01 }; + send(1, LL_RELIABLE_FLAG, duplicate_test); + read_until(system, 1); + REQUIRE(sDuplicateTestsHandled == 1); + const U32 left_out = overflow(2, duplicate_test); + + UNSCOPED_INFO("a copy of a packet already read is acked all the same, as one it would drop"); + send(1, LL_RELIABLE_FLAG | LL_RESENT_FLAG, duplicate_test); + drain_until_left_out(2); + CHECK(ll_test::CircuitProbe::acksWaitingFor(*circuit, 1) == 2); + + UNSCOPED_INFO("every packet queued is read, the first among them"); + read_queued(system); + CHECK(sDuplicateTestsHandled == FULL + 1); - UNSCOPED_INFO("every packet queued is read, the first among them"); - read_queued(system); - CHECK(sDuplicateTestsHandled == FULL); + UNSCOPED_INFO("and the one left out is taken when it comes again"); + send(left_out, LL_RELIABLE_FLAG | LL_RESENT_FLAG, duplicate_test); + read_until(system, FULL + 2); + CHECK(sDuplicateTestsHandled == FULL + 2); + CHECK(ll_test::CircuitProbe::acksWaitingFor(*circuit, left_out) == 1); + } - UNSCOPED_INFO("and the one left out is taken when it comes again"); - send(left_out, LL_RELIABLE_FLAG | LL_RESENT_FLAG); - read_until(system, FULL + 1); - CHECK(sDuplicateTestsHandled == FULL + 1); - CHECK(ll_test::CircuitProbe::acksWaitingFor(*circuit, left_out) == 1); + SECTION("the high priority ring") + { + // A Medium message, whose bytes after the number are none that + // isHighPriorityMessage keeps in the low priority ring. + const MessageNumber medium = { 0xff, 0x01, 0x01, 0x01 }; + overflow(1, medium); + } } } From 79057e095877688e26591602e42ec5d68e7a9837 Mon Sep 17 00:00:00 2001 From: Rye Date: Tue, 3 Nov 2026 04:27:59 -0500 Subject: [PATCH 4/4] A region waiting for its object cache's removal takes nothing more into it The removal of a region's object cache that an update asks for is carried out at the next update of the regions, and what the region took into its cache in between went with the rest, neither made nor asked for again. A probe found the object cached and asked for nothing, and an object sent whole was kept in the cache. Removing at once, as before, left a later probe to miss and a whole object to go into a fresh cache. Until the removal, a probe now misses and an object sent whole is asked for again, as they would be with the cache removed. The releaseAll test now sees each entry leave its octree entry, and the removal test the request's state. The region's probe and full update have no test, as newview_tests does not build the region. Co-Authored-By: Claude Opus 5.5 --- indra/newview/llviewerregion.cpp | 22 +++++++++++++++++++++ indra/newview/llviewerregion.h | 3 +++ indra/newview/llvocache.cpp | 7 ++++++- indra/newview/llvocache.h | 1 + indra/newview/tests/llvieweroctree_stub.cpp | 4 +++- indra/newview/tests/llvocache_test.cpp | 8 ++++++++ 6 files changed, 43 insertions(+), 2 deletions(-) diff --git a/indra/newview/llviewerregion.cpp b/indra/newview/llviewerregion.cpp index 128dc06cc0b..b9640822455 100644 --- a/indra/newview/llviewerregion.cpp +++ b/indra/newview/llviewerregion.cpp @@ -2962,6 +2962,14 @@ LLViewerRegion::eCacheUpdateResult LLViewerRegion::cacheFullUpdate(LLDataPackerB LLViewerObject::unpackU32(&dp, local_id, "LocalID"); LLViewerObject::unpackU32(&dp, crc, "CRC"); + // Nor is one sent whole kept in it, to be let go before it is made: it is + // asked for again, to come once the cache has been removed. + if (cacheRemovalRequested()) + { + addCacheMissFull(local_id); + return CACHE_UPDATE_DUPE; + } + LLVOCacheEntry* entry = getCacheEntry(local_id, false); if (entry) @@ -3100,10 +3108,24 @@ void LLViewerRegion::addToCreatedList(U32 local_id) // Get data packer for this object, if we have cached data // AND the CRC matches. JC +bool LLViewerRegion::cacheRemovalRequested() const +{ + return LLVOCache::instanceExists() && LLVOCache::getInstance()->isRemovalRequested(mHandle); +} + bool LLViewerRegion::probeCache(U32 local_id, U32 crc, U32 flags, U8 &cache_miss_type) { //llassert(mCacheLoaded); This assert failes often, changing to early-out -- davep, 2010/10/18 + // An object a cache about to be removed holds would never be made from it, + // so it is asked for as the removed cache would have it asked for. + if (cacheRemovalRequested()) + { + addCacheMiss(local_id, CACHE_MISS_TYPE_TOTAL); + cache_miss_type = CACHE_MISS_TYPE_TOTAL; + return false; + } + LLVOCacheEntry* entry = getCacheEntry(local_id, false); if (entry) diff --git a/indra/newview/llviewerregion.h b/indra/newview/llviewerregion.h index a2d07d00704..28b541d03e4 100644 --- a/indra/newview/llviewerregion.h +++ b/indra/newview/llviewerregion.h @@ -478,6 +478,9 @@ class LLViewerRegion: public LLCapabilityProvider // implements this interface void addCacheMiss(U32 id, LLViewerRegion::eCacheMissType miss_type); void decodeBoundingInfo(LLVOCacheEntry* entry); bool isNonCacheableObjectCreated(U32 local_id); + // Whether the region's object cache is to be removed, letting go of its + // entries, before anything made from them would be. + bool cacheRemovalRequested() const; public: void applyCacheMiscExtras(LLViewerObject* obj); diff --git a/indra/newview/llvocache.cpp b/indra/newview/llvocache.cpp index 179a712da3a..85830e585f5 100644 --- a/indra/newview/llvocache.cpp +++ b/indra/newview/llvocache.cpp @@ -1454,12 +1454,17 @@ void LLVOCache::removeEntry(U64 handle) void LLVOCache::requestRemoval(U64 handle) { - if (std::find(mRequestedRemovals.begin(), mRequestedRemovals.end(), handle) == mRequestedRemovals.end()) + if (!isRemovalRequested(handle)) { mRequestedRemovals.push_back(handle); } } +bool LLVOCache::isRemovalRequested(U64 handle) const +{ + return std::find(mRequestedRemovals.begin(), mRequestedRemovals.end(), handle) != mRequestedRemovals.end(); +} + void LLVOCache::removeRequestedEntries() { std::vector handles; diff --git a/indra/newview/llvocache.h b/indra/newview/llvocache.h index 12700cc2271..38427e58a61 100644 --- a/indra/newview/llvocache.h +++ b/indra/newview/llvocache.h @@ -360,6 +360,7 @@ class LLVOCache : public LLParamSingleton // Removes a region's entry as removeEntry does, once removeRequestedEntries runs: the removal lets go of the // region's objects in memory, one of which an object update asking for it is read from. void requestRemoval(U64 handle); + bool isRemovalRequested(U64 handle) const; void removeRequestedEntries(); void removeGenericExtrasForHandle(U64 handle); diff --git a/indra/newview/tests/llvieweroctree_stub.cpp b/indra/newview/tests/llvieweroctree_stub.cpp index b2d1de60983..36193a5d359 100644 --- a/indra/newview/tests/llvieweroctree_stub.cpp +++ b/indra/newview/tests/llvieweroctree_stub.cpp @@ -75,7 +75,9 @@ void LLViewerOctreeEntryData::setSpatialExtents(const LLVector4a& min, const LLV void LLViewerOctreeEntryData::setPositionGroup(const LLVector4a& pos) {} void LLViewerOctreeEntryData::setGroup(LLViewerOctreeGroup* group) {} void LLViewerOctreeEntryData::setOctreeEntry(LLViewerOctreeEntry* entry) {} -void LLViewerOctreeEntryData::removeOctreeEntry() {} +// The entries each removeOctreeEntry was called on, in order. +std::vector sRemovedOctreeEntries; +void LLViewerOctreeEntryData::removeOctreeEntry() { sRemovedOctreeEntries.push_back(this); } U32 LLViewerOctreeEntryData::sCurVisible{}; LLViewerOctreePartition::LLViewerOctreePartition() = default; diff --git a/indra/newview/tests/llvocache_test.cpp b/indra/newview/tests/llvocache_test.cpp index e4fb3dc2388..c6ffff6b38e 100644 --- a/indra/newview/tests/llvocache_test.cpp +++ b/indra/newview/tests/llvocache_test.cpp @@ -817,14 +817,17 @@ namespace LLVOCache::instance().writeToCache(handle, id, makeEntries(3), true, false); REQUIRE(LLVOCache::instance().getCacheEntries() == 1u); + CHECK_FALSE(LLVOCache::instance().isRemovalRequested(handle)); LLVOCache::instance().requestRemoval(handle); LLVOCache::instance().requestRemoval(handle); UNSCOPED_INFO("the region is still cached once its removal is asked for"); + CHECK(LLVOCache::instance().isRemovalRequested(handle)); CHECK(LLVOCache::instance().getCacheEntries() == 1u); CHECK(std::filesystem::exists(objectFile(handle, ".slc"))); LLVOCache::instance().removeRequestedEntries(); UNSCOPED_INFO("and gone once the removals are carried out"); + CHECK_FALSE(LLVOCache::instance().isRemovalRequested(handle)); CHECK(LLVOCache::instance().getCacheEntries() == 0u); CHECK_FALSE(std::filesystem::exists(objectFile(handle, ".slc"))); @@ -846,6 +849,7 @@ namespace std::vector left_tree; std::vector> keep; + sRemovedOctreeEntries.clear(); LLVOCacheEntry::releaseAll(entries, { &active, &visible }, [&](LLVOCacheEntry* entry) { left_tree.push_back(entry); }, keep); CHECK(entries.empty()); @@ -854,6 +858,10 @@ namespace UNSCOPED_INFO("every entry left the tree"); CHECK(left_tree.size() == 6u); CHECK(std::find(left_tree.begin(), left_tree.end(), watched.get()) != left_tree.end()); + UNSCOPED_INFO("and its octree entry"); + CHECK(sRemovedOctreeEntries.size() == 6u); + const LLViewerOctreeEntryData* watched_data = watched.get(); + CHECK(std::find(sRemovedOctreeEntries.begin(), sRemovedOctreeEntries.end(), watched_data) != sRemovedOctreeEntries.end()); UNSCOPED_INFO("and is held by keep alone besides the test"); CHECK(keep.size() == 6u); CHECK(watched->getNumRefs() == 2);