From b6e9817edbea4d23cf9480f9958ded45b86ff41f Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Sun, 20 Oct 2019 07:46:21 +0100 Subject: [PATCH] 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 | 9 +++-- src/saveload/extended_ver_sl.cpp | 1 + src/saveload/extended_ver_sl.h | 1 + src/saveload/station_sl.cpp | 63 ++++++++++++++++++++++---------- src/station_base.h | 42 +++++++++++++++++++-- src/station_cmd.cpp | 26 +++++-------- src/station_gui.cpp | 1 + 7 files changed, 101 insertions(+), 42 deletions(-) diff --git a/src/linkgraph/linkgraphjob.cpp b/src/linkgraph/linkgraphjob.cpp index 588d1b5ef7..29e0133bd8 100644 --- a/src/linkgraph/linkgraphjob.cpp +++ b/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(); - ++it; - } else { + should_erase = it->Invalidate(); + } + 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); diff --git a/src/saveload/extended_ver_sl.cpp b/src/saveload/extended_ver_sl.cpp index d0fa6b952f..3abd56de61 100644 --- a/src/saveload/extended_ver_sl.cpp +++ b/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 }; diff --git a/src/saveload/extended_ver_sl.h b/src/saveload/extended_ver_sl.h index 7ae414f9ab..4dd8de2800 100644 --- a/src/saveload/extended_ver_sl.h +++ b/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 diff --git a/src/saveload/station_sl.cpp b/src/saveload/station_sl.cpp index 99d4e7c85f..884e4d9d4e 100644 --- a/src/saveload/station_sl.cpp +++ b/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; - for (FlowStatMap::const_iterator it(st->goods[i].flows.begin()); it != st->goods[i].flows.end(); ++it) { - _num_flows += (uint32)it->size(); - } + _num_flows = st->goods[i].flows.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->RawWriteUint16(flow.source); + dumper->CheckBytes(2 + 4 + 1); 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(&(*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) { - // 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 (SlXvIsFeaturePresent(XSLFI_FLOW_STAT_FLAGS)) { + for (uint32 j = 0; j < _num_flows; ++j) { + FlowSaveLoad flow; + buffer->CheckBytes(2 + 4); + flow.source = buffer->RawReadUint16(); + uint32 flow_count = buffer->RawReadUint32(); - 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); + buffer->CheckBytes(2 + 4 + 1); + flow.via = buffer->RawReadUint16(); + flow.share = buffer->RawReadUint32(); + 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]); diff --git a/src/station_base.h b/src/station_base.h index 793eb99046..05a113c04c 100644 --- a/src/station_base.h +++ b/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::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 @@ -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) diff --git a/src/station_cmd.cpp b/src/station_cmd.cpp index d119341093..37751da55f 100644 --- a/src/station_cmd.cpp +++ b/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 count_map; + btree::btree_map 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 = { diff --git a/src/station_gui.cpp b/src/station_gui.cpp index 4589704ff9..9786caf2f5 100644 --- a/src/station_gui.cpp +++ b/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) {