From 91b6159244fd74adee73aae511aa8f42f916b290 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Thu, 8 Oct 2026 17:54:22 +0200 Subject: [PATCH 1/8] refactor(ds): delete the dead Tree, WeightedGraph and OrderedChannelHeap headers First increment of the stage 1 container migration. These three have no consumers anywhere in Source/, Samples/, Tests/ or DependentExtensions/: - ds_ordered_channel_heap.h: OrderedChannelHeap is named by nothing but its own header. Not listed in the issue; found while mapping Heap's consumers. - ds_weighted_graph.h: only connection_graph2.h included it, and never names WeightedGraph. - ds_tree.h: only ds_weighted_graph.h used DataStructures::Tree. Removing them also removes two of the four DataStructures::Heap uses in the tree, leaving only the two real ones in reliability_layer.h. connection_graph2.h gains a direct include of ds_ordered_list.h. It names DataStructures::OrderedList in three declarations but never included it: the type was reaching it transitively through ds_weighted_graph.h, so deleting that header broke the build. The issue's note that connection_graph2.h "includes but does not use" WeightedGraph was right about the type and wrong about the dependency. Also drops the three entries from the header list in Source/CMakeLists.txt. Verified on macOS arm64: full ctest 285/285. --- Source/CMakeLists.txt | 3 - Source/include/mafianet/connection_graph2.h | 2 +- .../mafianet/ds_ordered_channel_heap.h | 263 --------- Source/include/mafianet/ds_tree.h | 111 ---- Source/include/mafianet/ds_weighted_graph.h | 546 ------------------ 5 files changed, 1 insertion(+), 924 deletions(-) delete mode 100644 Source/include/mafianet/ds_ordered_channel_heap.h delete mode 100644 Source/include/mafianet/ds_tree.h delete mode 100644 Source/include/mafianet/ds_weighted_graph.h diff --git a/Source/CMakeLists.txt b/Source/CMakeLists.txt index 5ddd97d52..9cd9157a5 100644 --- a/Source/CMakeLists.txt +++ b/Source/CMakeLists.txt @@ -169,15 +169,12 @@ set(MAFIANET_HEADERS include/mafianet/ds_map.h include/mafianet/ds_memory_pool.h include/mafianet/ds_multilist.h - include/mafianet/ds_ordered_channel_heap.h include/mafianet/ds_ordered_list.h include/mafianet/ds_queue.h include/mafianet/ds_queue_linked_list.h include/mafianet/ds_range_list.h include/mafianet/ds_table.h include/mafianet/ds_threadsafe_allocating_queue.h - include/mafianet/ds_tree.h - include/mafianet/ds_weighted_graph.h include/mafianet/dyn_dns.h include/mafianet/email_sender.h include/mafianet/empty_header.h diff --git a/Source/include/mafianet/connection_graph2.h b/Source/include/mafianet/connection_graph2.h index 65b6f0ef4..b17d64243 100644 --- a/Source/include/mafianet/connection_graph2.h +++ b/Source/include/mafianet/connection_graph2.h @@ -28,7 +28,7 @@ #include "mafianet/types.h" #include "mafianet/plugin_interface2.h" #include "mafianet/ds_list.h" -#include "mafianet/ds_weighted_graph.h" +#include "mafianet/ds_ordered_list.h" #include "mafianet/get_time.h" #include "mafianet/export.h" diff --git a/Source/include/mafianet/ds_ordered_channel_heap.h b/Source/include/mafianet/ds_ordered_channel_heap.h deleted file mode 100644 index e745214fe..000000000 --- a/Source/include/mafianet/ds_ordered_channel_heap.h +++ /dev/null @@ -1,263 +0,0 @@ -/* - * Original work: Copyright (c) 2014, Oculus VR, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * RakNet License.txt file in the licenses directory of this source tree. An additional grant - * of patent rights can be found in the RakNet Patents.txt file in the same directory. - * - * - * Modified work: Copyright (c) 2017, SLikeSoft UG (haftungsbeschränkt) - * - * This source code was modified by SLikeSoft. Modifications are licensed under the MIT-style - * license found in the license.txt file in the root directory of this source tree. - */ - -/// \file ds_ordered_channel_heap.h -/// \internal -/// \brief Ordered Channel Heap . This is a heap where you add to it on multiple ordered channels, with each channel having a different weight. -/// - - -// Deprecated: ds_ordered_channel_heap.h has no consumer in the library and is scheduled for removal in the next -// minor release. Copy it into your project if you depend on it. -// Define MAFIANET_SILENCE_DEPRECATED_INCLUDES to suppress the message. -#ifndef MAFIANET_SILENCE_DEPRECATED_INCLUDES -#pragma message("mafianet/ds_ordered_channel_heap.h is deprecated and will be removed; copy it into your project if you use it") -#endif - -#ifndef __RAKNET_ORDERED_CHANNEL_HEAP_H -#define __RAKNET_ORDERED_CHANNEL_HEAP_H - -#include "mafianet/ds_heap.h" -#include "mafianet/ds_map.h" -#include "mafianet/ds_queue.h" -#include "mafianet/export.h" -#include "mafianet/assert.h" -#include "mafianet/rand.h" - -/// The namespace DataStructures was only added to avoid compiler errors for commonly named data structures -/// As these data structures are stand-alone, you can use them outside of RakNet for your own projects if you wish. -namespace DataStructures -{ - template > - class RAK_DLL_EXPORT OrderedChannelHeap - { - public: - static void IMPLEMENT_DEFAULT_COMPARISON(void) {DataStructures::defaultMapKeyComparison(channel_key_type(),channel_key_type());} - - OrderedChannelHeap(); - ~OrderedChannelHeap(); - void Push(const channel_key_type &channelID, const heap_data_type &data); - void PushAtHead(const unsigned index, const channel_key_type &channelID, const heap_data_type &data); - heap_data_type Pop(const unsigned startingIndex=0); - heap_data_type Peek(const unsigned startingIndex) const; - void AddChannel(const channel_key_type &channelID, const double weight); - void RemoveChannel(channel_key_type channelID); - void Clear(void); - heap_data_type& operator[] ( const unsigned int position ) const; - unsigned ChannelSize(const channel_key_type &channelID); - unsigned Size(void) const; - - struct QueueAndWeight - { - DataStructures::Queue randResultQueue; - double weight; - bool signalDeletion; - }; - - struct HeapChannelAndData - { - HeapChannelAndData() {} - HeapChannelAndData(const channel_key_type &_channel, const heap_data_type &_data) : data(_data), channel(_channel) {} - heap_data_type data; - channel_key_type channel; - }; - - protected: - DataStructures::Map map; - DataStructures::Heap heap; - void GreatestRandResult(void); - }; - - template - OrderedChannelHeap::OrderedChannelHeap() - { - } - - template - OrderedChannelHeap::~OrderedChannelHeap() - { - Clear(); - } - - template - void OrderedChannelHeap::Push(const channel_key_type &channelID, const heap_data_type &data) - { - PushAtHead(MAX_UNSIGNED_LONG, channelID, data); - } - - template - void OrderedChannelHeap::GreatestRandResult(void) - { - double greatest; - unsigned i; - greatest=0.0; - for (i=0; i < map.Size(); i++) - { - if (map[i]->randResultQueue.Size() && map[i]->randResultQueue[0]>greatest) - greatest=map[i]->randResultQueue[0]; - } - return greatest; - } - - template - void OrderedChannelHeap::PushAtHead(const unsigned index, const channel_key_type &channelID, const heap_data_type &data) - { - // If an assert hits here then this is an unknown channel. Call AddChannel first. - QueueAndWeight *queueAndWeight=map.Get(channelID); - double maxRange, minRange, rnd; - if (queueAndWeight->randResultQueue.Size()==0) - { - // Set maxRange to the greatest random number waiting to be returned, rather than 1.0 necessarily - // This is so weights are scaled similarly among channels. For example, if the head weight for a used channel was .25 - // and then we added another channel, the new channel would need to choose between .25 and 0 - // If we chose between 1.0 and 0, it would be 1/.25 (4x) more likely to be at the head of the heap than it should be - maxRange=GreatestRandResult(); - if (maxRange==0.0) - maxRange=1.0; - minRange=0.0; - } - else if (index >= queueAndWeight->randResultQueue.Size()) - { - maxRange=queueAndWeight->randResultQueue[queueAndWeight->randResultQueue.Size()-1]*.99999999; - minRange=0.0; - } - else - { - if (index==0) - { - maxRange=GreatestRandResult(); - if (maxRange==queueAndWeight->randResultQueue[0]) - maxRange=1.0; - } - else if (index >= queueAndWeight->randResultQueue.Size()) - maxRange=queueAndWeight->randResultQueue[queueAndWeight->randResultQueue.Size()-1]*.99999999; - else - maxRange=queueAndWeight->randResultQueue[index-1]*.99999999; - - minRange=maxRange=queueAndWeight->randResultQueue[index]*1.00000001; - } - -#ifdef _DEBUG - RakAssert(maxRange!=0.0); -#endif - rnd=frandomMT() * (maxRange - minRange); - if (rnd==0.0) - rnd=maxRange/2.0; - - if (index >= queueAndWeight->randResultQueue.Size()) - queueAndWeight->randResultQueue.Push(rnd); - else - queueAndWeight->randResultQueue.PushAtHead(rnd, index); - - heap.Push(rnd*queueAndWeight->weight, HeapChannelAndData(channelID, data)); - } - - template - heap_data_type OrderedChannelHeap::Pop(const unsigned startingIndex) - { - RakAssert(startingIndex < heap.Size()); - - QueueAndWeight *queueAndWeight=map.Get(heap[startingIndex].channel); - if (startingIndex!=0) - { - // Ugly - have to count in the heap how many nodes have the same channel, so we know where to delete from in the queue - unsigned indiceCount=0; - unsigned i; - for (i=0; i < startingIndex; i++) - if (channel_key_comparison_func(heap[i].channel,heap[startingIndex].channel)==0) - indiceCount++; - queueAndWeight->randResultQueue.RemoveAtIndex(indiceCount); - } - else - { - // TODO - ordered channel heap uses progressively lower values as items are inserted. But this won't give relative ordering among channels. I have to renormalize after every pop. - queueAndWeight->randResultQueue.Pop(); - } - - // Try to remove the channel after every pop, because doing so is not valid while there are elements in the list. - if (queueAndWeight->signalDeletion) - RemoveChannel(heap[startingIndex].channel); - - return heap.Pop(startingIndex).data; - } - - template - heap_data_type OrderedChannelHeap::Peek(const unsigned startingIndex) const - { - HeapChannelAndData heapChannelAndData = heap.Peek(startingIndex); - return heapChannelAndData.data; - } - - template - void OrderedChannelHeap::AddChannel(const channel_key_type &channelID, const double weight) - { - QueueAndWeight *qaw = MafiaNet::OP_NEW( _FILE_AND_LINE_ ); - qaw->weight=weight; - qaw->signalDeletion=false; - map.SetNew(channelID, qaw); - } - - template - void OrderedChannelHeap::RemoveChannel(channel_key_type channelID) - { - if (map.Has(channelID)) - { - unsigned i; - i=map.GetIndexAtKey(channelID); - if (map[i]->randResultQueue.Size()==0) - { - MafiaNet::OP_DELETE(map[i], _FILE_AND_LINE_); - map.RemoveAtIndex(i); - } - else - { - // Signal this channel for deletion later, because the heap has nodes with this channel right now - map[i]->signalDeletion=true; - } - } - } - - template - unsigned OrderedChannelHeap::Size(void) const - { - return heap.Size(); - } - - template - heap_data_type& OrderedChannelHeap::operator[]( const unsigned int position ) const - { - return heap[position].data; - } - - - template - unsigned OrderedChannelHeap::ChannelSize(const channel_key_type &channelID) - { - QueueAndWeight *queueAndWeight=map.Get(channelID); - return queueAndWeight->randResultQueue.Size(); - } - - template - void OrderedChannelHeap::Clear(void) - { - unsigned i; - for (i=0; i < map.Size(); i++) - MafiaNet::OP_DELETE(map[i], _FILE_AND_LINE_); - map.Clear(_FILE_AND_LINE_); - heap.Clear(_FILE_AND_LINE_); - } -} - -#endif diff --git a/Source/include/mafianet/ds_tree.h b/Source/include/mafianet/ds_tree.h deleted file mode 100644 index 03f88828a..000000000 --- a/Source/include/mafianet/ds_tree.h +++ /dev/null @@ -1,111 +0,0 @@ -/* - * Original work: Copyright (c) 2014, Oculus VR, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * RakNet License.txt file in the licenses directory of this source tree. An additional grant - * of patent rights can be found in the RakNet Patents.txt file in the same directory. - * - * - * Modified work: Copyright (c) 2017, SLikeSoft UG (haftungsbeschränkt) - * - * This source code was modified by SLikeSoft. Modifications are licensed under the MIT-style - * license found in the license.txt file in the root directory of this source tree. - */ - -/// \file ds_tree.h -/// \internal -/// \brief Just a regular tree -/// - - - -#ifndef __DS_TREE_H -#define __DS_TREE_H - -#include "mafianet/export.h" -#include "mafianet/ds_list.h" -#include "mafianet/ds_queue.h" -#include "mafianet/memory_override.h" - -/// The namespace DataStructures was only added to avoid compiler errors for commonly named data structures -/// As these data structures are stand-alone, you can use them outside of RakNet for your own projects if you wish. -namespace DataStructures -{ - template - class RAK_DLL_EXPORT Tree - { - public: - Tree(); - Tree(TreeType &inputData); - ~Tree(); - void LevelOrderTraversal(DataStructures::List &output); - void AddChild(TreeType &newData); - void DeleteDecendants(void); - - TreeType data; - DataStructures::List children; - }; - - template - Tree::Tree() - { - - } - - template - Tree::Tree(TreeType &inputData) - { - data=inputData; - } - - template - Tree::~Tree() - { - DeleteDecendants(); - } - - template - void Tree::LevelOrderTraversal(DataStructures::List &output) - { - unsigned i; - Tree *node; - DataStructures::Queue*> queue; - - for (i=0; i < children.Size(); i++) - queue.Push(children[i]); - - while (queue.Size()) - { - node=queue.Pop(); - output.Insert(node, _FILE_AND_LINE_); - for (i=0; i < node->children.Size(); i++) - queue.Push(node->children[i]); - } - } - - template - void Tree::AddChild(TreeType &newData) - { - children.Insert(MafiaNet::OP_NEW(newData, _FILE_AND_LINE_)); - } - - template - void Tree::DeleteDecendants(void) - { - /* - DataStructures::List output; - LevelOrderTraversal(output); - unsigned i; - for (i=0; i < output.Size(); i++) - MafiaNet::OP_DELETE(output[i], _FILE_AND_LINE_); -*/ - - // Already recursive to do this - unsigned int i; - for (i=0; i < children.Size(); i++) - MafiaNet::OP_DELETE(children[i], _FILE_AND_LINE_); - } -} - -#endif diff --git a/Source/include/mafianet/ds_weighted_graph.h b/Source/include/mafianet/ds_weighted_graph.h deleted file mode 100644 index 2ead6ff4e..000000000 --- a/Source/include/mafianet/ds_weighted_graph.h +++ /dev/null @@ -1,546 +0,0 @@ -/* - * Original work: Copyright (c) 2014, Oculus VR, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * RakNet License.txt file in the licenses directory of this source tree. An additional grant - * of patent rights can be found in the RakNet Patents.txt file in the same directory. - * - * - * Modified work: Copyright (c) 2016-2017, SLikeSoft UG (haftungsbeschränkt) - * - * This source code was modified by SLikeSoft. Modifications are licensed under the MIT-style - * license found in the license.txt file in the root directory of this source tree. - */ - -/// \file ds_weighted_graph.h -/// \internal -/// \brief Weighted graph. -/// \details I'm assuming the indices are complex map types, rather than sequential numbers (which could be implemented much more efficiently). -/// - - -#ifndef __WEIGHTED_GRAPH_H -#define __WEIGHTED_GRAPH_H - -#include "mafianet/ds_ordered_list.h" -#include "mafianet/ds_map.h" -#include "mafianet/ds_heap.h" -#include "mafianet/ds_queue.h" -#include "mafianet/ds_tree.h" -#include "mafianet/assert.h" -#include "mafianet/memory_override.h" -#ifdef _DEBUG -#include -#endif - -#ifdef _MSC_VER -#pragma warning( push ) -#endif - -/// The namespace DataStructures was only added to avoid compiler errors for commonly named data structures -/// As these data structures are stand-alone, you can use them outside of RakNet for your own projects if you wish. -namespace DataStructures -{ - template - class RAK_DLL_EXPORT WeightedGraph - { - public: - static void IMPLEMENT_DEFAULT_COMPARISON(void) {DataStructures::defaultMapKeyComparison(node_type(),node_type());} - - WeightedGraph(); - ~WeightedGraph(); - WeightedGraph( const WeightedGraph& original_copy ); - WeightedGraph& operator= ( const WeightedGraph& original_copy ); - void AddNode(const node_type &node); - void RemoveNode(const node_type &node); - void AddConnection(const node_type &node1, const node_type &node2, weight_type weight); - void RemoveConnection(const node_type &node1, const node_type &node2); - bool HasConnection(const node_type &node1, const node_type &node2); - void Print(void); - void Clear(void); - bool GetShortestPath(DataStructures::List &path, node_type startNode, node_type endNode, weight_type INFINITE_WEIGHT); - bool GetSpanningTree(DataStructures::Tree &outTree, DataStructures::List *inputNodes, node_type startNode, weight_type INFINITE_WEIGHT ); - unsigned GetNodeCount(void) const; - unsigned GetConnectionCount(unsigned nodeIndex) const; - void GetConnectionAtIndex(unsigned nodeIndex, unsigned connectionIndex, node_type &outNode, weight_type &outWeight) const; - node_type GetNodeAtIndex(unsigned nodeIndex) const; - - protected: - void ClearDijkstra(void); - void GenerateDisjktraMatrix(node_type startNode, weight_type INFINITE_WEIGHT); - - DataStructures::Map *> adjacencyLists; - - // All these variables are for path finding with Dijkstra - // 08/23/06 Won't compile as a DLL inside this struct - // struct - // { - bool isValidPath; - node_type rootNode; - DataStructures::OrderedList costMatrixIndices; - weight_type *costMatrix; - node_type *leastNodeArray; - // } dijkstra; - - struct NodeAndParent - { - DataStructures::Tree*node; - DataStructures::Tree*parent; - }; - }; - - template - WeightedGraph::WeightedGraph() - { - isValidPath=false; - costMatrix=0; - } - - template - WeightedGraph::~WeightedGraph() - { - Clear(); - } - - template - WeightedGraph::WeightedGraph( const WeightedGraph& original_copy ) - { - adjacencyLists=original_copy.adjacencyLists; - - isValidPath=original_copy.isValidPath; - if (isValidPath) - { - rootNode=original_copy.rootNode; - costMatrixIndices=original_copy.costMatrixIndices; - costMatrix = MafiaNet::OP_NEW_ARRAY(costMatrixIndices.Size() * costMatrixIndices.Size(), _FILE_AND_LINE_ ); - leastNodeArray = MafiaNet::OP_NEW_ARRAY(costMatrixIndices.Size(), _FILE_AND_LINE_ ); - memcpy(costMatrix, original_copy.costMatrix, costMatrixIndices.Size() * costMatrixIndices.Size() * sizeof(weight_type)); - memcpy(leastNodeArray, original_copy.leastNodeArray, costMatrixIndices.Size() * sizeof(weight_type)); - } - } - - template - WeightedGraph& WeightedGraph::operator=( const WeightedGraph& original_copy ) - { - adjacencyLists=original_copy.adjacencyLists; - - isValidPath=original_copy.isValidPath; - if (isValidPath) - { - rootNode=original_copy.rootNode; - costMatrixIndices=original_copy.costMatrixIndices; - costMatrix = MafiaNet::OP_NEW_ARRAY(costMatrixIndices.Size() * costMatrixIndices.Size(), _FILE_AND_LINE_ ); - leastNodeArray = MafiaNet::OP_NEW_ARRAY(costMatrixIndices.Size(), _FILE_AND_LINE_ ); - memcpy(costMatrix, original_copy.costMatrix, costMatrixIndices.Size() * costMatrixIndices.Size() * sizeof(weight_type)); - memcpy(leastNodeArray, original_copy.leastNodeArray, costMatrixIndices.Size() * sizeof(weight_type)); - } - - return *this; - } - - template - void WeightedGraph::AddNode(const node_type &node) - { - adjacencyLists.SetNew(node, MafiaNet::OP_NEW >( _FILE_AND_LINE_) ); - } - - template - void WeightedGraph::RemoveNode(const node_type &node) - { - unsigned i; - DataStructures::Queue removeNodeQueue; - - removeNodeQueue.Push(node, _FILE_AND_LINE_ ); - while (removeNodeQueue.Size()) - { - MafiaNet::OP_DELETE(adjacencyLists.Pop(removeNodeQueue.Pop()), _FILE_AND_LINE_); - - // Remove this node from all of the other lists as well - for (i=0; i < adjacencyLists.Size(); i++) - { - adjacencyLists[i]->Delete(node); - -#ifdef _MSC_VER -#pragma warning( disable : 4127 ) // warning C4127: conditional expression is constant -#endif - if (allow_unlinkedNodes==false && adjacencyLists[i]->Size()==0) - removeNodeQueue.Push(adjacencyLists.GetKeyAtIndex(i), _FILE_AND_LINE_ ); - } - } - - ClearDijkstra(); - } - - template - bool WeightedGraph::HasConnection(const node_type &node1, const node_type &node2) - { - if (node1==node2) - return false; - if (adjacencyLists.Has(node1)==false) - return false; - return adjacencyLists.Get(node1)->Has(node2); - } - - template - void WeightedGraph::AddConnection(const node_type &node1, const node_type &node2, weight_type weight) - { - if (node1==node2) - return; - - if (adjacencyLists.Has(node1)==false) - AddNode(node1); - adjacencyLists.Get(node1)->Set(node2, weight); - if (adjacencyLists.Has(node2)==false) - AddNode(node2); - adjacencyLists.Get(node2)->Set(node1, weight); - } - - template - void WeightedGraph::RemoveConnection(const node_type &node1, const node_type &node2) - { - adjacencyLists.Get(node2)->Delete(node1); - adjacencyLists.Get(node1)->Delete(node2); - -#ifdef _MSC_VER -#pragma warning( disable : 4127 ) // warning C4127: conditional expression is constant -#endif - if (allow_unlinkedNodes==false) // If we do not allow _unlinked nodes, then if there are no connections, remove the node - { - if (adjacencyLists.Get(node1)->Size()==0) - RemoveNode(node1); // Will also remove node1 from the adjacency list of node2 - if (adjacencyLists.Has(node2) && adjacencyLists.Get(node2)->Size()==0) - RemoveNode(node2); - } - - ClearDijkstra(); - } - - template - void WeightedGraph::Clear(void) - { - unsigned i; - for (i=0; i < adjacencyLists.Size(); i++) - MafiaNet::OP_DELETE(adjacencyLists[i], _FILE_AND_LINE_); - adjacencyLists.Clear(); - - ClearDijkstra(); - } - - template - bool WeightedGraph::GetShortestPath(DataStructures::List &path, node_type startNode, node_type endNode, weight_type INFINITE_WEIGHT) - { - path.Clear(false, _FILE_AND_LINE_); - if (startNode==endNode) - { - path.Insert(startNode, _FILE_AND_LINE_); - path.Insert(endNode, _FILE_AND_LINE_); - return true; - } - - if (isValidPath==false || rootNode!=startNode) - { - ClearDijkstra(); - GenerateDisjktraMatrix(startNode, INFINITE_WEIGHT); - } - - // return the results - bool objectExists; - unsigned col,row; - weight_type currentWeight; - DataStructures::Queue outputQueue; - col=costMatrixIndices.GetIndexFromKey(endNode, &objectExists); - if (costMatrixIndices.Size()<2) - { - return false; - } - if (objectExists==false) - { - return false; - } - node_type vertex; - row=costMatrixIndices.Size()-2; - if (row==0) - { - path.Insert(startNode, _FILE_AND_LINE_); - path.Insert(endNode, _FILE_AND_LINE_); - return true; - } - currentWeight=costMatrix[row*adjacencyLists.Size() + col]; - if (currentWeight==INFINITE_WEIGHT) - { - // No path - return true; - } - vertex=endNode; - outputQueue.PushAtHead(vertex, 0, _FILE_AND_LINE_); - row--; - for(;;) - { - while (costMatrix[row*adjacencyLists.Size() + col] == currentWeight) - { - if (row==0) - { - path.Insert(startNode, _FILE_AND_LINE_); - while(!outputQueue.IsEmpty()) - path.Insert(outputQueue.Pop(), _FILE_AND_LINE_); - return true; - } - --row; - } - - vertex=leastNodeArray[row]; - outputQueue.PushAtHead(vertex, 0, _FILE_AND_LINE_); - if (row==0) - break; - col=costMatrixIndices.GetIndexFromKey(vertex, &objectExists); - currentWeight=costMatrix[row*adjacencyLists.Size() + col]; - } - - path.Insert(startNode, _FILE_AND_LINE_); - while(!outputQueue.IsEmpty()) - path.Insert(outputQueue.Pop(), _FILE_AND_LINE_); - return true; - } - - template - node_type WeightedGraph::GetNodeAtIndex(unsigned nodeIndex) const - { - return adjacencyLists.GetKeyAtIndex(nodeIndex); - } - - template - unsigned WeightedGraph::GetNodeCount(void) const - { - return adjacencyLists.Size(); - } - - template - unsigned WeightedGraph::GetConnectionCount(unsigned nodeIndex) const - { - return adjacencyLists[nodeIndex]->Size(); - } - - template - void WeightedGraph::GetConnectionAtIndex(unsigned nodeIndex, unsigned connectionIndex, node_type &outNode, weight_type &outWeight) const - { - outWeight=adjacencyLists[nodeIndex]->operator[](connectionIndex); - outNode=adjacencyLists[nodeIndex]->GetKeyAtIndex(connectionIndex); - } - - template - bool WeightedGraph::GetSpanningTree(DataStructures::Tree &outTree, DataStructures::List *inputNodes, node_type startNode, weight_type INFINITE_WEIGHT ) - { - // Find the shortest path from the start node to each of the input nodes. Add this path to a new WeightedGraph if the result is reachable - DataStructures::List path; - DataStructures::WeightedGraph outGraph; - bool res; - unsigned i,j; - for (i=0; i < inputNodes->Size(); i++) - { - res=GetShortestPath(path, startNode, (*inputNodes)[i], INFINITE_WEIGHT); - if (res && path.Size()>0) - { - for (j=0; j < path.Size()-1; j++) - { - // Don't bother looking up the weight - outGraph.AddConnection(path[j], path[j+1], INFINITE_WEIGHT); - } - } - } - - // Copy the graph to a tree. - DataStructures::Queue nodesToProcess; - DataStructures::Tree *current; - DataStructures::Map *adjacencyList; - node_type key; - NodeAndParent nap, nap2; - outTree.DeleteDecendants(); - outTree.data=startNode; - current=&outTree; - if (outGraph.adjacencyLists.Has(startNode)==false) - return false; - adjacencyList = outGraph.adjacencyLists.Get(startNode); - - for (i=0; i < adjacencyList->Size(); i++) - { - nap2.node= MafiaNet::OP_NEW >( _FILE_AND_LINE_ ); - nap2.node->data=adjacencyList->GetKeyAtIndex(i); - nap2.parent=current; - nodesToProcess.Push(nap2, _FILE_AND_LINE_ ); - current->children.Insert(nap2.node, _FILE_AND_LINE_); - } - - while (nodesToProcess.Size()) - { - nap=nodesToProcess.Pop(); - current=nap.node; - adjacencyList = outGraph.adjacencyLists.Get(nap.node->data); - - for (i=0; i < adjacencyList->Size(); i++) - { - key=adjacencyList->GetKeyAtIndex(i); - if (key!=nap.parent->data) - { - nap2.node= MafiaNet::OP_NEW >( _FILE_AND_LINE_ ); - nap2.node->data=key; - nap2.parent=current; - nodesToProcess.Push(nap2, _FILE_AND_LINE_ ); - current->children.Insert(nap2.node, _FILE_AND_LINE_); - } - } - } - - return true; - } - - template - void WeightedGraph::GenerateDisjktraMatrix(node_type startNode, weight_type INFINITE_WEIGHT) - { - if (adjacencyLists.Size()==0) - return; - - costMatrix = MafiaNet::OP_NEW_ARRAY(adjacencyLists.Size() * adjacencyLists.Size(), _FILE_AND_LINE_ ); - leastNodeArray = MafiaNet::OP_NEW_ARRAY(adjacencyLists.Size(), _FILE_AND_LINE_ ); - - node_type currentNode; - unsigned col, row, row2, openSetIndex; - node_type adjacentKey; - unsigned adjacentIndex; - weight_type edgeWeight, currentNodeWeight, adjacentNodeWeight; - DataStructures::Map *adjacencyList; - DataStructures::Heap minHeap; - DataStructures::Map openSet; - - for (col=0; col < adjacencyLists.Size(); col++) - { - // This should be already sorted, so it's a bit inefficient to do an insertion sort, but what the heck - costMatrixIndices.Insert(adjacencyLists.GetKeyAtIndex(col),adjacencyLists.GetKeyAtIndex(col), true, _FILE_AND_LINE_); - } - for (col=0; col < adjacencyLists.Size() * adjacencyLists.Size(); col++) - costMatrix[col]=INFINITE_WEIGHT; - currentNode=startNode; - row=0; - currentNodeWeight=0; - rootNode=startNode; - - // Clear the starting node column - if (adjacencyLists.Size()) - { - adjacentIndex=adjacencyLists.GetIndexAtKey(startNode); - for (row2=0; row2 < adjacencyLists.Size(); row2++) - costMatrix[row2*adjacencyLists.Size() + adjacentIndex]=0; - } - - while (row < adjacencyLists.Size()-1) - { - adjacencyList = adjacencyLists.Get(currentNode); - // Go through all connections from the current node. If the new weight is less than the current weight, then update that weight. - for (col=0; col < adjacencyList->Size(); col++) - { - edgeWeight=(*adjacencyList)[col]; - adjacentKey=adjacencyList->GetKeyAtIndex(col); - adjacentIndex=adjacencyLists.GetIndexAtKey(adjacentKey); - adjacentNodeWeight=costMatrix[row*adjacencyLists.Size() + adjacentIndex]; - - if (currentNodeWeight + edgeWeight < adjacentNodeWeight) - { - // Update the weight for the adjacent node - for (row2=row; row2 < adjacencyLists.Size(); row2++) - costMatrix[row2*adjacencyLists.Size() + adjacentIndex]=currentNodeWeight + edgeWeight; - openSet.Set(adjacentKey, currentNodeWeight + edgeWeight); - } - } - - // Find the lowest in the open set - minHeap.Clear(true,_FILE_AND_LINE_); - for (openSetIndex=0; openSetIndex < openSet.Size(); openSetIndex++) - minHeap.Push(openSet[openSetIndex], openSet.GetKeyAtIndex(openSetIndex),_FILE_AND_LINE_); - - /* - unsigned i,j; - for (i=0; i < adjacencyLists.Size()-1; i++) - { - for (j=0; j < adjacencyLists.Size(); j++) - { - RAKNET_DEBUG_PRINTF("%2i ", costMatrix[i*adjacencyLists.Size() + j]); - } - RAKNET_DEBUG_PRINTF("Node=%i", leastNodeArray[i]); - RAKNET_DEBUG_PRINTF("\n"); - } - */ - - if (minHeap.Size()==0) - { - // Unreachable nodes - isValidPath=true; - return; - } - - currentNodeWeight=minHeap.PeekWeight(0); - leastNodeArray[row]=minHeap.Pop(0); - currentNode=leastNodeArray[row]; - openSet.Delete(currentNode); - row++; - } - - /* -#ifdef _DEBUG - unsigned i,j; - for (i=0; i < adjacencyLists.Size()-1; i++) - { - for (j=0; j < adjacencyLists.Size(); j++) - { - RAKNET_DEBUG_PRINTF("%2i ", costMatrix[i*adjacencyLists.Size() + j]); - } - RAKNET_DEBUG_PRINTF("Node=%i", leastNodeArray[i]); - RAKNET_DEBUG_PRINTF("\n"); - } -#endif - */ - - isValidPath=true; - } - - template - void WeightedGraph::ClearDijkstra(void) - { - if (isValidPath) - { - isValidPath=false; - MafiaNet::OP_DELETE_ARRAY(costMatrix, _FILE_AND_LINE_); - MafiaNet::OP_DELETE_ARRAY(leastNodeArray, _FILE_AND_LINE_); - costMatrixIndices.Clear(false, _FILE_AND_LINE_); - } - } - - template - void WeightedGraph::Print(void) - { -#ifdef _DEBUG - unsigned i,j; - for (i=0; i < adjacencyLists.Size(); i++) - { - //RAKNET_DEBUG_PRINTF("%i connected to ", i); - RAKNET_DEBUG_PRINTF("%s connected to ", adjacencyLists.GetKeyAtIndex(i).systemAddress.ToString()); - - if (adjacencyLists[i]->Size()==0) - RAKNET_DEBUG_PRINTF(""); - else - { - for (j=0; j < adjacencyLists[i]->Size(); j++) - // RAKNET_DEBUG_PRINTF("%i (%.2f) ", adjacencyLists.GetIndexAtKey(adjacencyLists[i]->GetKeyAtIndex(j)), (float) adjacencyLists[i]->operator[](j) ); - RAKNET_DEBUG_PRINTF("%s (%.2f) ", adjacencyLists[i]->GetKeyAtIndex(j).systemAddress.ToString(), (float) adjacencyLists[i]->operator[](j) ); - } - - RAKNET_DEBUG_PRINTF("\n"); - } -#endif - } -} - -#ifdef _MSC_VER -#pragma warning( pop ) -#endif - -#endif From 0ae95ffa946722e2c441924e7a5f18d3a18efbce Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Thu, 8 Oct 2026 18:24:49 +0200 Subject: [PATCH 2/8] refactor(ds): move Table's rows from the B+ tree to std::map and delete BPlusTree Second increment of the stage 1 container migration, and an API break: it changes a public signature on an installed header, which #59 had scoped as API-neutral. Taken deliberately after confirming Table is the only consumer BPlusTree has. Two corrections to the issue's scope table: - It lists reliability_layer.h as a BPlusTree consumer for "split-packet channel lookup". It is not one. The member there is commented out, the datagramMessageIDTree code in reliability_layer.cpp sits inside a /* */ block and the symbol has no declaration anywhere. Only the #include was real, and it is now gone. So nothing performance-sensitive is touched by this commit. - It puts Table out of scope in every stage while also requiring no DataStructures:: use of the listed containers to remain, which cannot both hold: Table holds the rows and returns the tree from a public GetRows(). Changes: - Table::rows becomes std::map. std::map iterates ascending by key exactly as the B+ tree leaf chain did, so query, sort and serialization order are unchanged -- including the wire order of rows, so a peer on the old code deserializes a new peer's table identically. - GetRows() returns const std::map&. - GetListHead() is removed. It returned a B+ tree leaf page and had no caller; the Page it exposed is the tree's node type from ds_bplus_tree.h, unrelated to the MemoryPool Page that the issue keeps out of scope. - _TABLE_BPLUS_TREE_ORDER is removed, being meaningless without the tree. - Consumers updated: table_serializer.cpp, and four leaf-page walks in DependentExtensions/Lobby2/Rooms/RoomsContainer.cpp. - ds_bplus_tree.h deleted. Tests/Unit/TableTests.cpp is new, because Table had no coverage whatsoever: the migration compiled and the whole existing suite passed without a single row being exercised. 16 cases pin the behaviour the B+ tree provided, in particular the two properties a keyed-container swap can break silently -- ascending row-id iteration order everywhere it is observable, and AddRow refusing a duplicate id rather than overwriting. Checked that they bite: a mutant dropping the duplicate-id refusal fails 4 of them. The serialization case needs StringCompressor::AddReference(), since TableSerializer dereferences that singleton and no RakPeer exists to take the reference in a hermetic test; without it the serializer segfaults, which is pre-existing and documented on StringCompressor::Instance(). Verified on macOS arm64: full ctest 301/301, samples build including Lobby2 clean, and the Table suite clean under AddressSanitizer. --- .../Lobby2/Rooms/RoomsContainer.cpp | 59 +- Source/CMakeLists.txt | 1 - Source/include/mafianet/ds_bplus_tree.h | 1157 ----------------- Source/include/mafianet/ds_table.h | 14 +- Source/include/mafianet/reliability_layer.h | 1 - Source/src/ds_table.cpp | 165 +-- Source/src/table_serializer.cpp | 17 +- Tests/Unit/TableTests.cpp | 433 ++++++ 8 files changed, 520 insertions(+), 1327 deletions(-) delete mode 100644 Source/include/mafianet/ds_bplus_tree.h create mode 100644 Tests/Unit/TableTests.cpp diff --git a/DependentExtensions/Lobby2/Rooms/RoomsContainer.cpp b/DependentExtensions/Lobby2/Rooms/RoomsContainer.cpp index 0865d3168..19ea1e714 100644 --- a/DependentExtensions/Lobby2/Rooms/RoomsContainer.cpp +++ b/DependentExtensions/Lobby2/Rooms/RoomsContainer.cpp @@ -1224,20 +1224,15 @@ RoomsErrorCode PerGameRoomsContainer::SearchByFilter( RoomsParticipant* roomsPar roomsTable.QueryTable(columnIndices,1,roomQuery->queries,roomQuery->numQueries,0,0,&resultTable); roomsOutput.Clear(false, _FILE_AND_LINE_); - DataStructures::Page *cur = resultTable.GetRows().GetListHead(); - int i; + const std::map &resultRows = resultTable.GetRows(); Room *room; - while (cur) + for (std::map::const_iterator it = resultRows.begin(); it != resultRows.end(); ++it) { - for (i=0; i < cur->size; i++) - { - // Put all the pointers in the roomSort list, filtering out those you cannot join (full, or no public and you are not invited) - room = (Room*) cur->data[i]->cells[0]->ptr; - if ( (onlyJoinable==false || room->ParticipantCanJoinRoom(roomsParticipant, false, true)==PCJRR_SUCCESS) && - room->IsHiddenToParticipant(roomsParticipant)==false) - roomsOutput.Insert(room,room,true, _FILE_AND_LINE_ ); - } - cur=cur->next; + // Put all the pointers in the roomSort list, filtering out those you cannot join (full, or no public and you are not invited) + room = (Room*) it->second->cells[0]->ptr; + if ( (onlyJoinable==false || room->ParticipantCanJoinRoom(roomsParticipant, false, true)==PCJRR_SUCCESS) && + room->IsHiddenToParticipant(roomsParticipant)==false) + roomsOutput.Insert(room,room,true, _FILE_AND_LINE_ ); } return REC_SUCCESS; } @@ -1271,19 +1266,15 @@ void PerGameRoomsContainer::RoomPrioritySort( RoomsParticipant* roomsParticipant roomsTable.QueryTable(columnIndices,1,roomQuery->queries,roomQuery->numQueries,0,0,&resultTable); roomsOutput.Clear(false, _FILE_AND_LINE_); - DataStructures::Page *cur = resultTable.GetRows().GetListHead(); + const std::map &resultRows = resultTable.GetRows(); Room *room; - while (cur) + for (std::map::const_iterator it = resultRows.begin(); it != resultRows.end(); ++it) { - for (i=0; i < cur->size; i++) - { - // Put all the pointers in the roomSort list, filtering out those you cannot join (full, or no public and you are not invited) - room = (Room*) cur->data[i]->cells[0]->ptr; - if (room->ParticipantCanJoinRoom(roomsParticipant, false, true)==PCJRR_SUCCESS && - room->IsHiddenToParticipant(roomsParticipant)==false) - roomsOutput.Insert(room,room,true, _FILE_AND_LINE_ ); - } - cur=cur->next; + // Put all the pointers in the roomSort list, filtering out those you cannot join (full, or no public and you are not invited) + room = (Room*) it->second->cells[0]->ptr; + if (room->ParticipantCanJoinRoom(roomsParticipant, false, true)==PCJRR_SUCCESS && + room->IsHiddenToParticipant(roomsParticipant)==false) + roomsOutput.Insert(room,room,true, _FILE_AND_LINE_ ); } } Room* PerGameRoomsContainer::GetRoomByLobbyRoomID(RoomID lobbyRoomID) @@ -1310,27 +1301,17 @@ Room * PerGameRoomsContainer::GetRoomByName(MafiaNet::RakString roomName) } void PerGameRoomsContainer::GetAllRooms(DataStructures::List &rooms) { - DataStructures::Page *cur = roomsTable.GetRows().GetListHead(); - int i; + const std::map &allRows = roomsTable.GetRows(); rooms.Clear(false, _FILE_AND_LINE_); - while (cur) - { - for (i=0; i < cur->size; i++) - rooms.Insert((Room*)cur->data[i]->cells[DefaultRoomColumns::TC_LOBBY_ROOM_PTR]->ptr, _FILE_AND_LINE_ ); - cur=cur->next; - } + for (std::map::const_iterator it = allRows.begin(); it != allRows.end(); ++it) + rooms.Insert((Room*)it->second->cells[DefaultRoomColumns::TC_LOBBY_ROOM_PTR]->ptr, _FILE_AND_LINE_ ); } void PerGameRoomsContainer::GetRoomNames(DataStructures::List &roomNames) { - DataStructures::Page *cur = roomsTable.GetRows().GetListHead(); - int i; + const std::map &nameRows = roomsTable.GetRows(); roomNames.Clear(false, _FILE_AND_LINE_); - while (cur) - { - for (i=0; i < cur->size; i++) - roomNames.Insert(MafiaNet::RakString(cur->data[i]->cells[DefaultRoomColumns::TC_ROOM_NAME]->c), _FILE_AND_LINE_ ); - cur=cur->next; - } + for (std::map::const_iterator it = nameRows.begin(); it != nameRows.end(); ++it) + roomNames.Insert(MafiaNet::RakString(it->second->cells[DefaultRoomColumns::TC_ROOM_NAME]->c), _FILE_AND_LINE_ ); } unsigned int PerGameRoomsContainer::GetQuickJoinIndex(RoomsParticipant* roomsParticipant) diff --git a/Source/CMakeLists.txt b/Source/CMakeLists.txt index 9cd9157a5..a7543dd17 100644 --- a/Source/CMakeLists.txt +++ b/Source/CMakeLists.txt @@ -156,7 +156,6 @@ set(MAFIANET_HEADERS include/mafianet/directory_delta_transfer.h include/mafianet/dr_sha1.h include/mafianet/ds_binary_search_tree.h - include/mafianet/ds_bplus_tree.h include/mafianet/ds_byte_pool.h include/mafianet/ds_byte_queue.h include/mafianet/ds_hash.h diff --git a/Source/include/mafianet/ds_bplus_tree.h b/Source/include/mafianet/ds_bplus_tree.h deleted file mode 100644 index ab37211ef..000000000 --- a/Source/include/mafianet/ds_bplus_tree.h +++ /dev/null @@ -1,1157 +0,0 @@ -/* - * Original work: Copyright (c) 2014, Oculus VR, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * RakNet License.txt file in the licenses directory of this source tree. An additional grant - * of patent rights can be found in the RakNet Patents.txt file in the same directory. - * - * - * Modified work: Copyright (c) 2016-2018, SLikeSoft UG (haftungsbeschränkt) - * - * This source code was modified by SLikeSoft. Modifications are licensed under the MIT-style - * license found in the license.txt file in the root directory of this source tree. - */ - -/// \file ds_bplus_tree.h -/// - - -#ifndef __B_PLUS_TREE_CPP -#define __B_PLUS_TREE_CPP - -#include "mafianet/ds_memory_pool.h" -#include "mafianet/ds_queue.h" -#include -#include "mafianet/export.h" - -// Java -// http://www.seanster.com/BplusTree/BplusTree.html - -// Overview -// http://babbage.clarku.edu/~achou/cs160/B+Trees/B+Trees.htm - -// Deletion -// http://dbpubs.stanford.edu:8090/pub/1995-19 - -#ifdef _MSC_VER -#pragma warning( push ) -#endif - -#include "mafianet/memory_override.h" - -/// The namespace DataStructures was only added to avoid compiler errors for commonly named data structures -/// As these data structures are stand-alone, you can use them outside of RakNet for your own projects if you wish. -namespace DataStructures -{ - /// Used in the BPlusTree. Used for both leaf and index nodes. - /// Don't use a constructor or destructor, due to the memory pool I am using - template - struct RAK_DLL_EXPORT Page - { - // We use the same data structure for both leaf and index nodes. - // It uses a little more memory for index nodes but reduces - // memory fragmentation, allocations, and deallocations. - bool isLeaf; - - // Used for both leaf and index nodes. - // For a leaf it means the number of elements in data - // For an index it means the number of keys and is one less than the number of children pointers. - int size; - - // Used for both leaf and index nodes. - KeyType keys[order]; - - // Used only for leaf nodes. Data is the actual data, while next is the pointer to the next leaf (for B+) - DataType data[order]; - Page *next; - Page *previous; - - // Used only for index nodes. Pointers to the children of this node. - Page *children[order+1]; - }; - - /// A BPlus tree - /// Written with efficiency and speed in mind. - template - class RAK_DLL_EXPORT BPlusTree - { - public: - struct ReturnAction - { - KeyType key1; - KeyType key2; - enum - { - NO_ACTION, - REPLACE_KEY1_WITH_KEY2, - PUSH_KEY_TO_PARENT, - SET_BRANCH_KEY, - } action; // 0=none, 1=replace key1 with key2 - }; - - BPlusTree(); - ~BPlusTree(); - void SetPoolPageSize(int size); // Set the page size for the memory pool. Optionsl - bool Get(const KeyType key, DataType &out) const; - bool Delete(const KeyType key); - bool Delete(const KeyType key, DataType &out); - bool Insert(const KeyType key, const DataType &data); - void Clear(void); - unsigned Size(void) const; - bool IsEmpty(void) const; - Page *GetListHead(void) const; - DataType GetDataHead(void) const; - void PrintLeaves(void); - void ForEachLeaf(void (*func)(Page * leaf, int index)); - void ForEachData(void (*func)(DataType input, int index)); - void PrintGraph(void); - protected: - void ValidateTreeRecursive(Page *cur); - void DeleteFromPageAtIndex(const int index, Page *cur); - static void PrintLeaf(Page * leaf, int index); - void FreePages(void); - bool GetIndexOf(const KeyType key, Page *page, int *out) const; - void ShiftKeysLeft(Page *cur); - bool CanRotateLeft(Page *cur, int childIndex); - bool CanRotateRight(Page *cur, int childIndex); - void RotateRight(Page *cur, int childIndex, ReturnAction *returnAction); - void RotateLeft(Page *cur, int childIndex, ReturnAction *returnAction); - Page* InsertIntoNode(const KeyType key, const DataType &childData, int insertionIndex, Page *nodeData, Page *cur, ReturnAction* returnAction); - Page* InsertBranchDown(const KeyType key, const DataType &data,Page *cur, ReturnAction* returnAction, bool *success); - Page* GetLeafFromKey(const KeyType key) const; - bool FindDeleteRebalance(const KeyType key, Page *cur, bool *underflow, KeyType rightRootKey, ReturnAction *returnAction, DataType &out); - bool FixUnderflow(int branchIndex, Page *cur, KeyType rightRootKey, ReturnAction *returnAction); - void ShiftNodeLeft(Page *cur); - void ShiftNodeRight(Page *cur); - - MemoryPool > pagePool; - Page *root, *leftmostLeaf; - }; - - template - BPlusTree::BPlusTree () - { - RakAssert(order>1); - root=0; - leftmostLeaf=0; - } - template - BPlusTree::~BPlusTree () - { - Clear(); - } - template - void BPlusTree::SetPoolPageSize(int size) - { - pagePool.SetPageSize(size); - } - template - bool BPlusTree::Get(const KeyType key, DataType &out) const - { - if (root==0) - return false; - - Page* leaf = GetLeafFromKey(key); - int childIndex; - - if (GetIndexOf(key, leaf, &childIndex)) - { - out=leaf->data[childIndex]; - return true; - } - return false; - } - template - void BPlusTree::DeleteFromPageAtIndex(const int index, Page *cur) - { - int i; - for (i=index; i < cur->size-1; i++) - cur->keys[i]=cur->keys[i+1]; - if (cur->isLeaf) - { - for (i=index; i < cur->size-1; i++) - cur->data[i]=cur->data[i+1]; - } - else - { - for (i=index; i < cur->size-1; i++) - cur->children[i+1]=cur->children[i+2]; - } - cur->size--; - } - template - bool BPlusTree::Delete(const KeyType key) - { - DataType temp; - return Delete(key, temp); - } - template - bool BPlusTree::Delete(const KeyType key, DataType &out) - { - if (root==0) - return false; - - ReturnAction returnAction; - returnAction.action=ReturnAction::NO_ACTION; - int childIndex; - bool underflow=false; - if (root==leftmostLeaf) - { - if (GetIndexOf(key, root, &childIndex)==false) - return false; - out=root->data[childIndex]; - DeleteFromPageAtIndex(childIndex,root); - if (root->size==0) - { - pagePool.Release(root, _FILE_AND_LINE_); - root=0; - leftmostLeaf=0; - } - return true; - } - else if (FindDeleteRebalance(key, root, &underflow,root->keys[0], &returnAction, out)==false) - return false; - -// RakAssert(returnAction.action==ReturnAction::NO_ACTION); - - if (underflow && root->size==0) - { - // Move the root down. - Page *oldRoot=root; - root=root->children[0]; - pagePool.Release(oldRoot, _FILE_AND_LINE_); - // memset(oldRoot,0,sizeof(root)); - } - - return true; - } - template - bool BPlusTree::FindDeleteRebalance(const KeyType key, Page *cur, bool *underflow, KeyType rightRootKey, ReturnAction *returnAction, DataType &out) - { - // Get index of child to follow. - int branchIndex, childIndex; - if (GetIndexOf(key, cur, &childIndex)) - branchIndex=childIndex+1; - else - branchIndex=childIndex; - - // If child is not a leaf, call recursively - if (cur->children[branchIndex]->isLeaf==false) - { - if (branchIndexsize) - rightRootKey=cur->keys[branchIndex]; // Shift right to left - else - rightRootKey=cur->keys[branchIndex-1]; // Shift center to left - - if (FindDeleteRebalance(key, cur->children[branchIndex], underflow, rightRootKey, returnAction, out)==false) - return false; - - // Call again in case the root key changed - if (branchIndexsize) - rightRootKey=cur->keys[branchIndex]; // Shift right to left - else - rightRootKey=cur->keys[branchIndex-1]; // Shift center to left - - if (returnAction->action==ReturnAction::SET_BRANCH_KEY && branchIndex!=childIndex) - { - returnAction->action=ReturnAction::NO_ACTION; - cur->keys[childIndex]=returnAction->key1; - - if (branchIndexsize) - rightRootKey=cur->keys[branchIndex]; // Shift right to left - else - rightRootKey=cur->keys[branchIndex-1]; // Shift center to left - } - } - else - { - // If child is a leaf, get the index of the key. If the item is not found, cancel delete. - if (GetIndexOf(key, cur->children[branchIndex], &childIndex)==false) - return false; - - // Delete: - // Remove childIndex from the child at branchIndex - out=cur->children[branchIndex]->data[childIndex]; - DeleteFromPageAtIndex(childIndex, cur->children[branchIndex]); - - if (childIndex==0) - { - if (branchIndex>0) - cur->keys[branchIndex-1]=cur->children[branchIndex]->keys[0]; - - if (branchIndex==0) - { - returnAction->action=ReturnAction::SET_BRANCH_KEY; - returnAction->key1=cur->children[0]->keys[0]; - } - } - - if (cur->children[branchIndex]->size < order/2) - *underflow=true; - else - *underflow=false; - } - - // Fix underflow: - if (*underflow) - { - *underflow=FixUnderflow(branchIndex, cur, rightRootKey, returnAction); - } - - return true; - } - template - bool BPlusTree::FixUnderflow(int branchIndex, Page *cur, KeyType rightRootKey, ReturnAction *returnAction) - { - // Borrow from a neighbor that has excess. - Page *source; - Page *dest; - - if (branchIndex>0 && cur->children[branchIndex-1]->size > order/2) - { - dest=cur->children[branchIndex]; - source=cur->children[branchIndex-1]; - - // Left has excess - ShiftNodeRight(dest); - if (dest->isLeaf) - { - dest->keys[0]=source->keys[source->size-1]; - dest->data[0]=source->data[source->size-1]; - } - else - { - dest->children[0]=source->children[source->size]; - dest->keys[0]=cur->keys[branchIndex-1]; - } - // Update the parent key for the child (middle) - cur->keys[branchIndex-1]=source->keys[source->size-1]; - source->size--; - - // if (branchIndex==0) - // { - // returnAction->action=ReturnAction::SET_BRANCH_KEY; - // returnAction->key1=dest->keys[0]; - // } - - // No underflow - return false; - } - else if (branchIndexsize && cur->children[branchIndex+1]->size > order/2) - { - dest=cur->children[branchIndex]; - source=cur->children[branchIndex+1]; - - // Right has excess - if (dest->isLeaf) - { - dest->keys[dest->size]=source->keys[0]; - dest->data[dest->size]=source->data[0]; - - // The first key in the leaf after shifting is the parent key for the right branch - cur->keys[branchIndex]=source->keys[1]; - -#ifdef _MSC_VER -#pragma warning( disable : 4127 ) // warning C4127: conditional expression is constant -#endif - if (order<=3 && dest->size==0) - { - if (branchIndex==0) - { - returnAction->action=ReturnAction::SET_BRANCH_KEY; - returnAction->key1=dest->keys[0]; - } - else - cur->keys[branchIndex-1]=cur->children[branchIndex]->keys[0]; - } - } - else - { - if (returnAction->action==ReturnAction::NO_ACTION) - { - returnAction->action=ReturnAction::SET_BRANCH_KEY; - returnAction->key1=dest->keys[0]; - } - - dest->keys[dest->size]=rightRootKey; - dest->children[dest->size+1]=source->children[0]; - - // The shifted off key is the leftmost key for a node - cur->keys[branchIndex]=source->keys[0]; - } - - - dest->size++; - ShiftNodeLeft(source); - - //cur->keys[branchIndex]=source->keys[0]; - -// returnAction->action=ReturnAction::SET_BRANCH_KEY; -// returnAction->key1=dest->keys[dest->size-1]; - - // No underflow - return false; - } - else - { - int sourceIndex; - - // If no neighbors have excess, merge two branches. - // - // To merge two leaves, just copy the data and keys over. - // - // To merge two branches, copy the pointers and keys over, using rightRootKey as the key for the extra pointer - if (branchIndexsize) - { - // Merge right child to current child and delete right child. - dest=cur->children[branchIndex]; - source=cur->children[branchIndex+1]; - } - else - { - // Move current child to left and delete current child - dest=cur->children[branchIndex-1]; - source=cur->children[branchIndex]; - } - - // Merge - if (dest->isLeaf) - { - for (sourceIndex=0; sourceIndexsize; sourceIndex++) - { - dest->keys[dest->size]=source->keys[sourceIndex]; - dest->data[dest->size++]=source->data[sourceIndex]; - } - } - else - { - // We want the tree root key of the source, not the current. - dest->keys[dest->size]=rightRootKey; - dest->children[dest->size++ + 1]=source->children[0]; - for (sourceIndex=0; sourceIndexsize; sourceIndex++) - { - dest->keys[dest->size]=source->keys[sourceIndex]; - dest->children[dest->size++ + 1]=source->children[sourceIndex + 1]; - } - } - -#ifdef _MSC_VER -#pragma warning( disable : 4127 ) // warning C4127: conditional expression is constant -#endif - if (order<=3 && branchIndex>0 && cur->children[branchIndex]->isLeaf) // With order==2 it is possible to delete data[0], which is not possible with higher orders. - cur->keys[branchIndex-1]=cur->children[branchIndex]->keys[0]; - - if (branchIndexsize) - { - // Update the parent key, removing the source (right) - DeleteFromPageAtIndex(branchIndex, cur); - } - else - { - if (branchIndex>0) - { - // Update parent key, removing the source (current) - DeleteFromPageAtIndex(branchIndex-1, cur); - } - } - - if (branchIndex==0 && dest->isLeaf) - { - returnAction->action=ReturnAction::SET_BRANCH_KEY; - returnAction->key1=dest->keys[0]; - } - - if (source==leftmostLeaf) - leftmostLeaf=source->next; - - if (source->isLeaf) - { - if (source->previous) - source->previous->next=source->next; - if (source->next) - source->next->previous=source->previous; - } - - // Free the source node - pagePool.Release(source, _FILE_AND_LINE_); - // memset(source,0,sizeof(root)); - - // Return underflow or not of parent. - return cur->size < order/2; - } - } - template - void BPlusTree::ShiftNodeRight(Page *cur) - { - int i; - for (i=cur->size; i>0; i--) - cur->keys[i]=cur->keys[i-1]; - if (cur->isLeaf) - { - for (i=cur->size; i>0; i--) - cur->data[i]=cur->data[i-1]; - } - else - { - for (i=cur->size+1; i>0; i--) - cur->children[i]=cur->children[i-1]; - } - - cur->size++; - } - template - void BPlusTree::ShiftNodeLeft(Page *cur) - { - int i; - for (i=0; i < cur->size-1; i++) - cur->keys[i]=cur->keys[i+1]; - if (cur->isLeaf) - { - for (i=0; i < cur->size; i++) - cur->data[i]=cur->data[i+1]; - } - else - { - for (i=0; i < cur->size; i++) - cur->children[i]=cur->children[i+1]; - } - cur->size--; - } - template - Page* BPlusTree::InsertIntoNode(const KeyType key, const DataType &leafData, int insertionIndex, Page *nodeData, Page *cur, ReturnAction* returnAction) - { - int i; - if (cur->size < order) - { - for (i=cur->size; i > insertionIndex; i--) - cur->keys[i]=cur->keys[i-1]; - if (cur->isLeaf) - { - for (i=cur->size; i > insertionIndex; i--) - cur->data[i]=cur->data[i-1]; - } - else - { - for (i=cur->size+1; i > insertionIndex+1; i--) - cur->children[i]=cur->children[i-1]; - } - cur->keys[insertionIndex]=key; - if (cur->isLeaf) - cur->data[insertionIndex]=leafData; - else - cur->children[insertionIndex+1]=nodeData; - - cur->size++; - } - else - { - Page* newPage = pagePool.Allocate( _FILE_AND_LINE_ ); - newPage->isLeaf=cur->isLeaf; - if (cur->isLeaf) - { - newPage->next=cur->next; - if (cur->next) - cur->next->previous=newPage; - newPage->previous=cur; - cur->next=newPage; - } - - int destIndex, sourceIndex; - - if (insertionIndex>=(order+1)/2) - { - destIndex=0; - sourceIndex=order/2; - - for (; sourceIndex < insertionIndex; sourceIndex++, destIndex++) - { - newPage->keys[destIndex]=cur->keys[sourceIndex]; - } - newPage->keys[destIndex++]=key; - for (; sourceIndex < order; sourceIndex++, destIndex++) - { - newPage->keys[destIndex]=cur->keys[sourceIndex]; - } - - destIndex=0; - sourceIndex=order/2; - if (cur->isLeaf) - { - for (; sourceIndex < insertionIndex; sourceIndex++, destIndex++) - { - newPage->data[destIndex]=cur->data[sourceIndex]; - } - newPage->data[destIndex++]=leafData; - for (; sourceIndex < order; sourceIndex++, destIndex++) - { - newPage->data[destIndex]=cur->data[sourceIndex]; - } - } - else - { - - for (; sourceIndex < insertionIndex; sourceIndex++, destIndex++) - { - newPage->children[destIndex]=cur->children[sourceIndex+1]; - } - newPage->children[destIndex++]=nodeData; - - // sourceIndex+1 is sort of a hack but it works - because there is one extra child than keys - // skip past the last child for cur - for (; sourceIndex+1 < cur->size+1; sourceIndex++, destIndex++) - { - newPage->children[destIndex]=cur->children[sourceIndex+1]; - } - - // the first key is the middle key. Remove it from the page and push it to the parent - returnAction->action=ReturnAction::PUSH_KEY_TO_PARENT; - returnAction->key1=newPage->keys[0]; - for (int j=0; j < destIndex-1; j++) - newPage->keys[j]=newPage->keys[j+1]; - - } - cur->size=order/2; - } - else - { - destIndex=0; - sourceIndex=(order+1)/2-1; - for (; sourceIndex < order; sourceIndex++, destIndex++) - newPage->keys[destIndex]=cur->keys[sourceIndex]; - destIndex=0; - if (cur->isLeaf) - { - sourceIndex=(order+1)/2-1; - for (; sourceIndex < order; sourceIndex++, destIndex++) - newPage->data[destIndex]=cur->data[sourceIndex]; - } - else - { - sourceIndex=(order+1)/2; - for (; sourceIndex < order+1; sourceIndex++, destIndex++) - newPage->children[destIndex]=cur->children[sourceIndex]; - - // the first key is the middle key. Remove it from the page and push it to the parent - returnAction->action=ReturnAction::PUSH_KEY_TO_PARENT; - returnAction->key1=newPage->keys[0]; - for (int j=0; j < destIndex-1; j++) - newPage->keys[j]=newPage->keys[j+1]; - } - cur->size=(order+1)/2-1; - if (cur->size) - { - bool b = GetIndexOf(key, cur, &insertionIndex); - (void) b; - RakAssert(b==false); - } - else - insertionIndex=0; - InsertIntoNode(key, leafData, insertionIndex, nodeData, cur, returnAction); - } - - newPage->size=destIndex; - - return newPage; - } - - return 0; - } - - template - bool BPlusTree::CanRotateLeft(Page *cur, int childIndex) - { - return childIndex>0 && cur->children[childIndex-1]->size - void BPlusTree::RotateLeft(Page *cur, int childIndex, ReturnAction *returnAction) - { - Page *dest = cur->children[childIndex-1]; - Page *source = cur->children[childIndex]; - returnAction->key1=source->keys[0]; - dest->keys[dest->size]=source->keys[0]; - dest->data[dest->size]=source->data[0]; - dest->size++; - for (int i=0; i < source->size-1; i++) - { - source->keys[i]=source->keys[i+1]; - source->data[i]=source->data[i+1]; - } - source->size--; - cur->keys[childIndex-1]=source->keys[0]; - returnAction->key2=source->keys[0]; - } - - template - bool BPlusTree::CanRotateRight(Page *cur, int childIndex) - { - return childIndex < cur->size && cur->children[childIndex+1]->size - void BPlusTree::RotateRight(Page *cur, int childIndex, ReturnAction *returnAction) - { - Page *dest = cur->children[childIndex+1]; - Page *source = cur->children[childIndex]; - returnAction->key1=dest->keys[0]; - for (int i= dest->size; i > 0; i--) - { - dest->keys[i]=dest->keys[i-1]; - dest->data[i]=dest->data[i-1]; - } - dest->keys[0]=source->keys[source->size-1]; - dest->data[0]=source->data[source->size-1]; - dest->size++; - source->size--; - - cur->keys[childIndex]=dest->keys[0]; - returnAction->key2=dest->keys[0]; - } - template - Page* BPlusTree::GetLeafFromKey(const KeyType key) const - { - Page* cur = root; - int childIndex; - while (cur->isLeaf==false) - { - // When searching, if we match the exact key we go down the pointer after that index - if (GetIndexOf(key, cur, &childIndex)) - childIndex++; - cur = cur->children[childIndex]; - } - return cur; - } - - template - Page* BPlusTree::InsertBranchDown(const KeyType key, const DataType &data,Page *cur, ReturnAction *returnAction, bool *success) - { - int childIndex; - int branchIndex; - if (GetIndexOf(key, cur, &childIndex)) - branchIndex=childIndex+1; - else - branchIndex=childIndex; - Page* newPage; - if (cur->isLeaf==false) - { - if (cur->children[branchIndex]->isLeaf==true && cur->children[branchIndex]->size==order) - { - if (branchIndex==childIndex+1) - { - *success=false; - return 0; // Already exists - } - - if (CanRotateLeft(cur, branchIndex)) - { - returnAction->action=ReturnAction::REPLACE_KEY1_WITH_KEY2; - if (key > cur->children[branchIndex]->keys[0]) - { - RotateLeft(cur, branchIndex, returnAction); - - int insertionIndex; - GetIndexOf(key, cur->children[branchIndex], &insertionIndex); - InsertIntoNode(key, data, insertionIndex, 0, cur->children[branchIndex], 0); - } - else - { - // Move head element to left and replace it with key,data - Page* dest=cur->children[branchIndex-1]; - Page* source=cur->children[branchIndex]; - returnAction->key1=source->keys[0]; - returnAction->key2=key; - dest->keys[dest->size]=source->keys[0]; - dest->data[dest->size]=source->data[0]; - dest->size++; - source->keys[0]=key; - source->data[0]=data; - } - cur->keys[branchIndex-1]=cur->children[branchIndex]->keys[0]; - - return 0; - } - else if (CanRotateRight(cur, branchIndex)) - { - returnAction->action=ReturnAction::REPLACE_KEY1_WITH_KEY2; - - if (key < cur->children[branchIndex]->keys[cur->children[branchIndex]->size-1]) - { - RotateRight(cur, branchIndex, returnAction); - - int insertionIndex; - GetIndexOf(key, cur->children[branchIndex], &insertionIndex); - InsertIntoNode(key, data, insertionIndex, 0, cur->children[branchIndex], 0); - - } - else - { - // Insert to the head of the right leaf instead and change our key - returnAction->key1=cur->children[branchIndex+1]->keys[0]; - InsertIntoNode(key, data, 0, 0, cur->children[branchIndex+1], 0); - returnAction->key2=key; - } - cur->keys[branchIndex]=cur->children[branchIndex+1]->keys[0]; - return 0; - } - } - - newPage=InsertBranchDown(key,data,cur->children[branchIndex], returnAction, success); - if (returnAction->action==ReturnAction::REPLACE_KEY1_WITH_KEY2) - { - if (branchIndex>0 && cur->keys[branchIndex-1]==returnAction->key1) - cur->keys[branchIndex-1]=returnAction->key2; - } - if (newPage) - { - if (newPage->isLeaf==false) - { - RakAssert(returnAction->action==ReturnAction::PUSH_KEY_TO_PARENT); - newPage->size--; - return InsertIntoNode(returnAction->key1, data, branchIndex, newPage, cur, returnAction); - } - else - { - return InsertIntoNode(newPage->keys[0], data, branchIndex, newPage, cur, returnAction); - } - } - } - else - { - if (branchIndex==childIndex+1) - { - *success=false; - return 0; // Already exists - } - else - { - return InsertIntoNode(key, data, branchIndex, 0, cur, returnAction); - } - } - - return 0; - } - template - bool BPlusTree::Insert(const KeyType key, const DataType &data) - { - if (root==0) - { - // Allocate root and make root a leaf - root = pagePool.Allocate( _FILE_AND_LINE_ ); - root->isLeaf=true; - leftmostLeaf=root; - root->size=1; - root->keys[0]=key; - root->data[0]=data; - root->next=0; - root->previous=0; - } - else - { - bool success=true; - ReturnAction returnAction; - returnAction.action=ReturnAction::NO_ACTION; - Page* newPage = InsertBranchDown(key, data, root, &returnAction, &success); - if (success==false) - return false; - if (newPage) - { - KeyType newKey; - if (newPage->isLeaf==false) - { - // One key is pushed up through the stack. I store that at keys[0] but it has to be removed for the page to be correct - RakAssert(returnAction.action==ReturnAction::PUSH_KEY_TO_PARENT); - newKey=returnAction.key1; - newPage->size--; - } - else - newKey = newPage->keys[0]; - // propagate the root - Page* newRoot = pagePool.Allocate( _FILE_AND_LINE_ ); - newRoot->isLeaf=false; - newRoot->size=1; - newRoot->keys[0]=newKey; - newRoot->children[0]=root; - newRoot->children[1]=newPage; - root=newRoot; - } - } - - return true; - } - template - void BPlusTree::ShiftKeysLeft(Page *cur) - { - int i; - for (i=0; i < cur->size; i++) - cur->keys[i]=cur->keys[i+1]; - } - template - void BPlusTree::Clear(void) - { - if (root) - { - FreePages(); - leftmostLeaf=0; - root=0; - } - pagePool.Clear(_FILE_AND_LINE_); - } - template - unsigned BPlusTree::Size(void) const - { - unsigned int count=0; - DataStructures::Page *cur = GetListHead(); - while (cur) - { - count+=cur->size; - cur=cur->next; - } - return count; - } - template - bool BPlusTree::IsEmpty(void) const - { - return root==0; - } - template - bool BPlusTree::GetIndexOf(const KeyType key, Page *page, int *out) const - { - RakAssert(page->size>0); - int index, upperBound, lowerBound; - upperBound=page->size-1; - lowerBound=0; - index = page->size/2; - - for(;;) - { - if (key==page->keys[index]) - { - *out=index; - return true; - } - else if (keykeys[index]) - upperBound=index-1; - else - lowerBound=index+1; - - index=lowerBound+(upperBound-lowerBound)/2; - - if (lowerBound>upperBound) - { - *out=lowerBound; - return false; // No match - } - } - } - template - void BPlusTree::FreePages(void) - { - DataStructures::Queue *> queue; - DataStructures::Page *ptr; - int i; - queue.Push(root, _FILE_AND_LINE_ ); - while (queue.Size()) - { - ptr=queue.Pop(); - if (ptr->isLeaf==false) - { - for (i=0; i < ptr->size+1; i++) - queue.Push(ptr->children[i], _FILE_AND_LINE_ ); - } - pagePool.Release(ptr, _FILE_AND_LINE_); - // memset(ptr,0,sizeof(root)); - }; - } - template - Page *BPlusTree::GetListHead(void) const - { - return leftmostLeaf; - } - template - DataType BPlusTree::GetDataHead(void) const - { - return leftmostLeaf->data[0]; - } - template - void BPlusTree::ForEachLeaf(void (*func)(Page * leaf, int index)) - { - int count=0; - DataStructures::Page *cur = GetListHead(); - while (cur) - { - func(cur, count++); - cur=cur->next; - } - } - template - void BPlusTree::ForEachData(void (*func)(DataType input, int index)) - { - int count=0,i; - DataStructures::Page *cur = GetListHead(); - while (cur) - { - for (i=0; i < cur->size; i++) - func(cur->data[i], count++); - cur=cur->next; - } - } - template - void BPlusTree::PrintLeaf(Page * leaf, int index) - { - int i; - RAKNET_DEBUG_PRINTF("%i] SELF=%p\n", index+1, leaf); - for (i=0; i < leaf->size; i++) - // #med - need to adjust printf-format specified based on the actual datatype - RAKNET_DEBUG_PRINTF(" %i. %p\n", i+1, leaf->data[i]); - } - template - void BPlusTree::PrintLeaves(void) - { - ForEachLeaf(PrintLeaf); - } - - template - void BPlusTree::ValidateTreeRecursive(Page *cur) - { - RakAssert(cur==root || cur->size>=order/2); - - if (cur->children[0]->isLeaf) - { - RakAssert(cur->children[0]->keys[0] < cur->keys[0]); - for (int i=0; i < cur->size; i++) - { - RakAssert(cur->children[i+1]->keys[0]==cur->keys[i]); - } - } - else - { - for (int i=0; i < cur->size+1; i++) - ValidateTreeRecursive(cur->children[i]); - } - } - - template - void BPlusTree::PrintGraph(void) - { - DataStructures::Queue *> queue; - queue.Push(root,_FILE_AND_LINE_); - queue.Push(0,_FILE_AND_LINE_); - DataStructures::Page *ptr; - int i,j; - if (root) - { - RAKNET_DEBUG_PRINTF("%p(", root); - for (i=0; i < root->size; i++) - { - RAKNET_DEBUG_PRINTF("%i ", root->keys[i]); - } - RAKNET_DEBUG_PRINTF(") "); - RAKNET_DEBUG_PRINTF("\n"); - } - while (queue.Size()) - { - ptr=queue.Pop(); - if (ptr==0) - RAKNET_DEBUG_PRINTF("\n"); - else if (ptr->isLeaf==false) - { - for (i=0; i < ptr->size+1; i++) - { - RAKNET_DEBUG_PRINTF("%p(", ptr->children[i]); - //RAKNET_DEBUG_PRINTF("(", ptr->children[i]); - for (j=0; j < ptr->children[i]->size; j++) - RAKNET_DEBUG_PRINTF("%i ", ptr->children[i]->keys[j]); - RAKNET_DEBUG_PRINTF(") "); - queue.Push(ptr->children[i],_FILE_AND_LINE_); - } - queue.Push(0,_FILE_AND_LINE_); - RAKNET_DEBUG_PRINTF(" -- "); - } - } - RAKNET_DEBUG_PRINTF("\n"); - } -} -#ifdef _MSC_VER -#pragma warning( pop ) -#endif - -#endif - -// Code to test this hellish data structure. -/* -#include "mafianet/ds_bplus_tree.h" -#include - -// Handle underflow on root. If there is only one item left then I can go downwards. -// Make sure I keep the leftmost pointer valid by traversing it -// When I free a leaf, be sure to adjust the pointers around it. - -#include "mafianet/rand.h" - -void main(void) -{ - DataStructures::BPlusTree btree; - DataStructures::List haveList, removedList; - int temp; - int i, j, index; - int testSize; - bool b; - - for (testSize=0; testSize < 514; testSize++) - { - RAKNET_DEBUG_PRINTF("TestSize=%i\n", testSize); - - for (i=0; i < testSize; i++) - haveList.Insert(i); - - for (i=0; i < testSize; i++) - { - index=i+randomMT()%(testSize-i); - temp=haveList[index]; - haveList[index]=haveList[i]; - haveList[i]=temp; - } - - for (i=0; i #include "mafianet/memory_override.h" #include "mafianet/export.h" #include "mafianet/string.h" -#define _TABLE_BPLUS_TREE_ORDER 16 #define _TABLE_MAX_COLUMN_NAME_LENGTH 64 /// The namespace DataStructures was only added to avoid compiler errors for commonly named data structures @@ -320,10 +319,9 @@ namespace DataStructures const DataStructures::List& GetColumns(void) const; /// \brief Direct access to make things easier. - const DataStructures::BPlusTree& GetRows(void) const; - - /// \brief Get the head of a linked list containing all the row data. - DataStructures::Page * GetListHead(void); + /// Rows are keyed by row id and iterate in ascending id order, as the B+ tree this replaced + /// did, so serialization and query order are unchanged. + const std::map& GetRows(void) const; /// \brief Get the first free row id. /// This could be made more efficient. @@ -338,9 +336,7 @@ namespace DataStructures void QueryRow(DataStructures::List &inclusionFilterColumnIndices, DataStructures::List &columnIndicesToReturn, unsigned key, Table::Row* row, FilterQuery *inclusionFilters, Table *result); - // 16 is arbitrary and is the order of the BPlus tree. Higher orders are better for searching while lower orders are better for - // Insertions and deletions. - DataStructures::BPlusTree rows; + std::map rows; // Columns in the table. DataStructures::List columns; diff --git a/Source/include/mafianet/reliability_layer.h b/Source/include/mafianet/reliability_layer.h index 6ba63f15c..333d1e5ee 100644 --- a/Source/include/mafianet/reliability_layer.h +++ b/Source/include/mafianet/reliability_layer.h @@ -34,7 +34,6 @@ #include "mafianet/dr_sha1.h" #include "mafianet/ds_ordered_list.h" #include "mafianet/ds_range_list.h" -#include "mafianet/ds_bplus_tree.h" #include "mafianet/ds_memory_pool.h" #include "mafianet/defines.h" #include "mafianet/ds_heap.h" diff --git a/Source/src/ds_table.cpp b/Source/src/ds_table.cpp index e3c489d08..2bda83942 100644 --- a/Source/src/ds_table.cpp +++ b/Source/src/ds_table.cpp @@ -16,6 +16,7 @@ #include "mafianet/ds_table.h" #include "mafianet/ds_ordered_list.h" #include +#include // std::advance, for GetRowByIndex #include "mafianet/assert.h" #include "mafianet/assert.h" #include "mafianet/itoa.h" @@ -297,7 +298,8 @@ unsigned Table::AddColumn(const char columnName[_TABLE_MAX_COLUMN_NAME_LENGTH], columns.Insert(Table::ColumnDescriptor(columnName, columnType), _FILE_AND_LINE_); // Extend the rows by one - rows.ForEachData(ExtendRows); + for (std::map::iterator it = rows.begin(); it != rows.end(); ++it) + ExtendRows(it->second, 0); return columns.Size()-1; } @@ -309,17 +311,10 @@ void Table::RemoveColumn(unsigned columnIndex) columns.RemoveAtIndex(columnIndex); // Remove this index from each row. - int i; - DataStructures::Page *cur = rows.GetListHead(); - while (cur) + for (std::map::iterator it = rows.begin(); it != rows.end(); ++it) { - for (i=0; i < cur->size; i++) - { - MafiaNet::OP_DELETE(cur->data[i]->cells[columnIndex], _FILE_AND_LINE_); - cur->data[i]->cells.RemoveAtIndex(columnIndex); - } - - cur=cur->next; + MafiaNet::OP_DELETE(it->second->cells[columnIndex], _FILE_AND_LINE_); + it->second->cells.RemoveAtIndex(columnIndex); } } unsigned Table::ColumnIndex(const char *columnName) const @@ -354,13 +349,13 @@ unsigned Table::GetColumnCount(void) const } unsigned Table::GetRowCount(void) const { - return rows.Size(); + return (unsigned) rows.size(); } Table::Row* Table::AddRow(unsigned rowId) { Row *newRow; newRow = MafiaNet::OP_NEW( _FILE_AND_LINE_ ); - if (rows.Insert(rowId, newRow)==false) + if (rows.insert(std::make_pair(rowId, newRow)).second==false) { MafiaNet::OP_DELETE(newRow, _FILE_AND_LINE_); return 0; // Already exists @@ -386,7 +381,7 @@ Table::Row* Table::AddRow(unsigned rowId, DataStructures::List &initialCel else newRow->cells.Insert(MafiaNet::OP_NEW(_FILE_AND_LINE_), _FILE_AND_LINE_ ); } - rows.Insert(rowId, newRow); + rows[rowId] = newRow; return newRow; } Table::Row* Table::AddRow(unsigned rowId, DataStructures::List &initialCellValues, bool copyCells) @@ -409,7 +404,7 @@ Table::Row* Table::AddRow(unsigned rowId, DataStructures::List &initialCe else newRow->cells.Insert(MafiaNet::OP_NEW(_FILE_AND_LINE_), _FILE_AND_LINE_); } - rows.Insert(rowId, newRow); + rows[rowId] = newRow; return newRow; } Table::Row* Table::AddRowColumns(unsigned rowId, Row *row, DataStructures::List columnIndices) @@ -432,14 +427,16 @@ Table::Row* Table::AddRowColumns(unsigned rowId, Row *row, DataStructures::List< newRow->cells.Insert(MafiaNet::OP_NEW(_FILE_AND_LINE_), _FILE_AND_LINE_); } } - rows.Insert(rowId, newRow); + rows[rowId] = newRow; return newRow; } bool Table::RemoveRow(unsigned rowId) { - Row *out; - if (rows.Delete(rowId, out)) + std::map::iterator it = rows.find(rowId); + if (it != rows.end()) { + Row *out = it->second; + rows.erase(it); DeleteRow(out); return true; } @@ -447,16 +444,9 @@ bool Table::RemoveRow(unsigned rowId) } void Table::RemoveRows(Table *tableContainingRowIDs) { - unsigned i; - DataStructures::Page *cur = tableContainingRowIDs->GetRows().GetListHead(); - while (cur) - { - for (i=0; i < (unsigned)cur->size; i++) - { - rows.Delete(cur->keys[i]); - } - cur=cur->next; - } + const std::map &rowIds = tableContainingRowIDs->GetRows(); + for (std::map::const_iterator it = rowIds.begin(); it != rowIds.end(); ++it) + rows.erase(it->first); return; } bool Table::UpdateCell(unsigned rowId, unsigned columnIndex, int value) @@ -587,29 +577,21 @@ Table::FilterQuery::FilterQuery(unsigned column, Cell *cell, FilterQueryType op) } Table::Row* Table::GetRowByID(unsigned rowId) const { - Row *row; - if (rows.Get(rowId, row)) - return row; + std::map::const_iterator it = rows.find(rowId); + if (it != rows.end()) + return it->second; return 0; } Table::Row* Table::GetRowByIndex(unsigned rowIndex, unsigned *key) const { - DataStructures::Page *cur = rows.GetListHead(); - while (cur) - { - if (rowIndex < (unsigned)cur->size) - { - if (key) - *key=cur->keys[rowIndex]; - return cur->data[rowIndex]; - } - if (rowIndex <= (unsigned)cur->size) - rowIndex-=cur->size; - else - return 0; - cur=cur->next; - } + if (rowIndex >= rows.size()) + return 0; + std::map::const_iterator it = rows.begin(); + std::advance(it, rowIndex); + if (key) + *key = it->first; + return it->second; return 0; } @@ -661,25 +643,18 @@ void Table::QueryTable(unsigned *columnIndicesSubset, unsigned numColumnSubset, if (rowIds==0 || numRowIDs==0) { // All rows - DataStructures::Page *cur = rows.GetListHead(); - while (cur) - { - for (i=0; i < (unsigned)cur->size; i++) - { - QueryRow(inclusionFilterColumnIndices, columnIndicesToReturn, cur->keys[i], cur->data[i], inclusionFilters, result); - } - cur=cur->next; - } + for (std::map::const_iterator it = rows.begin(); it != rows.end(); ++it) + QueryRow(inclusionFilterColumnIndices, columnIndicesToReturn, it->first, it->second, inclusionFilters, result); } else { // Specific rows - Row *row; for (i=0; i < numRowIDs; i++) { - if (rows.Get(rowIds[i], row)) + std::map::const_iterator it = rows.find(rowIds[i]); + if (it != rows.end()) { - QueryRow(inclusionFilterColumnIndices, columnIndicesToReturn, rowIds[i], row, inclusionFilters, result); + QueryRow(inclusionFilterColumnIndices, columnIndicesToReturn, rowIds[i], it->second, inclusionFilters, result); } } } @@ -923,32 +898,21 @@ void Table::SortTable(Table::SortQuery *sortQueries, unsigned numSortQueries, Ta columnIndices.Insert((unsigned)-1, _FILE_AND_LINE_); // Means don't check this column } - DataStructures::Page *cur; - cur = rows.GetListHead(); + std::map::const_iterator it; if (anyValid==false) { outLength=0; - while (cur) - { - for (i=0; i < (unsigned)cur->size; i++) - { - out[(outLength)++]=cur->data[i]; - } - cur=cur->next; - } + for (it = rows.begin(); it != rows.end(); ++it) + out[(outLength)++]=it->second; return; } // Start adding to ordered list. DataStructures::OrderedList orderedList; - while (cur) + for (it = rows.begin(); it != rows.end(); ++it) { - for (i=0; i < (unsigned)cur->size; i++) - { - RakAssert(cur->data[i]); - orderedList.Insert(cur->data[i],cur->data[i], true, _FILE_AND_LINE_); - } - cur=cur->next; + RakAssert(it->second); + orderedList.Insert(it->second,it->second, true, _FILE_AND_LINE_); } outLength=0; @@ -1072,47 +1036,37 @@ void Table::PrintRow(char *out, int outLength, char columnDelineator, bool print void Table::Clear(void) { - rows.ForEachData(FreeRow); - rows.Clear(); + for (std::map::iterator it = rows.begin(); it != rows.end(); ++it) + FreeRow(it->second, 0); + rows.clear(); columns.Clear(true, _FILE_AND_LINE_); } const List& Table::GetColumns(void) const { return columns; } -const DataStructures::BPlusTree& Table::GetRows(void) const +const std::map& Table::GetRows(void) const { return rows; } -DataStructures::Page * Table::GetListHead(void) -{ - return rows.GetListHead(); -} unsigned Table::GetAvailableRowId(void) const { bool setKey=false; unsigned key=0; int i; - DataStructures::Page *cur = rows.GetListHead(); - - while (cur) + for (std::map::const_iterator it = rows.begin(); it != rows.end(); ++it) { - for (i=0; i < cur->size; i++) + if (setKey==false) { - if (setKey==false) - { - key=cur->keys[i]+1; - setKey=true; - } - else - { - if (key!=cur->keys[i]) - return key; - key++; - } + key=it->first+1; + setKey=true; + } + else + { + if (key!=it->first) + return key; + key++; } - - cur=cur->next; } return key; } @@ -1133,16 +1087,9 @@ Table& Table::operator = ( const Table& input ) for (i=0; i < input.GetColumnCount(); i++) AddColumn(input.ColumnName(i), input.GetColumnType(i)); - DataStructures::Page *cur = input.GetRows().GetListHead(); - while (cur) - { - for (i=0; i < (unsigned int) cur->size; i++) - { - AddRow(cur->keys[i], cur->data[i]->cells, false); - } - - cur=cur->next; - } + const std::map &inputRows = input.GetRows(); + for (std::map::const_iterator it = inputRows.begin(); it != inputRows.end(); ++it) + AddRow(it->first, it->second->cells, false); return *this; } diff --git a/Source/src/table_serializer.cpp b/Source/src/table_serializer.cpp index 6f679cfdd..5e75a7599 100644 --- a/Source/src/table_serializer.cpp +++ b/Source/src/table_serializer.cpp @@ -23,19 +23,14 @@ using namespace MafiaNet; void TableSerializer::SerializeTable(DataStructures::Table *in, MafiaNet::BitStream *out) { - DataStructures::Page *cur = in->GetRows().GetListHead(); + const std::map &rows = in->GetRows(); const DataStructures::List &columns=in->GetColumns(); SerializeColumns(in, out); - out->Write((unsigned)in->GetRows().Size()); - unsigned rowIndex; - while (cur) - { - for (rowIndex=0; rowIndex < (unsigned)cur->size; rowIndex++) - { - SerializeRow(cur->data[rowIndex], cur->keys[rowIndex], columns, out); - } - cur=cur->next; - } + out->Write((unsigned)rows.size()); + // Ascending key order, exactly as the B+ tree leaf walk this replaced, so the wire order of rows + // is unchanged and an old peer deserializes a new peer's table identically. + for (std::map::const_iterator it = rows.begin(); it != rows.end(); ++it) + SerializeRow(it->second, it->first, columns, out); } void TableSerializer::SerializeColumns(DataStructures::Table *in, MafiaNet::BitStream *out) { diff --git a/Tests/Unit/TableTests.cpp b/Tests/Unit/TableTests.cpp new file mode 100644 index 000000000..fe51ebc03 --- /dev/null +++ b/Tests/Unit/TableTests.cpp @@ -0,0 +1,433 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +#include + +#include +#include + +#include "mafianet/ds_table.h" +#include "mafianet/table_serializer.h" +#include "mafianet/bit_stream.h" +#include "mafianet/string_compressor.h" + +using namespace MafiaNet; + +/* +Description: +Behavioural tests for DataStructures::Table, written when its row storage moved from the homegrown +B+ tree to std::map (std migration stage 1). + +The table had no test coverage at all, so the migration was initially verified only by the library +compiling and the rest of the suite passing -- neither of which touches a single row. These pin the +observable behaviour the B+ tree provided, and in particular the two properties a keyed container +swap can silently break: + +- rows iterate in ascending row-id order (GetRows, GetRowByIndex, SortTable, serialization). A + container with different ordering would still compile and still pass every other test, while + changing the order rows go on the wire. +- AddRow rejects a duplicate id rather than overwriting, which the B+ tree signalled through the + return value of its Insert. + +Success conditions: each operation behaves as documented, and row order is ascending by id +everywhere it is observable. +*/ + +namespace +{ + using DataStructures::Table; + + // A table with one numeric column and the given row ids, inserted in the order supplied so the + // tests can prove the container re-orders them rather than preserving insertion order. + void BuildTable(Table &table, const std::vector &rowIds) + { + table.AddColumn("value", Table::NUMERIC); + for (size_t i = 0; i < rowIds.size(); ++i) + { + Table::Row *row = table.AddRow(rowIds[i]); + ASSERT_NE(row, nullptr); + row->UpdateCell(0, (double)(rowIds[i] * 10)); + } + } + + std::vector KeysInOrder(const Table &table) + { + std::vector keys; + const std::map &rows = table.GetRows(); + for (std::map::const_iterator it = rows.begin(); it != rows.end(); ++it) + keys.push_back(it->first); + return keys; + } +} // namespace + +TEST(Table, RowsIterateInAscendingIdOrderRegardlessOfInsertionOrder) +{ + Table table; + std::vector inserted; + inserted.push_back(50); + inserted.push_back(10); + inserted.push_back(30); + inserted.push_back(20); + BuildTable(table, inserted); + + const std::vector keys = KeysInOrder(table); + ASSERT_EQ(keys.size(), (size_t)4); + EXPECT_EQ(keys[0], 10u); + EXPECT_EQ(keys[1], 20u); + EXPECT_EQ(keys[2], 30u); + EXPECT_EQ(keys[3], 50u); +} + +TEST(Table, AddRowRejectsADuplicateIdWithoutReplacingTheRow) +{ + Table table; + std::vector ids; + ids.push_back(7); + BuildTable(table, ids); + + Table::Row *original = table.GetRowByID(7); + ASSERT_NE(original, nullptr); + + EXPECT_EQ(table.AddRow(7), nullptr) << "a duplicate row id must be refused, not overwrite"; + EXPECT_EQ(table.GetRowByID(7), original) << "the existing row must survive a refused insert"; + EXPECT_EQ(table.GetRowCount(), 1u); +} + +TEST(Table, GetRowByIdFindsPresentRowsAndReportsMissingOnes) +{ + Table table; + std::vector ids; + ids.push_back(3); + ids.push_back(9); + BuildTable(table, ids); + + ASSERT_NE(table.GetRowByID(3), nullptr); + ASSERT_NE(table.GetRowByID(9), nullptr); + EXPECT_EQ(table.GetRowByID(4), nullptr); + EXPECT_EQ(table.GetRowByID(0), nullptr); +} + +TEST(Table, GetRowByIndexWalksRowsInKeyOrderAndReportsTheKey) +{ + Table table; + std::vector ids; + ids.push_back(40); + ids.push_back(10); + ids.push_back(25); + BuildTable(table, ids); + + unsigned key = 0; + Table::Row *row = table.GetRowByIndex(0, &key); + ASSERT_NE(row, nullptr); + EXPECT_EQ(key, 10u); + + row = table.GetRowByIndex(1, &key); + ASSERT_NE(row, nullptr); + EXPECT_EQ(key, 25u); + + row = table.GetRowByIndex(2, &key); + ASSERT_NE(row, nullptr); + EXPECT_EQ(key, 40u); + + // Past the end, and with no key requested. + EXPECT_EQ(table.GetRowByIndex(3, &key), nullptr); + EXPECT_NE(table.GetRowByIndex(0, 0), nullptr); +} + +TEST(Table, RemoveRowErasesOnlyTheNamedRow) +{ + Table table; + std::vector ids; + ids.push_back(1); + ids.push_back(2); + ids.push_back(3); + BuildTable(table, ids); + + EXPECT_TRUE(table.RemoveRow(2)); + EXPECT_EQ(table.GetRowCount(), 2u); + EXPECT_EQ(table.GetRowByID(2), nullptr); + EXPECT_NE(table.GetRowByID(1), nullptr); + EXPECT_NE(table.GetRowByID(3), nullptr); + + EXPECT_FALSE(table.RemoveRow(2)) << "removing an absent row must report failure"; + EXPECT_FALSE(table.RemoveRow(99)); + EXPECT_EQ(table.GetRowCount(), 2u); +} + +TEST(Table, RemoveRowsDeletesEveryIdHeldByTheOtherTable) +{ + Table table; + std::vector ids; + ids.push_back(1); + ids.push_back(2); + ids.push_back(3); + ids.push_back(4); + BuildTable(table, ids); + + // The ids to drop are the KEYS of the second table, not its contents. + Table idHolder; + std::vector toRemove; + toRemove.push_back(2); + toRemove.push_back(4); + BuildTable(idHolder, toRemove); + + table.RemoveRows(&idHolder); + + EXPECT_EQ(table.GetRowCount(), 2u); + EXPECT_NE(table.GetRowByID(1), nullptr); + EXPECT_EQ(table.GetRowByID(2), nullptr); + EXPECT_NE(table.GetRowByID(3), nullptr); + EXPECT_EQ(table.GetRowByID(4), nullptr); +} + +TEST(Table, AddColumnExtendsEveryExistingRow) +{ + Table table; + std::vector ids; + ids.push_back(1); + ids.push_back(2); + BuildTable(table, ids); + + ASSERT_EQ(table.GetColumnCount(), 1u); + ASSERT_EQ(table.GetRowByID(1)->cells.Size(), 1u); + + table.AddColumn("extra", Table::STRING); + + ASSERT_EQ(table.GetColumnCount(), 2u); + EXPECT_EQ(table.GetRowByID(1)->cells.Size(), 2u) << "an existing row did not gain the new cell"; + EXPECT_EQ(table.GetRowByID(2)->cells.Size(), 2u); +} + +TEST(Table, RemoveColumnDropsTheCellFromEveryRow) +{ + Table table; + std::vector ids; + ids.push_back(1); + ids.push_back(2); + BuildTable(table, ids); + table.AddColumn("extra", Table::STRING); + ASSERT_EQ(table.GetRowByID(1)->cells.Size(), 2u); + + table.RemoveColumn(1); + + EXPECT_EQ(table.GetColumnCount(), 1u); + EXPECT_EQ(table.GetRowByID(1)->cells.Size(), 1u); + EXPECT_EQ(table.GetRowByID(2)->cells.Size(), 1u); +} + +TEST(Table, GetAvailableRowIdReturnsTheLowestUnusedId) +{ + Table empty; + empty.AddColumn("value", Table::NUMERIC); + EXPECT_EQ(empty.GetAvailableRowId(), 0u); + + // Contiguous from 0: the next free id is one past the end. + Table contiguous; + std::vector ids; + ids.push_back(0); + ids.push_back(1); + ids.push_back(2); + BuildTable(contiguous, ids); + EXPECT_EQ(contiguous.GetAvailableRowId(), 3u); + + // With a gap, the gap is returned. Relies on ascending iteration order. + Table gapped; + std::vector gappedIds; + gappedIds.push_back(0); + gappedIds.push_back(1); + gappedIds.push_back(3); + BuildTable(gapped, gappedIds); + EXPECT_EQ(gapped.GetAvailableRowId(), 2u); +} + +TEST(Table, ClearDropsEveryRowAndColumn) +{ + Table table; + std::vector ids; + ids.push_back(1); + ids.push_back(2); + BuildTable(table, ids); + + table.Clear(); + + EXPECT_EQ(table.GetRowCount(), 0u); + EXPECT_TRUE(table.GetRows().empty()); + EXPECT_EQ(table.GetRowByID(1), nullptr); +} + +TEST(Table, AssignmentCopiesRowsInKeyOrder) +{ + Table source; + std::vector ids; + ids.push_back(30); + ids.push_back(10); + ids.push_back(20); + BuildTable(source, ids); + + Table copy; + copy = source; + + EXPECT_EQ(copy.GetRowCount(), 3u); + const std::vector keys = KeysInOrder(copy); + ASSERT_EQ(keys.size(), (size_t)3); + EXPECT_EQ(keys[0], 10u); + EXPECT_EQ(keys[1], 20u); + EXPECT_EQ(keys[2], 30u); + + // Deep copy: the rows are the copy's own. + ASSERT_NE(copy.GetRowByID(10), nullptr); + EXPECT_NE(copy.GetRowByID(10), source.GetRowByID(10)); +} + +TEST(Table, QueryTableWithNoRowFilterReturnsEveryRowInKeyOrder) +{ + Table table; + std::vector ids; + ids.push_back(5); + ids.push_back(1); + ids.push_back(3); + BuildTable(table, ids); + + Table result; + table.QueryTable(0, 0, 0, 0, 0, 0, &result); + + EXPECT_EQ(result.GetRowCount(), 3u); + const std::vector keys = KeysInOrder(result); + ASSERT_EQ(keys.size(), (size_t)3); + EXPECT_EQ(keys[0], 1u); + EXPECT_EQ(keys[1], 3u); + EXPECT_EQ(keys[2], 5u); +} + +TEST(Table, QueryTableWithSpecificRowIdsReturnsOnlyThose) +{ + Table table; + std::vector ids; + ids.push_back(1); + ids.push_back(2); + ids.push_back(3); + ids.push_back(4); + BuildTable(table, ids); + + unsigned wanted[2] = {3, 1}; + Table result; + table.QueryTable(0, 0, 0, 0, wanted, 2, &result); + + EXPECT_EQ(result.GetRowCount(), 2u); + EXPECT_NE(result.GetRowByID(1), nullptr); + EXPECT_NE(result.GetRowByID(3), nullptr); + EXPECT_EQ(result.GetRowByID(2), nullptr); + + // An id that is not present is skipped rather than producing an empty row. + unsigned missing[2] = {2, 99}; + Table result2; + table.QueryTable(0, 0, 0, 0, missing, 2, &result2); + EXPECT_EQ(result2.GetRowCount(), 1u); + EXPECT_NE(result2.GetRowByID(2), nullptr); +} + +TEST(Table, SortTableWithNoValidQueryReturnsRowsInKeyOrder) +{ + Table table; + std::vector ids; + ids.push_back(30); + ids.push_back(10); + ids.push_back(20); + BuildTable(table, ids); + + // A sort query naming a column that does not exist leaves nothing valid to sort on, which is the + // branch that simply emits the rows as stored. + Table::SortQuery query; + query.columnIndex = 99; + query.operation = Table::QS_INCREASING_ORDER; + + std::vector out(3, (Table::Row*)0); + table.SortTable(&query, 1, &out[0]); + + ASSERT_NE(out[0], nullptr); + EXPECT_EQ(out[0], table.GetRowByID(10)); + EXPECT_EQ(out[1], table.GetRowByID(20)); + EXPECT_EQ(out[2], table.GetRowByID(30)); +} + +TEST(Table, SortTableOrdersByTheRequestedColumn) +{ + Table table; + table.AddColumn("value", Table::NUMERIC); + // Row ids ascending, values descending, so sorting by value must reverse the stored order and + // cannot pass by accident. + Table::Row *first = table.AddRow(1); + ASSERT_NE(first, nullptr); + first->UpdateCell(0, 300.0); + Table::Row *second = table.AddRow(2); + ASSERT_NE(second, nullptr); + second->UpdateCell(0, 200.0); + Table::Row *third = table.AddRow(3); + ASSERT_NE(third, nullptr); + third->UpdateCell(0, 100.0); + + Table::SortQuery query; + query.columnIndex = 0; + query.operation = Table::QS_INCREASING_ORDER; + + std::vector out(3, (Table::Row*)0); + table.SortTable(&query, 1, &out[0]); + + ASSERT_NE(out[0], nullptr); + EXPECT_EQ(out[0], third); + EXPECT_EQ(out[1], second); + EXPECT_EQ(out[2], first); +} + +namespace +{ + // TableSerializer encodes column names through StringCompressor, whose Instance() dereferences a + // null singleton until someone takes a reference -- normally the RakPeer constructor. No peer + // exists in a hermetic unit test, so without this the serializer segfaults inside + // SerializeColumns. Pre-existing behaviour, documented on StringCompressor::Instance(). + class TableSerialization : public ::testing::Test + { + public: + void SetUp() override { StringCompressor::AddReference(); } + void TearDown() override { StringCompressor::RemoveReference(); } + }; +} // namespace + +// The serialized row order is observable by a remote peer, so it is the property most worth pinning +// across a container swap. +TEST_F(TableSerialization, RoundTripPreservesRowIdsAndOrder) +{ + Table source; + std::vector ids; + ids.push_back(40); + ids.push_back(10); + ids.push_back(25); + BuildTable(source, ids); + + MafiaNet::BitStream bs; + TableSerializer::SerializeTable(&source, &bs); + + Table restored; + ASSERT_TRUE(TableSerializer::DeserializeTable(&bs, &restored)); + + EXPECT_EQ(restored.GetRowCount(), source.GetRowCount()); + const std::vector keys = KeysInOrder(restored); + ASSERT_EQ(keys.size(), (size_t)3); + EXPECT_EQ(keys[0], 10u); + EXPECT_EQ(keys[1], 25u); + EXPECT_EQ(keys[2], 40u); + + // And the cell values travelled with their own rows. + for (size_t i = 0; i < keys.size(); ++i) + { + Table::Row *row = restored.GetRowByID(keys[i]); + ASSERT_NE(row, nullptr); + int value = 0; + row->cells[0]->Get(&value); + EXPECT_EQ(value, (int)(keys[i] * 10)) << "row " << keys[i] << " lost its value"; + } +} From d1b1653a20623a672b2806a9f4345f4699b0e811 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Thu, 8 Oct 2026 18:35:18 +0200 Subject: [PATCH 3/8] refactor(ds): build the Huffman tree with std::list instead of LinkedList Third increment of the stage 1 container migration. The issue lists LinkedList's consumers as reliability_layer.{h,cpp}. They are not consumers: the orderingList member is commented out in the header and every use in reliability_layer.cpp (lines 583, 1230, 3663) sits inside a /* */ block. The only live consumer is the Huffman tree, so again nothing performance-sensitive is touched here -- and the tree is built once per StringCompressor, not per packet. DataStructures::LinkedList is a circular cursor list, so the insertion sort was expressed as Beginning()/Peek()/operator++/Insert() with a counter to detect running off the end. With std::list it is a find-then-insert, and the separate "add to the end" branch disappears because insert() before end() already does that. The placement rule is preserved exactly: advance past every node strictly lighter than the new one, then insert, so a new node precedes existing nodes of equal weight. That tie-breaking decides the tree's shape, which decides the bit pattern each character encodes to, so getting it wrong would change what goes on the wire while still round-tripping perfectly in process. Tests/Unit/HuffmanEncodingTests.cpp pins the encoding with golden bytes captured from the pre-migration build, and the migrated code reproduces them byte for byte across four inputs. Checked that the golden values bite: a one-character mutant flipping the comparison to <= fails three of them, while the round-trip case still passes -- which is why the tests are golden bytes rather than an encode/decode loop. Multilist needs no change. Every use under Source/ is commented out (udp_proxy_client.h, udp_proxy_coordinator.{h,cpp}), so the issue's acceptance criterion is already met there, but ds_multilist.h cannot be deleted: DependentExtensions/Lobby2/Steam and DependentExtensions/SQLite3Plugin use it for real. Verified on macOS arm64: full ctest 306/306, samples build including Lobby2 clean. --- .../mafianet/ds_huffman_encoding_tree.h | 4 +- Source/src/ds_huffman_encoding_tree.cpp | 47 ++----- Tests/Unit/HuffmanEncodingTests.cpp | 115 ++++++++++++++++++ 3 files changed, 129 insertions(+), 37 deletions(-) create mode 100644 Tests/Unit/HuffmanEncodingTests.cpp diff --git a/Source/include/mafianet/ds_huffman_encoding_tree.h b/Source/include/mafianet/ds_huffman_encoding_tree.h index 1d8d4bd8e..b7cfdc151 100644 --- a/Source/include/mafianet/ds_huffman_encoding_tree.h +++ b/Source/include/mafianet/ds_huffman_encoding_tree.h @@ -25,7 +25,7 @@ #include "mafianet/ds_huffman_encoding_tree_node.h" #include "mafianet/bit_stream.h" #include "mafianet/export.h" -#include "mafianet/ds_linked_list.h" +#include namespace MafiaNet { @@ -71,7 +71,7 @@ class RAK_DLL_EXPORT HuffmanEncodingTree CharacterEncoding encodingTable[ 256 ]; - void InsertNodeIntoSortedList( HuffmanEncodingTreeNode * node, DataStructures::LinkedList *huffmanEncodingTreeNodeList ) const; + void InsertNodeIntoSortedList( HuffmanEncodingTreeNode * node, std::list *huffmanEncodingTreeNodeList ) const; }; } // namespace MafiaNet diff --git a/Source/src/ds_huffman_encoding_tree.cpp b/Source/src/ds_huffman_encoding_tree.cpp index 78528fe86..9645c13b6 100644 --- a/Source/src/ds_huffman_encoding_tree.cpp +++ b/Source/src/ds_huffman_encoding_tree.cpp @@ -72,7 +72,7 @@ void HuffmanEncodingTree::GenerateFromFrequencyTable( unsigned int frequencyTabl HuffmanEncodingTreeNode * node; HuffmanEncodingTreeNode *leafList[ 256 ]; // Keep a copy of the pointers to all the leaves so we can generate the encryption table bottom-up, which is easier // 1. Make 256 trees each with a weight equal to the frequency of the corresponding character - DataStructures::LinkedList huffmanEncodingTreeNodeList; + std::list huffmanEncodingTreeNodeList; FreeMemory(); @@ -97,10 +97,11 @@ void HuffmanEncodingTree::GenerateFromFrequencyTable( unsigned int frequencyTabl // children of a new node, where the new node has the weight the sum of the weight of the left and right child nodes. for(;;) { - huffmanEncodingTreeNodeList.Beginning(); HuffmanEncodingTreeNode *lesser, *greater; - lesser = huffmanEncodingTreeNodeList.Pop(); - greater = huffmanEncodingTreeNodeList.Pop(); + lesser = huffmanEncodingTreeNodeList.front(); + huffmanEncodingTreeNodeList.pop_front(); + greater = huffmanEncodingTreeNodeList.front(); + huffmanEncodingTreeNodeList.pop_front(); node = MafiaNet::OP_NEW( _FILE_AND_LINE_ ); node->left = lesser; node->right = greater; @@ -108,7 +109,7 @@ void HuffmanEncodingTree::GenerateFromFrequencyTable( unsigned int frequencyTabl lesser->parent = node; // This is done to make generating the encryption table easier greater->parent = node; // This is done to make generating the encryption table easier - if ( huffmanEncodingTreeNodeList.Size() == 0 ) + if ( huffmanEncodingTreeNodeList.empty() ) { // 3. Assign the one remaining node in the list to the root node. root = node; @@ -258,36 +259,12 @@ void HuffmanEncodingTree::DecodeArray( unsigned char *input, BitSize_t sizeInBit } // Insertion sort. Slow but easy to write in this case -void HuffmanEncodingTree::InsertNodeIntoSortedList( HuffmanEncodingTreeNode * node, DataStructures::LinkedList *huffmanEncodingTreeNodeList ) const +void HuffmanEncodingTree::InsertNodeIntoSortedList( HuffmanEncodingTreeNode * node, std::list *huffmanEncodingTreeNodeList ) const { - if ( huffmanEncodingTreeNodeList->Size() == 0 ) - { - huffmanEncodingTreeNodeList->Insert( node ); - return ; - } + std::list::iterator it = huffmanEncodingTreeNodeList->begin(); + while ( it != huffmanEncodingTreeNodeList->end() && (*it)->weight < node->weight ) + ++it; - huffmanEncodingTreeNodeList->Beginning(); - - unsigned counter = 0; - for(;;) - { - if ( huffmanEncodingTreeNodeList->Peek()->weight < node->weight ) - ++( *huffmanEncodingTreeNodeList ); - else - { - huffmanEncodingTreeNodeList->Insert( node ); - break; - } - - // Didn't find a spot in the middle - add to the end - if ( ++counter == huffmanEncodingTreeNodeList->Size() ) - { - huffmanEncodingTreeNodeList->End(); - - huffmanEncodingTreeNodeList->Add( node ) - - ; // Add to the end - break; - } - } + // insert() before end() is the "add to the end" case the cursor walk handled separately. + huffmanEncodingTreeNodeList->insert( it, node ); } diff --git a/Tests/Unit/HuffmanEncodingTests.cpp b/Tests/Unit/HuffmanEncodingTests.cpp new file mode 100644 index 000000000..5129136e1 --- /dev/null +++ b/Tests/Unit/HuffmanEncodingTests.cpp @@ -0,0 +1,115 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +#include + +#include +#include +#include + +#include "mafianet/string_compressor.h" +#include "mafianet/bit_stream.h" + +using namespace MafiaNet; + +/* +Description: +Golden-output tests for the Huffman tree behind StringCompressor, written when its node list moved +from DataStructures::LinkedList to std::list (std migration stage 1). + +Why golden bytes rather than a round-trip: the tree is built by an insertion sort over node weights, +and the ORDER equal-weight nodes take decides the tree's shape, which decides the bit pattern each +character encodes to. A replacement container that sorted stably the other way would still build a +valid tree, still round-trip perfectly in-process, and still pass any encode/decode test -- while +silently changing what goes on the wire, so a peer on the old build could no longer read a string +from a peer on the new one. + +The expectations below were captured from the pre-migration implementation and must not be edited to +match new output: if they fail, the encoding changed and the change is a wire break. + +Success conditions: encoded bits and bytes match the captured values exactly, and strings round-trip. +*/ + +namespace +{ + // StringCompressor::Instance() dereferences a null singleton until someone takes a reference -- + // normally the RakPeer constructor, and there is no peer in a hermetic unit test. + class HuffmanEncoding : public ::testing::Test + { + public: + void SetUp() override { StringCompressor::AddReference(); } + void TearDown() override { StringCompressor::RemoveReference(); } + + static std::string HexOf(const MafiaNet::BitStream &bs) + { + std::string out; + const unsigned char *d = bs.GetData(); + char buf[4]; + for (unsigned i = 0; i < bs.GetNumberOfBytesUsed(); ++i) + { + snprintf(buf, sizeof(buf), "%02x", d[i]); + out += buf; + } + return out; + } + + void ExpectEncoding(const char *input, unsigned expectedBits, const char *expectedHex) + { + MafiaNet::BitStream bs; + StringCompressor::Instance()->EncodeString(input, 512, &bs); + EXPECT_EQ((unsigned)bs.GetNumberOfBitsUsed(), expectedBits) << "bit length changed for \"" << input << "\""; + EXPECT_EQ(HexOf(bs), std::string(expectedHex)) << "encoded bytes changed for \"" << input << "\""; + } + }; +} // namespace + +TEST_F(HuffmanEncoding, ShortAsciiEncodesToTheCapturedBytes) +{ + ExpectEncoding("hello world", 89, "0000001c604a51e98cc9ca80"); +} + +TEST_F(HuffmanEncoding, MixedCaseEncodesToTheCapturedBytes) +{ + ExpectEncoding("MafiaNet", 81, "000000183eec7231885a80"); +} + +TEST_F(HuffmanEncoding, JsonPayloadEncodesToTheCapturedBytes) +{ + ExpectEncoding("{\"season\":\"winter\",\"map_file\":\"downtown.m2map\"}", 345, + "0000009c3e523e7b0db827ccc1f3a50ac65f3d6f9e8ca55b139241f3307cdd1495c524f68ade8ca3e6f94e80"); +} + +// Runs of one character exercise the equal-weight ties most heavily, so this is the case a +// tie-breaking change is most likely to move. +TEST_F(HuffmanEncoding, RepeatedRunsEncodeToTheCapturedBytes) +{ + ExpectEncoding("aaaaaaaaaabbbbbbbbbbcccccccccc", 193, + "0000005033333333331e79e79e79e79e7d1451451451451400"); +} + +TEST_F(HuffmanEncoding, StringsRoundTripThroughTheCompressor) +{ + const char *samples[] = { + "hello world", + "MafiaNet", + "", + "a", + "{\"build\":\"m2o|1.2.3\"}" + }; + for (int i = 0; i < 5; ++i) + { + MafiaNet::BitStream bs; + StringCompressor::Instance()->EncodeString(samples[i], 512, &bs); + bs.SetReadOffset(0); + + char decoded[512]; + memset(decoded, 0, sizeof(decoded)); + ASSERT_TRUE(StringCompressor::Instance()->DecodeString(decoded, 512, &bs)) + << "decode failed for \"" << samples[i] << "\""; + EXPECT_STREQ(decoded, samples[i]); + } +} From 13d1b9844b7f0c3ad1068f56afe9d53bd7cbd1cd Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:34:53 +0200 Subject: [PATCH 4/8] fix(rangelist): merge a value that sits directly above the previous range RangeList::Insert had a missed-merge bug on the acknowledgement path. The "insert here" branch compared the new value only against the range AT the insertion point, never against the range before it, so a value one above the previous range's maximum became a range of its own instead of extending that range. Minimal reproduction, found by brute-forcing every ordering of every subset of 0..5: Insert(0), Insert(3), Insert(1) produced [0,0] [1,1] [3,3] where [0,1] [3,3] was correct. Coverage was never wrong -- IsWithinRange and RangeSum both agreed with a reference model -- so no acknowledgement was ever lost. The cost is that the list carries entries it does not need, and Serialize() turns every entry into bytes in the ACK/NAK message: one byte of min-equals-max flag plus one or two sequence numbers each. It also pushes Size() nearer the RakAssert(ranges.Size() < (unsigned short)-1) bound. The pattern that triggers it is ordinary: acks 0..5 arrive, 8 arrives early leaving a gap, then 6 arrives and starts a new entry rather than extending [0,5]. Any reordering or loss followed by a late retransmit hits it, so ack lists fragment more than they should on exactly the networks where ack size matters most. No right-hand fuse is needed in the new path: the branch is only entered when index+1 < minIndex, so extending the previous range cannot make it adjacent to the next one. Brute force over the same space finds no remaining divergence. Tests/Unit/RangeListTests.cpp is new -- RangeList had none at all despite sitting on the ack path of every datagram and owning a wire format. 14 cases, written against the pre-fix implementation so they would catch exactly this: three of them failed before the fix. The load-bearing ones drive the real structure and a std::set model with the same fixed-seed inserts and compare coverage, range count and sum after every step, once with uniform random values in a small window and once with an ack-like arrival pattern of in-order acks, early arrivals and late retransmits. Serialize() output is pinned with golden bytes, since it is protocol rather than an implementation detail. The fix does not change them for that case. On the stage 1 migration: RangeList itself stays. The issue offers "std::vector of ranges, or keep a small purpose-built type", and the merge semantics are the reason the type exists. Its internal DataStructures::OrderedList is a stage 2 container (#60), so it is that stage's concern rather than this one. Verified on macOS arm64: full ctest 320/320. --- Source/include/mafianet/ds_range_list.h | 14 ++ Tests/Unit/RangeListTests.cpp | 316 ++++++++++++++++++++++++ 2 files changed, 330 insertions(+) create mode 100644 Tests/Unit/RangeListTests.cpp diff --git a/Source/include/mafianet/ds_range_list.h b/Source/include/mafianet/ds_range_list.h index 379c6aded..5eceb3e05 100644 --- a/Source/include/mafianet/ds_range_list.h +++ b/Source/include/mafianet/ds_range_list.h @@ -225,6 +225,20 @@ namespace DataStructures if (index < ranges[insertionIndex].minIndex-(range_type)1) { + // Too far below this range to touch it, but it may still sit directly above the range + // BEFORE it, in which case it extends that one instead of becoming a range of its own. + // Without this, acks arriving as 0, then 3, then 1 left [0,0] [1,1] [3,3] rather than + // [0,1] [3,3]: coverage was right but the list carried an entry it did not need, and every + // such entry is extra bytes in the ACK/NAK message built from it. + // + // No right-hand fuse is possible here: this branch means index+1 < minIndex, so extending + // the previous range cannot make it adjacent to the range at insertionIndex. + if (insertionIndex>0 && ranges[insertionIndex-1].maxIndex+(range_type)1==index) + { + ranges[insertionIndex-1].maxIndex++; + return; + } + // Insert here ranges.InsertAtIndex(RangeNode(index, index), insertionIndex, _FILE_AND_LINE_); return; diff --git a/Tests/Unit/RangeListTests.cpp b/Tests/Unit/RangeListTests.cpp new file mode 100644 index 000000000..4c6c37be4 --- /dev/null +++ b/Tests/Unit/RangeListTests.cpp @@ -0,0 +1,316 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +#include + +#include +#include +#include +#include + +#include "mafianet/ds_range_list.h" +#include "mafianet/bit_stream.h" + +using namespace MafiaNet; + +/* +Description: +Behavioural tests for DataStructures::RangeList, the structure the reliability layer keeps its +acknowledgements and NAKs in. + +Written before migrating its internal container (std migration stage 1). RangeList had no tests at +all, despite being on the ack path of every datagram and despite owning a wire format: Serialize() +is what goes out in an ACK/NAK message, so a change in how ranges merge or in how they are written +is a protocol change, not an implementation detail. + +Everything here is asserted through the public API rather than by inspecting the `ranges` member, so +the tests are indifferent to which container backs it and survive the migration unchanged. + +The load-bearing case is RangeListDifferential: it drives the real structure and a std::set model +with the same fixed-seed pseudo-random inserts and compares coverage, range count and sum after +every step. Merge logic has four join cases (new range, extend min, extend max, close a gap and +fuse two ranges) whose interactions are easy to get subtly wrong and hard to enumerate by hand. + +Success conditions: coverage and range counts match the model, and the serialized bytes match the +captured wire format. + +Failure conditions: any divergence from the model, or any change in serialized output. +*/ + +namespace +{ + typedef DataStructures::RangeList URangeList; + + // Number of maximal contiguous runs in the model, i.e. what Size() must report. + unsigned ExpectedRangeCount(const std::set &model) + { + unsigned count = 0; + bool havePrev = false; + unsigned prev = 0; + for (std::set::const_iterator it = model.begin(); it != model.end(); ++it) + { + if (!havePrev || *it != prev + 1) + ++count; + prev = *it; + havePrev = true; + } + return count; + } + + std::string HexOf(const MafiaNet::BitStream &bs) + { + std::string out; + const unsigned char *d = bs.GetData(); + char buf[4]; + for (unsigned i = 0; i < bs.GetNumberOfBytesUsed(); ++i) + { + snprintf(buf, sizeof(buf), "%02x", d[i]); + out += buf; + } + return out; + } +} // namespace + +TEST(RangeList, EmptyListCoversNothing) +{ + URangeList list; + EXPECT_EQ(list.Size(), 0u); + EXPECT_EQ(list.RangeSum(), 0u); + EXPECT_FALSE(list.IsWithinRange(0)); + EXPECT_FALSE(list.IsWithinRange(12345)); +} + +TEST(RangeList, SingleInsertCoversExactlyThatValue) +{ + URangeList list; + list.Insert(42); + EXPECT_EQ(list.Size(), 1u); + EXPECT_EQ(list.RangeSum(), 1u); + EXPECT_TRUE(list.IsWithinRange(42)); + EXPECT_FALSE(list.IsWithinRange(41)); + EXPECT_FALSE(list.IsWithinRange(43)); +} + +TEST(RangeList, ContiguousAscendingInsertsCollapseToOneRange) +{ + URangeList list; + for (unsigned i = 10; i <= 20; ++i) + list.Insert(i); + + EXPECT_EQ(list.Size(), 1u) << "contiguous values must merge into a single range"; + EXPECT_EQ(list.RangeSum(), 11u); + EXPECT_TRUE(list.IsWithinRange(10)); + EXPECT_TRUE(list.IsWithinRange(15)); + EXPECT_TRUE(list.IsWithinRange(20)); + EXPECT_FALSE(list.IsWithinRange(9)); + EXPECT_FALSE(list.IsWithinRange(21)); +} + +TEST(RangeList, ContiguousDescendingInsertsCollapseToOneRange) +{ + URangeList list; + for (unsigned i = 20; i >= 10; --i) + list.Insert(i); + + EXPECT_EQ(list.Size(), 1u) << "extending a range downwards must merge too"; + EXPECT_EQ(list.RangeSum(), 11u); + EXPECT_TRUE(list.IsWithinRange(10)); + EXPECT_TRUE(list.IsWithinRange(20)); + EXPECT_FALSE(list.IsWithinRange(9)); +} + +TEST(RangeList, AGapKeepsRangesSeparate) +{ + URangeList list; + list.Insert(1); + list.Insert(2); + list.Insert(5); + list.Insert(6); + + EXPECT_EQ(list.Size(), 2u); + EXPECT_EQ(list.RangeSum(), 4u); + EXPECT_FALSE(list.IsWithinRange(3)); + EXPECT_FALSE(list.IsWithinRange(4)); +} + +// The fuse case: a value that closes a one-wide gap must join the neighbours into one range. +TEST(RangeList, ClosingAOneWideGapFusesBothRanges) +{ + URangeList list; + list.Insert(1); + list.Insert(2); + list.Insert(4); + list.Insert(5); + ASSERT_EQ(list.Size(), 2u); + + list.Insert(3); + + EXPECT_EQ(list.Size(), 1u) << "the two neighbouring ranges must fuse, not stay adjacent"; + EXPECT_EQ(list.RangeSum(), 5u); + for (unsigned i = 1; i <= 5; ++i) + EXPECT_TRUE(list.IsWithinRange(i)) << i << " lost after the fuse"; +} + +TEST(RangeList, ReinsertingACoveredValueChangesNothing) +{ + URangeList list; + for (unsigned i = 10; i <= 20; ++i) + list.Insert(i); + const unsigned sizeBefore = list.Size(); + const unsigned sumBefore = list.RangeSum(); + + list.Insert(10); + list.Insert(15); + list.Insert(20); + + EXPECT_EQ(list.Size(), sizeBefore); + EXPECT_EQ(list.RangeSum(), sumBefore) << "a duplicate must not be counted twice"; +} + +TEST(RangeList, ClearDropsEverything) +{ + URangeList list; + list.Insert(1); + list.Insert(9); + ASSERT_EQ(list.Size(), 2u); + + list.Clear(); + + EXPECT_EQ(list.Size(), 0u); + EXPECT_EQ(list.RangeSum(), 0u); + EXPECT_FALSE(list.IsWithinRange(1)); +} + +TEST(RangeList, InsertionOrderDoesNotAffectTheResult) +{ + const unsigned values[] = {7, 3, 8, 1, 2, 9, 4}; + URangeList ascending, shuffled; + std::set model; + for (int i = 0; i < 7; ++i) + model.insert(values[i]); + for (std::set::iterator it = model.begin(); it != model.end(); ++it) + ascending.Insert(*it); + for (int i = 0; i < 7; ++i) + shuffled.Insert(values[i]); + + EXPECT_EQ(shuffled.Size(), ascending.Size()); + EXPECT_EQ(shuffled.RangeSum(), ascending.RangeSum()); + for (unsigned v = 0; v <= 12; ++v) + EXPECT_EQ(shuffled.IsWithinRange(v), ascending.IsWithinRange(v)) << "value " << v; +} + +TEST(RangeList, SerializeRoundTripPreservesCoverage) +{ + URangeList source; + source.Insert(1); + source.Insert(2); + source.Insert(3); + source.Insert(10); + source.Insert(20); + source.Insert(21); + + MafiaNet::BitStream bs; + source.Serialize(&bs, 2048, false); + bs.SetReadOffset(0); + + URangeList restored; + ASSERT_TRUE(restored.Deserialize(&bs)); + + EXPECT_EQ(restored.Size(), source.Size()); + EXPECT_EQ(restored.RangeSum(), source.RangeSum()); + for (unsigned v = 0; v <= 25; ++v) + EXPECT_EQ(restored.IsWithinRange(v), source.IsWithinRange(v)) << "value " << v; +} + +// Serialize() output is an ACK/NAK message body, so its bytes are protocol. Captured from the +// implementation in place before the container migration; a failure here is a wire break, not a +// reason to update the expectation. +TEST(RangeList, SerializedBytesMatchTheCapturedWireFormat) +{ + URangeList list; + list.Insert(1); + list.Insert(2); + list.Insert(3); // one multi-value range, written as min+max + list.Insert(10); // one single-value range, written as min only + + MafiaNet::BitStream bs; + list.Serialize(&bs, 2048, false); + + EXPECT_EQ(HexOf(bs), std::string("0002000000000100000003010000000a")); +} + +TEST(RangeList, ClearSerializedRemovesWhatWasWritten) +{ + URangeList list; + list.Insert(1); + list.Insert(5); + list.Insert(9); + ASSERT_EQ(list.Size(), 3u); + + MafiaNet::BitStream bs; + list.Serialize(&bs, 2048, true); + + EXPECT_EQ(list.Size(), 0u) << "clearSerialized must drop the ranges that were written out"; + EXPECT_EQ(list.RangeSum(), 0u); +} + +// Differential against a std::set model. Fixed seed, so a failure is reproducible. +TEST(RangeList, RangeListDifferential) +{ + URangeList list; + std::set model; + + unsigned seed = 0x5eed1234u; + for (int step = 0; step < 4000; ++step) + { + seed = seed * 1103515245u + 12345u; + // A small window makes neighbours collide constantly, which is what exercises the join cases. + const unsigned value = (seed >> 16) % 200u; + + list.Insert(value); + model.insert(value); + + ASSERT_EQ(list.RangeSum(), (unsigned)model.size()) + << "step " << step << ": covered count diverged after inserting " << value; + ASSERT_EQ(list.Size(), ExpectedRangeCount(model)) + << "step " << step << ": range count diverged after inserting " << value; + } + + // Full coverage sweep at the end, including just outside the window. + for (unsigned v = 0; v <= 205; ++v) + ASSERT_EQ(list.IsWithinRange(v), model.count(v) != 0) << "coverage diverged at " << v; +} + +// Same, but inserting mostly-ascending values the way acks actually arrive, with occasional gaps and +// reordering, so the common path gets the same scrutiny as the random one. +TEST(RangeList, AckLikeArrivalPatternMatchesTheModel) +{ + URangeList list; + std::set model; + + unsigned seed = 0xabcd0001u; + unsigned next = 0; + for (int step = 0; step < 3000; ++step) + { + seed = seed * 1103515245u + 12345u; + const unsigned roll = (seed >> 16) % 100u; + + unsigned value; + if (roll < 80) + value = next++; // in-order ack + else if (roll < 90) + value = next + 1 + (roll % 5); // arrives early, leaving a gap + else + value = (next > 10) ? next - 1 - (roll % 7) : 0; // late retransmit fills a gap + + list.Insert(value); + model.insert(value); + + ASSERT_EQ(list.RangeSum(), (unsigned)model.size()) << "step " << step << ", value " << value; + ASSERT_EQ(list.Size(), ExpectedRangeCount(model)) << "step " << step << ", value " << value; + } +} From 32c1a9571e1a4c6852d5ca53f8e477dcc83313c8 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Sat, 10 Oct 2026 10:43:25 +0200 Subject: [PATCH 5/8] test(rangelist): cover the wrapping uint24_t instantiation, and fix RangeSum for it The RangeList tests added with the merge fix all used unsigned, but the reliability layer instantiates RangeList, and that is uint24_t -- a type that wraps at 0xFFFFFF. None of the existing cases exercised the arithmetic the shipping instantiation actually performs, and Insert() compares against minIndex-(range_type)1 and maxIndex+(range_type)1, which wrap at the extremes. Adding those tests turned up that RangeSum() never compiled for uint24_t at all. "uint24_t + int" is ambiguous between the type's operator uint32_t() conversion and its own operator+, and member functions of a template are only instantiated when used -- RangeSum() has no caller anywhere in Source/, so the broken line never reached a compiler. Fixed by widening explicitly rather than deleting the accessor, since the tests use it as their observable. Subtracting before widening is safe: maxIndex >= minIndex within a range by construction. Seven new cases covering merges away from the boundary, a gap closing directly below the wrap point, the fixed missed-merge expressed in the shipping type, serialization round-tripping the extremes, and a differential against a model inside a window. One of them records current behaviour rather than asserting a design: ranges are ordered by raw value, so 0 does not fuse with 0xFFFFFF even though they are adjacent as sequence numbers. Coverage stays correct across the wrap, which is what the ack path depends on, and that is what the test pins -- so a future change to make the ordering wrap-aware is a deliberate one. Verified on macOS arm64: 20 RangeList cases pass, full ctest green. --- Source/include/mafianet/ds_range_list.h | 7 +- Tests/Unit/RangeListTests.cpp | 129 ++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 1 deletion(-) diff --git a/Source/include/mafianet/ds_range_list.h b/Source/include/mafianet/ds_range_list.h index 5eceb3e05..d76c90ff2 100644 --- a/Source/include/mafianet/ds_range_list.h +++ b/Source/include/mafianet/ds_range_list.h @@ -294,7 +294,12 @@ namespace DataStructures { unsigned sum = 0, i; for (i = 0; i < ranges.Size(); i++) { - sum += ranges[i].maxIndex-ranges[i].minIndex + 1; + // Widened explicitly. range_type is uint24_t in the shipping instantiation, and + // "uint24_t + int" is ambiguous between its operator uint32_t() conversion and its own + // operator+, so this line never compiled for that type -- RangeSum() has no caller in + // Source/, which is why nobody noticed. Subtracting first is safe because maxIndex >= + // minIndex within a single range by construction. + sum += (unsigned)ranges[i].maxIndex - (unsigned)ranges[i].minIndex + 1; } return sum; } diff --git a/Tests/Unit/RangeListTests.cpp b/Tests/Unit/RangeListTests.cpp index 4c6c37be4..99f301f3d 100644 --- a/Tests/Unit/RangeListTests.cpp +++ b/Tests/Unit/RangeListTests.cpp @@ -314,3 +314,132 @@ TEST(RangeList, AckLikeArrivalPatternMatchesTheModel) ASSERT_EQ(list.Size(), ExpectedRangeCount(model)) << "step " << step << ", value " << value; } } + +// --------------------------------------------------------------------------------------------- +// The reliability layer instantiates RangeList with DatagramSequenceNumberType, which is uint24_t +// -- a type that WRAPS at 0xFFFFFF. The cases above all use unsigned, so none of them exercise the +// arithmetic the shipping instantiation actually performs: Insert() compares against +// minIndex-(range_type)1 and maxIndex+(range_type)1, and at the extremes those wrap. +// +// These pin what the type does today rather than asserting a wrap-aware design it does not have. +// RangeNodeComp orders by raw value, so a range list spanning the wrap point is ordered by value +// and not by sequence adjacency; that is a pre-existing property of the structure, recorded here so +// a future change to it is a deliberate one. +// --------------------------------------------------------------------------------------------- + +namespace +{ + typedef DataStructures::RangeList SeqRangeList; + const uint32_t kUint24Max = 0x00FFFFFF; +} + +TEST(RangeListUint24, ContiguousSequenceNumbersMergeAwayFromTheWrapPoint) +{ + SeqRangeList list; + for (uint32_t i = 1000; i <= 1010; ++i) + list.Insert(MafiaNet::uint24_t(i)); + + EXPECT_EQ(list.Size(), 1u); + EXPECT_EQ(list.RangeSum(), 11u); + EXPECT_TRUE(list.IsWithinRange(MafiaNet::uint24_t(1000))); + EXPECT_TRUE(list.IsWithinRange(MafiaNet::uint24_t(1010))); + EXPECT_FALSE(list.IsWithinRange(MafiaNet::uint24_t(1011))); +} + +// The fix for the missed merge added arithmetic on the previous range's maxIndex. Exercise it with +// the wrapping type, at the top of the range where maxIndex+1 wraps to 0. +TEST(RangeListUint24, MergingJustBelowTheWrapPointBehavesLikeAnywhereElse) +{ + SeqRangeList list; + list.Insert(MafiaNet::uint24_t(kUint24Max - 2)); + list.Insert(MafiaNet::uint24_t(kUint24Max)); // leaves a one-wide gap at max-1 + ASSERT_EQ(list.Size(), 2u); + + list.Insert(MafiaNet::uint24_t(kUint24Max - 1)); // closes it + + EXPECT_EQ(list.Size(), 1u) << "the gap below the wrap point must fuse like any other"; + EXPECT_EQ(list.RangeSum(), 3u); + EXPECT_TRUE(list.IsWithinRange(MafiaNet::uint24_t(kUint24Max - 2))); + EXPECT_TRUE(list.IsWithinRange(MafiaNet::uint24_t(kUint24Max - 1))); + EXPECT_TRUE(list.IsWithinRange(MafiaNet::uint24_t(kUint24Max))); +} + +// The missed-merge case that was fixed, expressed in the shipping type: a value sitting directly +// above the previous range must extend it rather than becoming its own entry. +TEST(RangeListUint24, ValueAbovePreviousRangeExtendsItRatherThanAddingAnEntry) +{ + SeqRangeList list; + list.Insert(MafiaNet::uint24_t(100)); + list.Insert(MafiaNet::uint24_t(103)); + list.Insert(MafiaNet::uint24_t(101)); + + EXPECT_EQ(list.Size(), 2u) << "101 must extend [100,100] instead of starting a third entry"; + EXPECT_EQ(list.RangeSum(), 3u); +} + +// Crossing the wrap boundary. Ordering is by raw value, so 0 does NOT merge with 0xFFFFFF even +// though they are adjacent as sequence numbers. Recorded as current behaviour: coverage stays +// correct, which is what the ack path depends on. +TEST(RangeListUint24, WrapAroundDoesNotFuseButKeepsCoverageCorrect) +{ + SeqRangeList list; + list.Insert(MafiaNet::uint24_t(kUint24Max - 1)); + list.Insert(MafiaNet::uint24_t(kUint24Max)); + list.Insert(MafiaNet::uint24_t(0)); + list.Insert(MafiaNet::uint24_t(1)); + + // Two entries, not one: [0,1] and [max-1,max], ordered by value. + EXPECT_EQ(list.Size(), 2u); + EXPECT_EQ(list.RangeSum(), 4u) << "no value may be lost or double counted across the wrap"; + EXPECT_TRUE(list.IsWithinRange(MafiaNet::uint24_t(0))); + EXPECT_TRUE(list.IsWithinRange(MafiaNet::uint24_t(1))); + EXPECT_TRUE(list.IsWithinRange(MafiaNet::uint24_t(kUint24Max - 1))); + EXPECT_TRUE(list.IsWithinRange(MafiaNet::uint24_t(kUint24Max))); + EXPECT_FALSE(list.IsWithinRange(MafiaNet::uint24_t(2))); + EXPECT_FALSE(list.IsWithinRange(MafiaNet::uint24_t(kUint24Max - 2))); +} + +TEST(RangeListUint24, SerializeRoundTripsSequenceNumbersIncludingTheExtremes) +{ + SeqRangeList source; + source.Insert(MafiaNet::uint24_t(0)); + source.Insert(MafiaNet::uint24_t(1)); + source.Insert(MafiaNet::uint24_t(5000)); + source.Insert(MafiaNet::uint24_t(kUint24Max)); + + MafiaNet::BitStream bs; + source.Serialize(&bs, 2048, false); + bs.SetReadOffset(0); + + SeqRangeList restored; + ASSERT_TRUE(restored.Deserialize(&bs)); + + EXPECT_EQ(restored.Size(), source.Size()); + EXPECT_EQ(restored.RangeSum(), source.RangeSum()); + EXPECT_TRUE(restored.IsWithinRange(MafiaNet::uint24_t(0))); + EXPECT_TRUE(restored.IsWithinRange(MafiaNet::uint24_t(1))); + EXPECT_TRUE(restored.IsWithinRange(MafiaNet::uint24_t(5000))); + EXPECT_TRUE(restored.IsWithinRange(MafiaNet::uint24_t(kUint24Max))); +} + +// Differential with the wrapping type, staying inside a window well away from the boundary so the +// model and the structure agree on adjacency. +TEST(RangeListUint24, DifferentialAgainstAModelWithinAWindow) +{ + SeqRangeList list; + std::set model; + + unsigned seed = 0x24b17799u; + const unsigned base = 70000; + for (int step = 0; step < 3000; ++step) + { + seed = seed * 1103515245u + 12345u; + const unsigned value = base + ((seed >> 16) % 200u); + + list.Insert(MafiaNet::uint24_t(value)); + model.insert(value); + + ASSERT_EQ(list.RangeSum(), (unsigned)model.size()) << "step " << step << ", value " << value; + ASSERT_EQ(list.Size(), ExpectedRangeCount(model)) << "step " << step << ", value " << value; + } +} From feab638b05cf9eb0cc2ebf85a976a604fd217a6b Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Sat, 10 Oct 2026 10:43:43 +0200 Subject: [PATCH 6/8] refactor(ds): replace DataStructures::Heap with a std::vector-backed WeightedHeap Last container in the stage 1 migration. The outgoing packet buffer and the per-channel ordering heaps were the only users left once the dead Tree, WeightedGraph and OrderedChannelHeap headers went. Source/include/mafianet/weighted_heap.h is a small minimum-first heap holding std::push_heap/std::pop_heap over a std::vector, so the heap arithmetic is the standard library's rather than ours. Not std::priority_queue: the reliability layer reads the minimum's weight without popping it, and sweeps every element by index when freeing on teardown and when scanning for stale split packets, neither of which priority_queue exposes. StartSeries()/PushSeries() are deliberately not reproduced. They appended without sifting while the caller promised ascending weights, and measurement of the one call site's pattern shows the promise bought nothing: at 64, 750 and 4000 fragments PushSeries was never faster than an ordinary Push and at 4000 was 12% slower. It also silently corrupts pop order if the promise is ever broken, so it was risk without return. Both call sites now use Push. Measured against the implementation it replaces, on long-lived heaps, which is how the reliability layer holds them -- one per connection, reused: steady state, 2M interleaved push+pop depth 16 23.8ms -> 16.6ms 0.70x depth 128 29.2ms -> 24.8ms 0.85x depth 1024 36.4ms -> 28.7ms 0.79x split-packet burst, push N ascending then drain 64 frags 2.0ms -> 1.9ms 0.92x 750 frags 28.3ms -> 26.1ms 0.92x 4000 frags 137.4ms -> 216.7ms 1.58x So the path every message takes is 15-30% faster, bursts up to roughly a megabyte are 8% faster, and the regression is confined to single messages of several megabytes -- 4000 fragments is about 5.6MB at a 1400 byte MTU, where 72us of heap work per message sits behind the I/O and copying it takes to send it. Taking that trade rather than keeping a hand-written heap. A first pass measured 2.1x SLOWER and was wrong: it constructed and destroyed a heap per repetition, so it was timing the allocator rather than the heap. Tests/Unit/WeightedHeapTests.cpp is new -- the structure deciding the order packets leave the send buffer had none. 9 cases, including a differential that drives it and a std::multiset model through 20000 mixed pushes and pops and checks every pop returns the lowest weight still held, plus the ascending-run ordering that PushSeries used to be responsible for. ds_heap.h is deleted. Remaining DataStructures:: matches for the stage 1 containers under Source/ are all commented-out code, except RangeList, which stays by design. Verified on macOS arm64: full ctest 335/335. --- Source/CMakeLists.txt | 2 +- Source/include/mafianet/ds_heap.h | 310 -------------------- Source/include/mafianet/reliability_layer.h | 6 +- Source/include/mafianet/weighted_heap.h | 122 ++++++++ Source/src/reliability_layer.cpp | 17 +- Tests/Unit/WeightedHeapTests.cpp | 197 +++++++++++++ 6 files changed, 331 insertions(+), 323 deletions(-) delete mode 100644 Source/include/mafianet/ds_heap.h create mode 100644 Source/include/mafianet/weighted_heap.h create mode 100644 Tests/Unit/WeightedHeapTests.cpp diff --git a/Source/CMakeLists.txt b/Source/CMakeLists.txt index a7543dd17..1944a57d0 100644 --- a/Source/CMakeLists.txt +++ b/Source/CMakeLists.txt @@ -159,7 +159,7 @@ set(MAFIANET_HEADERS include/mafianet/ds_byte_pool.h include/mafianet/ds_byte_queue.h include/mafianet/ds_hash.h - include/mafianet/ds_heap.h + include/mafianet/weighted_heap.h include/mafianet/ds_huffman_encoding_tree.h include/mafianet/ds_huffman_encoding_tree_factory.h include/mafianet/ds_huffman_encoding_tree_node.h diff --git a/Source/include/mafianet/ds_heap.h b/Source/include/mafianet/ds_heap.h deleted file mode 100644 index 65856085d..000000000 --- a/Source/include/mafianet/ds_heap.h +++ /dev/null @@ -1,310 +0,0 @@ -/* - * Original work: Copyright (c) 2014, Oculus VR, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * RakNet License.txt file in the licenses directory of this source tree. An additional grant - * of patent rights can be found in the RakNet Patents.txt file in the same directory. - * - * - * Modified work: Copyright (c) 2016-2017, SLikeSoft UG (haftungsbeschränkt) - * - * This source code was modified by SLikeSoft. Modifications are licensed under the MIT-style - * license found in the license.txt file in the root directory of this source tree. - */ - -/// \file ds_heap.h -/// \internal -/// \brief Heap (Also serves as a priority queue) -/// - - - -#ifndef __RAKNET_HEAP_H -#define __RAKNET_HEAP_H - -#include "mafianet/memory_override.h" -#include "mafianet/ds_list.h" -#include "mafianet/export.h" -#include "mafianet/assert.h" - -#ifdef _MSC_VER -#pragma warning( push ) -#endif - -/// The namespace DataStructures was only added to avoid compiler errors for commonly named data structures -/// As these data structures are stand-alone, you can use them outside of RakNet for your own projects if you wish. -namespace DataStructures -{ - template - class RAK_DLL_EXPORT Heap - { - public: - struct HeapNode - { - HeapNode() {} - HeapNode(const weight_type &w, const data_type &d) : weight(w), data(d) {} - weight_type weight; // I'm assuming key is a native numerical type - float or int - data_type data; - }; - - Heap(); - ~Heap(); - void Push(const weight_type &weight, const data_type &data, const char *file, unsigned int line); - /// Call before calling PushSeries, for a new series of items - void StartSeries(void) {optimizeNextSeriesPush=false;} - /// If you are going to push a list of items, where the weights of the items on the list are in order and follow the heap order, PushSeries is faster than Push() - void PushSeries(const weight_type &weight, const data_type &data, const char *file, unsigned int line); - data_type Pop(const unsigned startingIndex); - data_type Peek(const unsigned startingIndex=0) const; - weight_type PeekWeight(const unsigned startingIndex=0) const; - void Clear(bool doNotDeallocateSmallBlocks, const char *file, unsigned int line); - data_type& operator[] ( const unsigned int position ) const; - unsigned Size(void) const; - - protected: - unsigned LeftChild(const unsigned i) const; - unsigned RightChild(const unsigned i) const; - unsigned Parent(const unsigned i) const; - void Swap(const unsigned i, const unsigned j); - DataStructures::List heap; - bool optimizeNextSeriesPush; - }; - - template - Heap::Heap() - { - optimizeNextSeriesPush=false; - } - - template - Heap::~Heap() - { - //Clear(true, _FILE_AND_LINE_); - } - - template - void Heap::PushSeries(const weight_type &weight, const data_type &data, const char *file, unsigned int line) - { - if (optimizeNextSeriesPush==false) - { - // If the weight of what we are inserting is greater than / less than in order of the heap of every sibling and sibling of parent, then can optimize next push - unsigned currentIndex = heap.Size(); - unsigned parentIndex; - if (currentIndex>0) - { - for (parentIndex = Parent(currentIndex); parentIndex < currentIndex; parentIndex++) - { -#ifdef _MSC_VER -#pragma warning(disable:4127) // conditional expression is constant -#endif - if (isMaxHeap) - { - // Every child is less than its parent - if (weight>heap[parentIndex].weight) - { - // Can't optimize - Push(weight,data,file,line); - return; - } - } - else - { - // Every child is greater than than its parent - if (weight - void Heap::Push(const weight_type &weight, const data_type &data, const char *file, unsigned int line) - { - unsigned currentIndex = heap.Size(); - unsigned parentIndex; - heap.Insert(HeapNode(weight, data), file, line); - while (currentIndex!=0) - { - parentIndex = Parent(currentIndex); -#ifdef _MSC_VER -#pragma warning( disable : 4127 ) // warning C4127: conditional expression is constant -#endif - if (isMaxHeap) - { - if (heap[parentIndex].weight < weight) - { - Swap(currentIndex, parentIndex); - currentIndex=parentIndex; - } - else - break; - } - else - { - if (heap[parentIndex].weight > weight) - { - Swap(currentIndex, parentIndex); - currentIndex=parentIndex; - } - else - break; - } - } - } - - template - data_type Heap::Pop(const unsigned startingIndex) - { - // While we have children, swap out with the larger of the two children. - - // This line will assert on an empty heap - data_type returnValue=heap[startingIndex].data; - - // Move the last element to the head, and re-heapify - heap[startingIndex]=heap[heap.Size()-1]; - - unsigned currentIndex,leftChild,rightChild; - weight_type currentWeight; - currentIndex=startingIndex; - currentWeight=heap[startingIndex].weight; - heap.RemoveFromEnd(); - -#ifdef _MSC_VER -#pragma warning( disable : 4127 ) // warning C4127: conditional expression is constant -#endif - for(;;) - { - leftChild=LeftChild(currentIndex); - rightChild=RightChild(currentIndex); - if (leftChild >= heap.Size()) - { - // Done - return returnValue; - } - if (rightChild >= heap.Size()) - { - // Only left node. - if ((isMaxHeap==true && currentWeight < heap[leftChild].weight) || - (isMaxHeap==false && currentWeight > heap[leftChild].weight)) - Swap(leftChild, currentIndex); - - return returnValue; - } - else - { - // Swap with the bigger/smaller of the two children and continue - if (isMaxHeap) - { - if (heap[leftChild].weight <= currentWeight && heap[rightChild].weight <= currentWeight) - return returnValue; - - if (heap[leftChild].weight > heap[rightChild].weight) - { - Swap(leftChild, currentIndex); - currentIndex=leftChild; - } - else - { - Swap(rightChild, currentIndex); - currentIndex=rightChild; - } - } - else - { - if (heap[leftChild].weight >= currentWeight && heap[rightChild].weight >= currentWeight) - return returnValue; - - if (heap[leftChild].weight < heap[rightChild].weight) - { - Swap(leftChild, currentIndex); - currentIndex=leftChild; - } - else - { - Swap(rightChild, currentIndex); - currentIndex=rightChild; - } - } - } - } - } - - template - inline data_type Heap::Peek(const unsigned startingIndex) const - { - return heap[startingIndex].data; - } - - template - inline weight_type Heap::PeekWeight(const unsigned startingIndex) const - { - return heap[startingIndex].weight; - } - - template - void Heap::Clear(bool doNotDeallocateSmallBlocks, const char *file, unsigned int line) - { - heap.Clear(doNotDeallocateSmallBlocks, file, line); - } - - template - inline data_type& Heap::operator[] ( const unsigned int position ) const - { - return heap[position].data; - } - template - unsigned Heap::Size(void) const - { - return heap.Size(); - } - - template - inline unsigned Heap::LeftChild(const unsigned i) const - { - return i*2+1; - } - - template - inline unsigned Heap::RightChild(const unsigned i) const - { - return i*2+2; - } - - template - inline unsigned Heap::Parent(const unsigned i) const - { -#ifdef _DEBUG - RakAssert(i!=0); -#endif - return (i-1)/2; - } - - template - void Heap::Swap(const unsigned i, const unsigned j) - { - HeapNode temp; - temp=heap[i]; - heap[i]=heap[j]; - heap[j]=temp; - } -} - -#ifdef _MSC_VER -#pragma warning( pop ) -#endif - -#endif diff --git a/Source/include/mafianet/reliability_layer.h b/Source/include/mafianet/reliability_layer.h index 333d1e5ee..b5314feb5 100644 --- a/Source/include/mafianet/reliability_layer.h +++ b/Source/include/mafianet/reliability_layer.h @@ -36,7 +36,7 @@ #include "mafianet/ds_range_list.h" #include "mafianet/ds_memory_pool.h" #include "mafianet/defines.h" -#include "mafianet/ds_heap.h" +#include "mafianet/weighted_heap.h" #include "mafianet/bit_stream.h" #include "mafianet/native_feature_includes.h" #include "mafianet/secure_handshake.h" @@ -479,7 +479,7 @@ class ReliabilityLayer// // CCTimeType lastPacketlossTime; //DataStructures::Queue sendPacketSet[ MafiaNet::NUMBER_OF_PRIORITIES ]; - DataStructures::Heap outgoingPacketBuffer; + MafiaNet::WeightedHeap outgoingPacketBuffer; reliabilityHeapWeightType outgoingPacketBufferNextWeights[MafiaNet::NUMBER_OF_PRIORITIES]; void InitHeapWeights(void); reliabilityHeapWeightType GetNextWeight(int priorityLevel); @@ -527,7 +527,7 @@ class ReliabilityLayer// OrderingIndexType orderedReadIndex[NUMBER_OF_ORDERED_STREAMS]; // Highest value received for sequencedWriteIndex for the current value of orderedReadIndex on the same channel. OrderingIndexType highestSequencedReadIndex[NUMBER_OF_ORDERED_STREAMS]; - DataStructures::Heap orderingHeaps[NUMBER_OF_ORDERED_STREAMS]; + MafiaNet::WeightedHeap orderingHeaps[NUMBER_OF_ORDERED_STREAMS]; OrderingIndexType heapIndexOffsets[NUMBER_OF_ORDERED_STREAMS]; diff --git a/Source/include/mafianet/weighted_heap.h b/Source/include/mafianet/weighted_heap.h new file mode 100644 index 000000000..528370cfc --- /dev/null +++ b/Source/include/mafianet/weighted_heap.h @@ -0,0 +1,122 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +/// \file weighted_heap.h +/// \brief A minimum-first binary heap over std::vector, used for outgoing packet ordering. + +#ifndef __WEIGHTED_HEAP_H +#define __WEIGHTED_HEAP_H + +#include +#include +#include + +#include "mafianet/assert.h" + +namespace MafiaNet +{ + /// \brief Pops the element with the lowest weight first. + /// + /// Replaces DataStructures::Heap, which was a hand-written sift-up/sift-down over + /// DataStructures::List. This keeps std::push_heap/std::pop_heap on a std::vector, so the heap + /// arithmetic is the standard library's rather than ours, while staying a small named type + /// instead of std::priority_queue: the reliability layer needs to read the minimum's weight + /// without popping it (PeekWeight), and measured ~1.8x slower pushes through a + /// priority_queue of std::pair on the split-packet path. + /// + /// The predecessor also had StartSeries()/PushSeries(), which appended without sifting while a + /// caller promised ascending weights. It is deliberately not reproduced: benchmarking the one + /// call site's pattern (64, 750 and 4000 fragments) showed it was never faster than an ordinary + /// push and usually slower, while silently corrupting pop order if the promise were ever broken. + template + class WeightedHeap + { + public: + WeightedHeap() {} + ~WeightedHeap() {} + + void Push(const weight_type &weight, const data_type &data) + { + heap.push_back(Node(weight, data)); + std::push_heap(heap.begin(), heap.end(), Greater()); + } + + /// \brief Lowest-weight element, which must exist. + const data_type &Peek(void) const + { + RakAssert(heap.empty()==false); + return heap.front().data; + } + + /// \brief Weight of the lowest-weight element, which must exist. + const weight_type &PeekWeight(void) const + { + RakAssert(heap.empty()==false); + return heap.front().weight; + } + + /// \brief Removes and returns the lowest-weight element, which must exist. + data_type Pop(void) + { + RakAssert(heap.empty()==false); + std::pop_heap(heap.begin(), heap.end(), Greater()); + data_type out = heap.back().data; + heap.pop_back(); + return out; + } + + void Clear(void) + { + heap.clear(); + } + + unsigned Size(void) const + { + return (unsigned) heap.size(); + } + + bool IsEmpty(void) const + { + return heap.empty(); + } + + /// \brief Element at a position in the backing array, in heap order rather than weight order. + /// For sweeps over everything held -- freeing on teardown, scanning for stale entries -- where + /// the caller does not care about ordering. Same contract as the structure this replaced. + const data_type &operator[](unsigned index) const + { + RakAssert(index < heap.size()); + return heap[index].data; + } + + data_type &operator[](unsigned index) + { + RakAssert(index < heap.size()); + return heap[index].data; + } + + private: + struct Node + { + Node() {} + Node(const weight_type &w, const data_type &d) : weight(w), data(d) {} + weight_type weight; + data_type data; + }; + + // std::push_heap builds a maximum-first heap, so inverting the comparison gives + // minimum-first, matching the isMaxHeap=false instantiations this replaces. + struct Greater + { + bool operator()(const Node &a, const Node &b) const { return b.weight < a.weight; } + }; + + std::vector heap; + }; +} + +#endif diff --git a/Source/src/reliability_layer.cpp b/Source/src/reliability_layer.cpp index 79343711e..4e8dbd582 100644 --- a/Source/src/reliability_layer.cpp +++ b/Source/src/reliability_layer.cpp @@ -606,7 +606,7 @@ void ReliabilityLayer::FreeThreadSafeMemory( void ) FreeInternalPacketData(orderingHeaps[i][j], _FILE_AND_LINE_ ); ReleaseToInternalPacketPool( orderingHeaps[i][j] ); } - orderingHeaps[i].Clear(true, _FILE_AND_LINE_); + orderingHeaps[i].Clear(); } //resendList.ForEachData(DeleteInternalPacket); @@ -646,7 +646,7 @@ void ReliabilityLayer::FreeThreadSafeMemory( void ) ReleaseToInternalPacketPool( outgoingPacketBuffer[ j ] ); } - outgoingPacketBuffer.Clear(true, _FILE_AND_LINE_); + outgoingPacketBuffer.Clear(); #ifdef _DEBUG for (i = 0; i < delayList.Size(); i++ ) @@ -1437,7 +1437,7 @@ bool ReliabilityLayer::HandleSocketReceiveFromConnectedPlayer( while (orderingHeaps[internalPacket->orderingChannel].Size()>0 && orderingHeaps[internalPacket->orderingChannel].Peek()->orderingIndex==orderedReadIndex[internalPacket->orderingChannel]) { - internalPacket = orderingHeaps[internalPacket->orderingChannel].Pop(0); + internalPacket = orderingHeaps[internalPacket->orderingChannel].Pop(); #ifdef PRINT_TO_FILE_RELIABLE_ORDERED_TEST BitStream bitStream2(internalPacket->data, BITS_TO_BYTES(internalPacket->dataBitLength), false); @@ -1504,7 +1504,7 @@ bool ReliabilityLayer::HandleSocketReceiveFromConnectedPlayer( weight+=internalPacket->sequencingIndex; else weight+=(1048576-1); - orderingHeaps[internalPacket->orderingChannel].Push(weight, internalPacket, _FILE_AND_LINE_); + orderingHeaps[internalPacket->orderingChannel].Push(weight, internalPacket); #ifdef PRINT_TO_FILE_RELIABLE_ORDERED_TEST if (packetId==ID_USER_PACKET_ENUM+1 && fp) @@ -1734,7 +1734,7 @@ bool ReliabilityLayer::Send( char *data, BitSize_t numberOfBitsToSend, MafiaNet: RakAssert(internalPacket->dataBitLengthmessageNumberAssigned==false); - outgoingPacketBuffer.Push( GetNextWeight((int)internalPacket->priority), internalPacket, _FILE_AND_LINE_ ); + outgoingPacketBuffer.Push( GetNextWeight((int)internalPacket->priority), internalPacket ); RakAssert(outgoingPacketBuffer.Size()==0 || outgoingPacketBuffer.Peek()->dataBitLengthpriority]++; statistics.bytesInSendBuffer[(int)internalPacket->priority]+=(double) BITS_TO_BYTES(internalPacket->dataBitLength); @@ -2101,7 +2101,7 @@ void ReliabilityLayer::UpdateInternal( RakNetSocket2 *s, SystemAddress &systemAd if (internalPacket->data==0) { //sendPacketSet[ i ].Pop(); - outgoingPacketBuffer.Pop(0); + outgoingPacketBuffer.Pop(); RakAssert(outgoingPacketBuffer.Size()==0 || outgoingPacketBuffer.Peek()->dataBitLengthpriority]--; statistics.bytesInSendBuffer[(int)internalPacket->priority]-=(double) BITS_TO_BYTES(internalPacket->dataBitLength); @@ -2132,7 +2132,7 @@ void ReliabilityLayer::UpdateInternal( RakNetSocket2 *s, SystemAddress &systemAd isReliable = false; //sendPacketSet[ i ].Pop(); - outgoingPacketBuffer.Pop(0); + outgoingPacketBuffer.Pop(); RakAssert(outgoingPacketBuffer.Size()==0 || outgoingPacketBuffer.Peek()->dataBitLengthmessageNumberAssigned==false); statistics.messageInSendBuffer[(int)internalPacket->priority]--; @@ -3130,7 +3130,6 @@ void ReliabilityLayer::SplitPacket( InternalPacket *internalPacket ) // Tell the heap we are going to push a list of elements where each element in the list follows the heap order RakAssert(outgoingPacketBuffer.Size()==0 || outgoingPacketBuffer.Peek()->dataBitLengthsplitPacketCount; i++ ) @@ -3141,7 +3140,7 @@ void ReliabilityLayer::SplitPacket( InternalPacket *internalPacket ) // sendPacketSet[ internalPacket->priority ].Push( internalPacketArray[ i ], _FILE_AND_LINE_ ); RakAssert(internalPacketArray[ i ]->dataBitLengthmessageNumberAssigned==false); - outgoingPacketBuffer.PushSeries(GetNextWeight((int)internalPacketArray[ i ]->priority), internalPacketArray[ i ], _FILE_AND_LINE_); + outgoingPacketBuffer.Push(GetNextWeight((int)internalPacketArray[ i ]->priority), internalPacketArray[ i ]); RakAssert(outgoingPacketBuffer.Size()==0 || outgoingPacketBuffer.Peek()->dataBitLengthpriority]++; statistics.bytesInSendBuffer[(int)(int)internalPacketArray[ i ]->priority]+=(double) BITS_TO_BYTES(internalPacketArray[ i ]->dataBitLength); diff --git a/Tests/Unit/WeightedHeapTests.cpp b/Tests/Unit/WeightedHeapTests.cpp new file mode 100644 index 000000000..78298cc3a --- /dev/null +++ b/Tests/Unit/WeightedHeapTests.cpp @@ -0,0 +1,197 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +#include + +#include +#include +#include + +#include "mafianet/weighted_heap.h" + +using namespace MafiaNet; + +/* +Description: +Tests for the weighted min-heap the reliability layer keeps outgoing packets and per-channel +ordering in. + +Written when that structure moved off DataStructures::Heap (std migration stage 1). It had no tests, +despite deciding the order packets leave the send buffer: pop the wrong element and messages go out +in the wrong order, which is not something the integration suite would reliably surface. + +The load-bearing case is WeightedHeapDifferential: it drives the heap and a std::multiset model +through the same fixed-seed mix of pushes and pops and checks that every pop returns the lowest +weight still held. A heap bug that only shows up under a particular interleaving of pushes and pops +is exactly what a hand-written sequence of operations misses. +*/ + +namespace +{ + typedef MafiaNet::WeightedHeap Heap; + + // Drain the heap and return weights in pop order. + std::vector DrainWeights(Heap &heap) + { + std::vector out; + while (heap.Size()) + { + out.push_back(heap.PeekWeight()); + heap.Pop(); + } + return out; + } +} // namespace + +TEST(WeightedHeap, EmptyHeapReportsNoSize) +{ + Heap heap; + EXPECT_EQ(heap.Size(), 0u); + EXPECT_TRUE(heap.IsEmpty()); +} + +TEST(WeightedHeap, SingleElementPeeksAndPops) +{ + Heap heap; + heap.Push(4.5, 42); + + ASSERT_EQ(heap.Size(), 1u); + EXPECT_FALSE(heap.IsEmpty()); + EXPECT_EQ(heap.Peek(), 42); + EXPECT_DOUBLE_EQ(heap.PeekWeight(), 4.5); + + EXPECT_EQ(heap.Pop(), 42); + EXPECT_EQ(heap.Size(), 0u); +} + +TEST(WeightedHeap, PopsInAscendingWeightOrderWhateverThePushOrder) +{ + const double weights[] = {5.0, 1.0, 4.0, 2.0, 3.0}; + Heap heap; + for (int i = 0; i < 5; ++i) + heap.Push(weights[i], i); + + const std::vector popped = DrainWeights(heap); + ASSERT_EQ(popped.size(), (size_t)5); + for (int i = 0; i < 5; ++i) + EXPECT_DOUBLE_EQ(popped[i], (double)(i + 1)) << "position " << i; +} + +TEST(WeightedHeap, AscendingPushesPopInTheSameOrder) +{ + Heap heap; + for (int i = 0; i < 64; ++i) + heap.Push((double)i, i); + + const std::vector popped = DrainWeights(heap); + ASSERT_EQ(popped.size(), (size_t)64); + for (int i = 0; i < 64; ++i) + EXPECT_DOUBLE_EQ(popped[i], (double)i) << "position " << i; +} + +TEST(WeightedHeap, DescendingPushesStillPopAscending) +{ + Heap heap; + for (int i = 63; i >= 0; --i) + heap.Push((double)i, i); + + const std::vector popped = DrainWeights(heap); + ASSERT_EQ(popped.size(), (size_t)64); + for (int i = 0; i < 64; ++i) + EXPECT_DOUBLE_EQ(popped[i], (double)i) << "position " << i; +} + +TEST(WeightedHeap, ClearDropsEverything) +{ + Heap heap; + for (int i = 0; i < 10; ++i) + heap.Push((double)i, i); + ASSERT_EQ(heap.Size(), 10u); + + heap.Clear(); + + EXPECT_EQ(heap.Size(), 0u); + EXPECT_TRUE(heap.IsEmpty()); +} + +TEST(WeightedHeap, EqualWeightsAllComeOutAndNoneAreLost) +{ + Heap heap; + for (int i = 0; i < 16; ++i) + heap.Push(7.0, i); + + std::set seen; + while (heap.Size()) + { + EXPECT_DOUBLE_EQ(heap.PeekWeight(), 7.0); + seen.insert(heap.Pop()); + } + EXPECT_EQ(seen.size(), (size_t)16) << "an element was lost or duplicated among equal weights"; +} + +// The split-packet path pushes a run of ascending weights. This was the case DataStructures::Heap +// had a dedicated StartSeries/PushSeries fast path for; measurement showed the fast path was slower +// than an ordinary push, so it is gone, but the ordering it had to preserve is asserted here. +TEST(WeightedHeap, ARunOfAscendingPushesOntoANonEmptyHeapKeepsOrder) +{ + Heap heap; + heap.Push(100.0, 1000); + heap.Push(200.0, 2000); + + for (int i = 0; i < 750; ++i) + heap.Push(300.0 + (double)i, i); + + const std::vector popped = DrainWeights(heap); + ASSERT_EQ(popped.size(), (size_t)752); + EXPECT_DOUBLE_EQ(popped[0], 100.0); + EXPECT_DOUBLE_EQ(popped[1], 200.0); + for (size_t i = 2; i < popped.size(); ++i) + ASSERT_LE(popped[i - 1], popped[i]) << "pop order broke at " << i; +} + +TEST(WeightedHeapDifferential, MatchesAMultisetModelUnderMixedPushAndPop) +{ + Heap heap; + std::multiset model; + + unsigned seed = 0x5eed7777u; + for (int step = 0; step < 20000; ++step) + { + seed = seed * 1103515245u + 12345u; + const unsigned roll = (seed >> 16) % 100u; + + if (roll < 60 || model.empty()) + { + seed = seed * 1103515245u + 12345u; + const double weight = (double)((seed >> 16) % 1000u); + heap.Push(weight, step); + model.insert(weight); + } + else + { + // Every pop must hand back the smallest weight the model still holds. + const double expected = *model.begin(); + ASSERT_EQ(heap.Size(), (unsigned)model.size()) << "step " << step; + ASSERT_DOUBLE_EQ(heap.PeekWeight(), expected) << "step " << step << ": wrong minimum"; + heap.Pop(); + model.erase(model.begin()); + } + ASSERT_EQ(heap.Size(), (unsigned)model.size()) << "step " << step; + } + + // Drain and confirm the tail comes out sorted. + double prev = -1.0; + while (heap.Size()) + { + const double w = heap.PeekWeight(); + ASSERT_LE(prev, w) << "drain order broke"; + prev = w; + heap.Pop(); + model.erase(model.begin()); + } + EXPECT_TRUE(model.empty()); +} From a9c4d7a2adc74e6b8101c7b4c5cf78dfaea1f21d Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Sat, 10 Oct 2026 11:01:26 +0200 Subject: [PATCH 7/8] fix(table): refuse a duplicate row id in the AddRow overloads that take cells Review finding on #71, and a regression I introduced. Moving the rows to std::map turned three call sites into rows[rowId] = newRow, which replaces the row already stored. That leaks it and leaves anything holding a pointer to it outside the table -- Room::tableRow in the Lobby2 rooms container comes straight from one of these overloads. The B+ tree Insert that preceded it refused duplicate keys, so the table was never modified in that case. It ignored the result here and returned a row it had not stored, which leaked the NEW row and handed the caller something the table did not contain. Both behaviours are wrong; the fix is neither. All three now use insert() and, on a duplicate, free the new row and return 0, matching the AddRow(unsigned) overload that already documented "Already exists" that way. Two tests added. Only AddRow(unsigned) was covered when the rows moved, which is exactly how this got through: the overloads taking initial cell values had no duplicate-id case at all. Checked that they bite -- restoring the overwriting version fails both. Verified on macOS arm64: full ctest 337/337, Table suite clean under ASAN, samples build including Lobby2 clean. --- Source/src/ds_table.cpp | 39 ++++++++++++++++++++++++++++++--- Tests/Unit/TableTests.cpp | 46 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/Source/src/ds_table.cpp b/Source/src/ds_table.cpp index 2bda83942..5a6e3c368 100644 --- a/Source/src/ds_table.cpp +++ b/Source/src/ds_table.cpp @@ -381,7 +381,18 @@ Table::Row* Table::AddRow(unsigned rowId, DataStructures::List &initialCel else newRow->cells.Insert(MafiaNet::OP_NEW(_FILE_AND_LINE_), _FILE_AND_LINE_ ); } - rows[rowId] = newRow; + // Refuse a duplicate id rather than replacing the row, which is what the B+ tree Insert this + // replaced did. Overwriting would leak the row already held and leave anyone holding a pointer to + // it -- Room::tableRow, for one -- pointing at a row no longer in the table. + // + // Returning 0 matches the AddRow(unsigned) overload. The B+ tree version ignored Insert's result + // here and handed back a row it had not stored, which leaked the new row and gave the caller + // something the table did not contain. + if (rows.insert(std::make_pair(rowId, newRow)).second==false) + { + DeleteRow(newRow); + return 0; + } return newRow; } Table::Row* Table::AddRow(unsigned rowId, DataStructures::List &initialCellValues, bool copyCells) @@ -404,7 +415,18 @@ Table::Row* Table::AddRow(unsigned rowId, DataStructures::List &initialCe else newRow->cells.Insert(MafiaNet::OP_NEW(_FILE_AND_LINE_), _FILE_AND_LINE_); } - rows[rowId] = newRow; + // Refuse a duplicate id rather than replacing the row, which is what the B+ tree Insert this + // replaced did. Overwriting would leak the row already held and leave anyone holding a pointer to + // it -- Room::tableRow, for one -- pointing at a row no longer in the table. + // + // Returning 0 matches the AddRow(unsigned) overload. The B+ tree version ignored Insert's result + // here and handed back a row it had not stored, which leaked the new row and gave the caller + // something the table did not contain. + if (rows.insert(std::make_pair(rowId, newRow)).second==false) + { + DeleteRow(newRow); + return 0; + } return newRow; } Table::Row* Table::AddRowColumns(unsigned rowId, Row *row, DataStructures::List columnIndices) @@ -427,7 +449,18 @@ Table::Row* Table::AddRowColumns(unsigned rowId, Row *row, DataStructures::List< newRow->cells.Insert(MafiaNet::OP_NEW(_FILE_AND_LINE_), _FILE_AND_LINE_); } } - rows[rowId] = newRow; + // Refuse a duplicate id rather than replacing the row, which is what the B+ tree Insert this + // replaced did. Overwriting would leak the row already held and leave anyone holding a pointer to + // it -- Room::tableRow, for one -- pointing at a row no longer in the table. + // + // Returning 0 matches the AddRow(unsigned) overload. The B+ tree version ignored Insert's result + // here and handed back a row it had not stored, which leaked the new row and gave the caller + // something the table did not contain. + if (rows.insert(std::make_pair(rowId, newRow)).second==false) + { + DeleteRow(newRow); + return 0; + } return newRow; } bool Table::RemoveRow(unsigned rowId) diff --git a/Tests/Unit/TableTests.cpp b/Tests/Unit/TableTests.cpp index fe51ebc03..61a115698 100644 --- a/Tests/Unit/TableTests.cpp +++ b/Tests/Unit/TableTests.cpp @@ -97,6 +97,52 @@ TEST(Table, AddRowRejectsADuplicateIdWithoutReplacingTheRow) EXPECT_EQ(table.GetRowCount(), 1u); } +// The AddRow overloads that take initial cell values must refuse a duplicate id too. Only the plain +// AddRow(unsigned) overload was covered when the rows moved to std::map, and the others were written +// as rows[rowId] = newRow -- which replaced the stored row, leaked it, and left anything holding a +// pointer to it (Room::tableRow in the Lobby2 rooms container, for one) pointing outside the table. +TEST(Table, AddRowWithInitialCellValuesRefusesADuplicateId) +{ + Table table; + table.AddColumn("value", Table::NUMERIC); + + DataStructures::List initial; + Table::Cell cell; + cell.Set(5); + initial.Insert(cell, _FILE_AND_LINE_); + + Table::Row *first = table.AddRow(1, initial); + ASSERT_NE(first, nullptr); + ASSERT_EQ(table.GetRowCount(), 1u); + + Table::Row *second = table.AddRow(1, initial); + EXPECT_EQ(second, nullptr) << "a duplicate id must be refused, not overwrite the stored row"; + EXPECT_EQ(table.GetRowCount(), 1u); + EXPECT_EQ(table.GetRowByID(1), first) << "the originally stored row must still be the one held"; +} + +TEST(Table, AddRowWithCellPointersRefusesADuplicateId) +{ + Table table; + table.AddColumn("value", Table::NUMERIC); + + DataStructures::List initial; + Table::Cell *cell = MafiaNet::OP_NEW(_FILE_AND_LINE_); + cell->Set(7); + initial.Insert(cell, _FILE_AND_LINE_); + + Table::Row *first = table.AddRow(2, initial, true); + ASSERT_NE(first, nullptr); + ASSERT_EQ(table.GetRowCount(), 1u); + + Table::Row *second = table.AddRow(2, initial, true); + EXPECT_EQ(second, nullptr) << "a duplicate id must be refused, not overwrite the stored row"; + EXPECT_EQ(table.GetRowCount(), 1u); + EXPECT_EQ(table.GetRowByID(2), first); + + MafiaNet::OP_DELETE(cell, _FILE_AND_LINE_); +} + TEST(Table, GetRowByIdFindsPresentRowsAndReportsMissingOnes) { Table table; From e716df5c17abac98b91eae0b3f68be3ddf8ff1fc Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Sat, 10 Oct 2026 15:43:12 +0200 Subject: [PATCH 8/8] docs(ds): correct the performance claims on WeightedHeap The three performance statements in this header were wrong. Re-measured, both as microbenchmarks and end to end through two ReliabilityLayer instances on a fake socket with simulated time, comparing 32c1a957 (DataStructures::Heap) against feab638b (this class) -- two commits that differ only in the heap. - "~1.8x slower pushes through a priority_queue of std::pair": false. A priority_queue of the same node measures the same as this class at every depth tried, and slightly faster on fragment bursts. The real reason not to use it is that it exposes neither the minimum's weight nor indexed access for the teardown sweep, both of which the reliability layer uses. - "StartSeries/PushSeries was never faster than an ordinary push and usually slower": false. It does fewer comparisons and measures up to ~35% faster in a microbenchmark of its call site's pattern. Removing it is still right, because it silently corrupts pop order if its ascending-weight precondition breaks, and its advantage does not survive to the end-to-end path -- but the justification was not the one given. - A 1.58x regression on a 4000-fragment burst, claimed on the PR: false. It was a microbenchmark artifact. End to end that case is 1.00x. What the end-to-end numbers actually show: 0.97-1.00x for deep backlogs and fragment bursts, and 1.07x for the lightest case (two small messages per tick). That last figure is a per-operation overhead of vector-backed storage at tiny queue depths which no implementation could remove -- std::push_heap, three hole-method variants, an explicit-count variant and std::priority_queue all measure the same, while the old List-backed heap is uniquely fast there. No code change; the implementation is unchanged and the claims now match measurement. Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c --- Source/include/mafianet/weighted_heap.h | 33 ++++++++++++++++++------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/Source/include/mafianet/weighted_heap.h b/Source/include/mafianet/weighted_heap.h index 528370cfc..c53067d2c 100644 --- a/Source/include/mafianet/weighted_heap.h +++ b/Source/include/mafianet/weighted_heap.h @@ -22,16 +22,31 @@ namespace MafiaNet /// \brief Pops the element with the lowest weight first. /// /// Replaces DataStructures::Heap, which was a hand-written sift-up/sift-down over - /// DataStructures::List. This keeps std::push_heap/std::pop_heap on a std::vector, so the heap - /// arithmetic is the standard library's rather than ours, while staying a small named type - /// instead of std::priority_queue: the reliability layer needs to read the minimum's weight - /// without popping it (PeekWeight), and measured ~1.8x slower pushes through a - /// priority_queue of std::pair on the split-packet path. + /// DataStructures::List. std::push_heap/std::pop_heap over a std::vector put the heap + /// arithmetic in the standard library rather than here. /// - /// The predecessor also had StartSeries()/PushSeries(), which appended without sifting while a - /// caller promised ascending weights. It is deliberately not reproduced: benchmarking the one - /// call site's pattern (64, 750 and 4000 fragments) showed it was never faster than an ordinary - /// push and usually slower, while silently corrupting pop order if the promise were ever broken. + /// It stays a small named type instead of std::priority_queue for a functional reason, not a + /// performance one: the reliability layer reads the minimum's weight without popping it + /// (PeekWeight) and sweeps every queued element by index to free it at teardown + /// (operator[]), and std::priority_queue's interface exposes neither -- reaching its + /// container means deriving from it to get at the protected member. A priority_queue of + /// this same node measured indistinguishable from this class at every queue depth tried. + /// + /// The predecessor also had StartSeries()/PushSeries(), which appended without sifting while + /// a caller promised ascending weights. It is not reproduced because it is unsound, not + /// because it was slow: it silently corrupts pop order if the promise is ever broken. In a + /// microbenchmark of the one call site's pattern it was in fact up to ~35% faster than an + /// ordinary push, but end to end through two ReliabilityLayer instances its advantage did not + /// show above measurement noise, so the safety is free. + /// + /// Measured, comparing this against DataStructures::Heap end to end on the send path (two + /// ReliabilityLayer instances over a fake socket with simulated time, Release, best of 25): + /// deep backlogs and fragment bursts up to ~4000 fragments came out at 0.97-1.00x, and the + /// lightest case (two small messages per tick) at 1.07x. The last figure is a per-operation + /// overhead of vector-backed storage at tiny queue depths that no implementation tried could + /// remove -- std::push_heap, three hole-method variants, an explicit-count variant and + /// std::priority_queue all measured the same -- and it is a fraction of a percent of a real + /// send, which this harness omits the syscall for. template class WeightedHeap {