Link graph: Explicitly flag invalidated flow stats instead of minimising their flows

Entirely exclude invalidated flow stats from link stats

Delete invalidated flow stats if they stay invalid for 32 link graph jobs

This is to prevent large numbers of invalidated flow stats from
unduly influencing link statistics

src/linkgraph/linkgraphjob.cpp

@@ -148,10 +148,11 @@ void LinkGraphJob::FinaliseJob()
 		for (FlowStatMap::iterator it(ge.flows.begin()); it != ge.flows.end();) {
 			FlowStatMap::iterator new_it = flows.find(it->GetOrigin());
 			if (new_it == flows.end()) {
+				bool should_erase = true;
 				if (_settings_game.linkgraph.GetDistributionType(this->Cargo()) != DT_MANUAL) {
-					it->Invalidate();
+					should_erase = it->Invalidate();
-					++it;
+				}
-				} else {
+				if (should_erase) {
 					FlowStat shares(INVALID_STATION, INVALID_STATION, 1);
 					it->SwapShares(shares);
 					it = ge.flows.erase(it);
@@ -159,6 +160,8 @@ void LinkGraphJob::FinaliseJob()
 							shares_it != shares.end(); ++shares_it) {
 						RerouteCargo(st, this->Cargo(), shares_it->second, st->index);
 					}
+				} else {
+					++it;
 				}
 			} else {
 				it->SwapShares(*new_it);

src/saveload/extended_ver_sl.cpp

@@ -111,6 +111,7 @@ const SlxiSubChunkInfo _sl_xv_sub_chunk_infos[] = {
 	{ XSLFI_TOWN_CARGO_MATRIX,      XSCF_NULL,                1,   1, "town_cargo_matrix",         nullptr, nullptr, nullptr        },
 	{ XSLFI_STATE_CHECKSUM,         XSCF_NULL,                1,   1, "state_checksum",            nullptr, nullptr, nullptr        },
 	{ XSLFI_DEBUG,                  XSCF_IGNORABLE_ALL,       1,   1, "debug",                     nullptr, nullptr, "DBGL"      },
+	{ XSLFI_FLOW_STAT_FLAGS,        XSCF_NULL,                1,   1, "flow_stat_flags",           nullptr, nullptr, nullptr        },
 	{ XSLFI_NULL, XSCF_NULL, 0, 0, nullptr, nullptr, nullptr, nullptr },// This is the end marker
 };
 

src/saveload/extended_ver_sl.h

@@ -77,6 +77,7 @@ enum SlXvFeatureIndex {
 	XSLFI_TOWN_CARGO_MATRIX,                      ///< Town cargo matrix savegame format changes
 	XSLFI_STATE_CHECKSUM,                         ///< State checksum
 	XSLFI_DEBUG,                                  ///< Debugging info
+	XSLFI_FLOW_STAT_FLAGS,                        ///< FlowStat flags
 
 	XSLFI_RIFF_HEADER_60_BIT,                     ///< Size field in RIFF chunk header is 60 bit
 	XSLFI_HEIGHT_8_BIT,                           ///< Map tile height is 8 bit instead of 4 bit, but savegame version may be before this became true in trunk

src/saveload/station_sl.cpp

@@ -521,15 +521,15 @@ static void RealSave_STNN(BaseStation *bst)
 		Station *st = Station::From(bst);
 		for (CargoID i = 0; i < NUM_CARGO; i++) {
 			_num_dests = (uint32)st->goods[i].cargo.Packets()->MapSize();
-			_num_flows = 0;
+			_num_flows = st->goods[i].flows.size();
-			for (FlowStatMap::const_iterator it(st->goods[i].flows.begin()); it != st->goods[i].flows.end(); ++it) {
-				_num_flows += (uint32)it->size();
-			}
 			SlObjectSaveFiltered(&st->goods[i], _filtered_goods_desc.data());
 			for (FlowStatMap::const_iterator outer_it(st->goods[i].flows.begin()); outer_it != st->goods[i].flows.end(); ++outer_it) {
 				uint32 sum_shares = 0;
 				FlowSaveLoad flow;
 				flow.source = outer_it->GetOrigin();
+				dumper->CheckBytes(2 + 4);
+				dumper->RawWriteUint16(flow.source);
+				dumper->RawWriteUint32(outer_it->size());
 				FlowStat::const_iterator inner_it(outer_it->begin());
 				const FlowStat::const_iterator end(outer_it->end());
 				for (; inner_it != end; ++inner_it) {
@@ -540,12 +540,12 @@ static void RealSave_STNN(BaseStation *bst)
 					assert(flow.share > 0);
 
 					// SlObject(&flow, _flow_desc); /* this is highly performance-sensitive, manually unroll */
-					dumper->CheckBytes(2 + 2 + 4 + 1);
+					dumper->CheckBytes(2 + 4 + 1);
-					dumper->RawWriteUint16(flow.source);
 					dumper->RawWriteUint16(flow.via);
 					dumper->RawWriteUint32(flow.share);
 					dumper->RawWriteByte(flow.restricted != 0);
 				}
+				SlWriteUint16(outer_it->GetRawFlags());
 			}
 			for (StationCargoPacketMap::ConstMapIterator it(st->goods[i].cargo.Packets()->begin()); it != st->goods[i].cargo.Packets()->end(); ++it) {
 				SlObjectSaveFiltered(const_cast<StationCargoPacketMap::value_type *>(&(*it)), _cargo_list_desc); // _cargo_list_desc has no conditionals
@@ -599,23 +599,46 @@ static void Load_STNN()
 
 			for (CargoID i = 0; i < num_cargo; i++) {
 				SlObjectLoadFiltered(&st->goods[i], _filtered_goods_desc.data());
-				FlowSaveLoad flow;
-				FlowStat *fs = nullptr;
 				StationID prev_source = INVALID_STATION;
-				for (uint32 j = 0; j < _num_flows; ++j) {
+				if (SlXvIsFeaturePresent(XSLFI_FLOW_STAT_FLAGS)) {
-					// SlObject(&flow, _flow_desc); /* this is highly performance-sensitive, manually unroll */
+					for (uint32 j = 0; j < _num_flows; ++j) {
-					buffer->CheckBytes(2 + 2 + 4);
+						FlowSaveLoad flow;
-					flow.source = buffer->RawReadUint16();
+						buffer->CheckBytes(2 + 4);
-					flow.via = buffer->RawReadUint16();
+						flow.source = buffer->RawReadUint16();
-					flow.share = buffer->RawReadUint32();
+						uint32 flow_count = buffer->RawReadUint32();
-					if (!IsSavegameVersionBefore(SLV_187)) flow.restricted = (buffer->ReadByte() != 0);
 
-					if (fs == nullptr || prev_source != flow.source) {
+						buffer->CheckBytes(2 + 4 + 1);
-						fs = &(*(st->goods[i].flows.insert(st->goods[i].flows.end(), FlowStat(flow.source, flow.via, flow.share, flow.restricted))));
+						flow.via = buffer->RawReadUint16();
-					} else {
+						flow.share = buffer->RawReadUint32();
-						fs->AppendShare(flow.via, flow.share, flow.restricted);
+						flow.restricted = (buffer->RawReadByte() != 0);
+						FlowStat *fs = &(*(st->goods[i].flows.insert(st->goods[i].flows.end(), FlowStat(flow.source, flow.via, flow.share, flow.restricted))));
+						for (uint32 k = 1; k < flow_count; ++k) {
+							buffer->CheckBytes(2 + 4 + 1);
+							flow.via = buffer->RawReadUint16();
+							flow.share = buffer->RawReadUint32();
+							flow.restricted = (buffer->RawReadByte() != 0);
+							fs->AppendShare(flow.via, flow.share, flow.restricted);
+						}
+						fs->SetRawFlags(SlReadUint16());
+					}
+				} else {
+					FlowSaveLoad flow;
+					FlowStat *fs = nullptr;
+					for (uint32 j = 0; j < _num_flows; ++j) {
+						// SlObject(&flow, _flow_desc); /* this is highly performance-sensitive, manually unroll */
+						buffer->CheckBytes(2 + 2 + 4);
+						flow.source = buffer->RawReadUint16();
+						flow.via = buffer->RawReadUint16();
+						flow.share = buffer->RawReadUint32();
+						if (!IsSavegameVersionBefore(SLV_187)) flow.restricted = (buffer->ReadByte() != 0);
+
+						if (fs == nullptr || prev_source != flow.source) {
+							fs = &(*(st->goods[i].flows.insert(st->goods[i].flows.end(), FlowStat(flow.source, flow.via, flow.share, flow.restricted))));
+						} else {
+							fs->AppendShare(flow.via, flow.share, flow.restricted);
+						}
+						prev_source = flow.source;
 					}
-					prev_source = flow.source;
 				}
 				if (IsSavegameVersionBefore(SLV_183)) {
 					SwapPackets(&st->goods[i]);

src/station_base.h

@@ -93,6 +93,7 @@ public:
 		this->unrestricted = restricted ? 0 : flow;
 		this->count = 1;
 		this->origin = origin;
+		this->flags = 0;
 	}
 
 private:
@@ -117,6 +118,7 @@ private:
 			free(this->storage.ptr_shares.buffer);
 		}
 		this->count = 0;
+		this->flags = 0;
 	}
 
 	iterator erase_item(iterator iter, uint flow_reduction);
@@ -131,6 +133,7 @@ private:
 		MemCpyT(this->data(), other.data(), this->count);
 		this->unrestricted = other.unrestricted;
 		this->origin = other.origin;
+		this->flags = other.flags;
 	}
 
 public:
@@ -234,6 +237,7 @@ public:
 		std::swap(this->storage, other.storage);
 		std::swap(this->unrestricted, other.unrestricted);
 		std::swap(this->count, other.count);
+		std::swap(this->flags, other.flags);
 	}
 
 	/**
@@ -269,13 +273,39 @@ public:
 
 	StationID GetVia(StationID excluded, StationID excluded2 = INVALID_STATION) const;
 
-	void Invalidate();
+	/**
+	 * Mark this flow stat as invalid, such that it is not included in link statistics.
+	 * @return True if the flow stat should be deleted.
+	 */
+	inline bool Invalidate()
+	{
+		if ((this->flags & 0x1F) == 0x1F) return true;
+		this->flags++;
+		return false;
+	}
 
 	inline StationID GetOrigin() const
 	{
 		return this->origin;
 	}
 
+	inline bool IsInvalid() const
+	{
+		return (this->flags & 0x1F) != 0;
+	}
+
+	/* for save/load use only */
+	inline uint16 GetRawFlags() const
+	{
+		return this->flags;
+	}
+
+	/* for save/load use only */
+	inline void SetRawFlags(uint16 flags)
+	{
+		this->flags = flags;;
+	}
+
 private:
 	uint32 GetLastKey() const
 	{
@@ -302,10 +332,11 @@ private:
 	uint unrestricted; ///< Limit for unrestricted shares.
 	uint16 count;
 	StationID origin;
+	uint16 flags;
 };
 static_assert(std::is_nothrow_move_constructible<FlowStat>::value, "FlowStat must be nothrow move constructible");
 #if OTTD_ALIGNMENT == 0 && (defined(__GNUC__) || defined(__clang__))
-static_assert(sizeof(FlowStat) == 20, "");
+static_assert(sizeof(FlowStat) == 24, "");
 #endif
 
 template<typename cv_value, typename cv_container, typename cv_index_iter>
@@ -397,7 +428,12 @@ public:
 
 	bool empty() const
 	{
-		return this->flows_index.empty();
+		return this->flows_storage.empty();
+	}
+
+	size_t size() const
+	{
+		return this->flows_storage.size();
 	}
 
 	void erase(StationID st)

src/station_cmd.cpp

@@ -4713,22 +4713,6 @@ StationID FlowStat::GetVia(StationID excluded, StationID excluded2) const
 	return it3->second;
 }
 
-/**
- * Reduce all flows to minimum capacity so that they don't get in the way of
- * link usage statistics too much. Keep them around, though, to continue
- * routing any remaining cargo.
- */
-void FlowStat::Invalidate()
-{
-	assert(!this->empty());
-	uint i = 0;
-	for (iterator it(this->begin()); it != this->end(); ++it) {
-		if (it->first == this->unrestricted) this->unrestricted = i + 1;
-		it->first = ++i;
-	}
-	assert(!this->empty() && this->unrestricted <= (this->end() - 1)->first);
-}
-
 /**
  * Change share for specified station. By specifying INT_MIN as parameter you
  * can erase a share. Newly added flows will be unrestricted.
@@ -4950,6 +4934,7 @@ uint FlowStatMap::GetFlow() const
 {
 	uint ret = 0;
 	for (FlowStatMap::const_iterator i = this->begin(); i != this->end(); ++i) {
+		if (i->IsInvalid()) continue;
 		ret += (i->end() - 1)->first;
 	}
 	return ret;
@@ -4964,6 +4949,7 @@ uint FlowStatMap::GetFlowVia(StationID via) const
 {
 	uint ret = 0;
 	for (FlowStatMap::const_iterator i = this->begin(); i != this->end(); ++i) {
+		if (i->IsInvalid()) continue;
 		ret += i->GetShare(via);
 	}
 	return ret;
@@ -4978,6 +4964,7 @@ uint FlowStatMap::GetFlowFrom(StationID from) const
 {
 	FlowStatMap::const_iterator i = this->find(from);
 	if (i == this->end()) return 0;
+	if (i->IsInvalid()) return 0;
 	return (i->end() - 1)->first;
 }
 
@@ -4991,6 +4978,7 @@ uint FlowStatMap::GetFlowFromVia(StationID from, StationID via) const
 {
 	FlowStatMap::const_iterator i = this->find(from);
 	if (i == this->end()) return 0;
+	if (i->IsInvalid()) return 0;
 	return i->GetShare(via);
 }
 
@@ -5010,12 +4998,14 @@ void FlowStatMap::SortStorage()
 void DumpStationFlowStats(char *b, const char *last)
 {
 	btree::btree_map<uint, uint> count_map;
+	btree::btree_map<uint, uint> invalid_map;
 	const Station *st;
 	FOR_ALL_STATIONS(st) {
 		for (CargoID i = 0; i < NUM_CARGO; i++) {
 			const GoodsEntry &ge = st->goods[i];
 			for (FlowStatMap::const_iterator it(ge.flows.begin()); it != ge.flows.end(); ++it) {
 				count_map[(uint32)it->size()]++;
+				invalid_map[it->GetRawFlags() & 0x1F]++;
 			}
 		}
 	}
@@ -5023,6 +5013,10 @@ void DumpStationFlowStats(char *b, const char *last)
 	for (const auto &it : count_map) {
 		b += seprintf(b, last, "%-5u %-5u\n", it.first, it.second);
 	}
+	b += seprintf(b, last, "Flow state shares invalid state distribution:\n");
+	for (const auto &it : invalid_map) {
+		b += seprintf(b, last, "%-2u %-5u\n", it.first, it.second);
+	}
 }
 
 extern const TileTypeProcs _tile_type_station_procs = {

src/station_gui.cpp

@@ -1594,6 +1594,7 @@ struct StationViewWindow : public Window {
 	{
 		const CargoDataEntry *source_dest = this->cached_destinations.Retrieve(i);
 		for (FlowStatMap::const_iterator it = flows.begin(); it != flows.end(); ++it) {
+			if (it->IsInvalid()) continue;
 			StationID from = it->GetOrigin();
 			const CargoDataEntry *source_entry = source_dest->Retrieve(from);
 			for (FlowStat::const_iterator flow_it = it->begin(); flow_it != it->end(); ++flow_it) {