From ce622e1852809c6a272402a66e8b4c35b3b82474 Mon Sep 17 00:00:00 2001 From: Pavel Solodovnikov Date: Tue, 12 Mar 2024 01:01:11 +0300 Subject: [PATCH] Switch `STRUCTURE` to use `PagedEntityContainer` as backing storage 1. Split the structure storage into pages containing 256 structs 2. Disable slot reuse to guard against memory-related issues when some object pointers won't get updated properly, e.g. when transitioning between the base and offworld missions. Signed-off-by: Pavel Solodovnikov --- src/display3d.cpp | 9 ++++- src/objmem.cpp | 69 ++++++++++++++++++++++++++++------ src/structure.cpp | 95 ++++++++++++++++++++++++----------------------- src/structure.h | 6 +++ 4 files changed, 119 insertions(+), 60 deletions(-) diff --git a/src/display3d.cpp b/src/display3d.cpp index 6f32364d1c0..b6c01662d9a 100644 --- a/src/display3d.cpp +++ b/src/display3d.cpp @@ -363,7 +363,8 @@ struct Blueprint STRUCTURE *psStruct = buildBlueprint(); ASSERT_OR_RETURN(, psStruct != nullptr, "buildBlueprint returned nullptr"); renderStructure(psStruct, viewMatrix, perspectiveViewMatrix); - delete psStruct; + + objmemDestroy(psStruct, false); } } @@ -692,7 +693,11 @@ STRUCTURE *getTileBlueprintStructure(int mapX, int mapY) Blueprint blueprint = getTileBlueprint(mapX, mapY); if (blueprint.state == SS_BLUEPRINT_PLANNED) { - delete psStruct; // Delete previously returned structure, if any. + if (psStruct) + { + // Delete previously returned structure, if any. + objmemDestroy(psStruct, false); + } psStruct = blueprint.buildBlueprint(); return psStruct; // This blueprint was clicked on. } diff --git a/src/objmem.cpp b/src/objmem.cpp index 12c3457fbf3..0a12bb8b345 100644 --- a/src/objmem.cpp +++ b/src/objmem.cpp @@ -226,14 +226,22 @@ bool objmemDestroy(BASE_OBJECT *psObj, bool checkRefs) { return false; } - // Droids are managed by a separate droid container. if (psObj->type == OBJ_DROID) { + // Droids are managed by a separate droid container. auto& droidContainer = GlobalDroidContainer(); auto it = droidContainer.find(*static_cast(psObj)); ASSERT(it != droidContainer.end(), "Droid not found in the global container!"); droidContainer.erase(it); } + else if (psObj->type == OBJ_STRUCTURE) + { + // Structs are managed by a separate struct container. + auto& structContainer = GlobalStructContainer(); + auto it = structContainer.find(*static_cast(psObj)); + ASSERT(it != structContainer.end(), "Structure not found in the global container!"); + structContainer.erase(it); + } else { delete psObj; @@ -452,16 +460,53 @@ void killDroid(DROID *psDel) destroyObject(apsDroidLists, psDel); } -static void freeAllDroidsImpl(PerPlayerDroidLists& droidLists) +template +struct GlobalEntityContainerTraits; + +template <> +struct GlobalEntityContainerTraits { - auto& droidContainer = GlobalDroidContainer(); - for (auto& list : droidLists) + using StorageType = DroidContainer; + + static DroidContainer& getContainer() + { + return GlobalDroidContainer(); + } + + static const char* entityName() + { + return "Droid"; + } +}; + +template <> +struct GlobalEntityContainerTraits +{ + using StorageType = StructContainer; + + static StructContainer& getContainer() + { + return GlobalStructContainer(); + } + + static const char* entityName() + { + return "Structure"; + } +}; + +template +static void freeAllEntitiesImpl(PerPlayerObjectLists& entityLists) +{ + using Traits = GlobalEntityContainerTraits; + auto& entityContainer = Traits::getContainer(); + for (auto& list : entityLists) { - for (DROID* d : list) + for (auto* ent : list) { - auto it = droidContainer.find(*d); - ASSERT(it != droidContainer.end(), "Droid not found in the global container!"); - droidContainer.erase(it); + auto it = entityContainer.find(*ent); + ASSERT(it != entityContainer.end(), "%s not found in the global container!", Traits::entityName()); + entityContainer.erase(it); } list.clear(); } @@ -470,7 +515,7 @@ static void freeAllDroidsImpl(PerPlayerDroidLists& droidLists) /* Remove all droids */ void freeAllDroids() { - freeAllDroidsImpl(apsDroidLists); + freeAllEntitiesImpl(apsDroidLists); } /*Remove a single Droid from a list*/ @@ -503,13 +548,13 @@ void removeDroid(DROID* psDroidToRemove, PerPlayerDroidLists& pList) /*Removes all droids that may be stored in the mission lists*/ void freeAllMissionDroids() { - freeAllDroidsImpl(mission.apsDroidLists); + freeAllEntitiesImpl(mission.apsDroidLists); } /*Removes all droids that may be stored in the limbo lists*/ void freeAllLimboDroids() { - freeAllDroidsImpl(apsLimboDroids); + freeAllEntitiesImpl(apsLimboDroids); } /************************** STRUCTURE *******************************/ @@ -592,7 +637,7 @@ void killStruct(STRUCTURE *psBuilding) /* Remove heapall structures */ void freeAllStructs() { - releaseAllObjectsInList(apsStructLists); + freeAllEntitiesImpl(apsStructLists); } /*Remove a single Structure from a list*/ diff --git a/src/structure.cpp b/src/structure.cpp index 450176a9da4..ac2ca417264 100644 --- a/src/structure.cpp +++ b/src/structure.cpp @@ -1414,31 +1414,27 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y } } - // allocate memory for and initialize a structure object - psBuilding = new STRUCTURE(id, player); - if (psBuilding == nullptr) - { - return nullptr; - } + // initialize the structure object + STRUCTURE building(id, player); //fill in other details - psBuilding->pStructureType = pStructureType; + building.pStructureType = pStructureType; - psBuilding->pos.x = x; - psBuilding->pos.y = y; - psBuilding->rot.direction = snapDirection(direction); - psBuilding->rot.pitch = 0; - psBuilding->rot.roll = 0; + building.pos.x = x; + building.pos.y = y; + building.rot.direction = snapDirection(direction); + building.rot.pitch = 0; + building.rot.roll = 0; //This needs to be done before the functionality bit... //load into the map data and structure list if not an upgrade Vector2i map = map_coord(Vector2i(x, y)) - size / 2; //set up the imd to use for the display - psBuilding->sDisplay.imd = pStructureType->pIMD[0]; + building.sDisplay.imd = pStructureType->pIMD[0]; - psBuilding->state = SAS_NORMAL; - psBuilding->lastStateTime = gameTime; + building.state = SAS_NORMAL; + building.lastStateTime = gameTime; /* if resource extractor - need to remove oil feature first, but do not do any * consistency checking here - save games do not have any feature to remove @@ -1452,7 +1448,6 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y if (fireOnLocation(psFeature->pos.x, psFeature->pos.y)) { // Can't build on burning oil resource - delete psBuilding; return nullptr; } // remove it from the map @@ -1486,16 +1481,18 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y #if defined(WZ_CC_GNU) && !defined(WZ_CC_INTEL) && !defined(WZ_CC_CLANG) && (7 <= __GNUC__) # pragma GCC diagnostic pop #endif - delete psBuilding; return nullptr; } } } + // Emplace the structure being built in the global storage to obtain stable address. + STRUCTURE& stableBuilding = GlobalStructContainer().emplace(std::move(building)); + psBuilding = &stableBuilding; for (int tileY = map.y; tileY < map.y + size.y; ++tileY) { for (int tileX = map.x; tileX < map.x + size.x; ++tileX) { - // We now know the previous loop didn't return early, so it is safe to save references to psBuilding now. + // We now know the previous loop didn't return early, so it is safe to save references to `stableBuilding` now. MAPTILE *psTile = mapTile(tileX, tileY); psTile->psObject = psBuilding; @@ -1612,7 +1609,7 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y if (!setFunctionality(psBuilding, pStructureType->type)) { removeStructFromMap(psBuilding); - delete psBuilding; + objmemDestroy(psBuilding, false); //better reset these if you couldn't build the structure! if (FromSave && player == selectedPlayer && missionLimboExpand()) { @@ -1834,8 +1831,6 @@ STRUCTURE *buildStructureDir(STRUCTURE_STATS *pStructureType, UDWORD x, UDWORD y STRUCTURE *buildBlueprint(STRUCTURE_STATS const *psStats, Vector3i pos, uint16_t direction, unsigned moduleIndex, STRUCT_STATES state, uint8_t ownerPlayer) { - STRUCTURE *blueprint = nullptr; - ASSERT_OR_RETURN(nullptr, psStats != nullptr, "No blueprint stats"); ASSERT_OR_RETURN(nullptr, psStats->pIMD[0] != nullptr, "No blueprint model for %s", getStatsName(psStats)); ASSERT_OR_RETURN(nullptr, ownerPlayer < MAX_PLAYERS, "invalid ownerPlayer: %" PRIu8 "", ownerPlayer); @@ -1866,55 +1861,57 @@ STRUCTURE *buildBlueprint(STRUCTURE_STATS const *psStats, Vector3i pos, uint16_t } } - blueprint = new STRUCTURE(0, ownerPlayer); + STRUCTURE blueprint(0, ownerPlayer); // construct the fake structure - blueprint->pStructureType = const_cast(psStats); // Couldn't be bothered to fix const correctness everywhere. + blueprint.pStructureType = const_cast(psStats); // Couldn't be bothered to fix const correctness everywhere. if (selectedPlayer < MAX_PLAYERS) { - blueprint->visible[selectedPlayer] = UBYTE_MAX; + blueprint.visible[selectedPlayer] = UBYTE_MAX; } - blueprint->sDisplay.imd = (*pIMD)[std::min(moduleNumber, pIMD->size() - 1)]; - blueprint->pos = pos; - blueprint->rot = rot; - blueprint->selected = false; + blueprint.sDisplay.imd = (*pIMD)[std::min(moduleNumber, pIMD->size() - 1)]; + blueprint.pos = pos; + blueprint.rot = rot; + blueprint.selected = false; - blueprint->numWeaps = 0; - blueprint->asWeaps[0].nStat = 0; + blueprint.numWeaps = 0; + blueprint.asWeaps[0].nStat = 0; // give defensive structures a weapon if (psStats->psWeapStat[0]) { - blueprint->asWeaps[0].nStat = psStats->psWeapStat[0] - asWeaponStats.data(); + blueprint.asWeaps[0].nStat = psStats->psWeapStat[0] - asWeaponStats.data(); } // things with sensors or ecm (or repair facilities) need these set, even if they have no official weapon - blueprint->numWeaps = 0; - blueprint->asWeaps[0].lastFired = 0; - blueprint->asWeaps[0].rot.pitch = 0; - blueprint->asWeaps[0].rot.direction = 0; - blueprint->asWeaps[0].rot.roll = 0; - blueprint->asWeaps[0].prevRot = blueprint->asWeaps[0].rot; + blueprint.numWeaps = 0; + blueprint.asWeaps[0].lastFired = 0; + blueprint.asWeaps[0].rot.pitch = 0; + blueprint.asWeaps[0].rot.direction = 0; + blueprint.asWeaps[0].rot.roll = 0; + blueprint.asWeaps[0].prevRot = blueprint.asWeaps[0].rot; - blueprint->expectedDamage = 0; + blueprint.expectedDamage = 0; // Times must be different, but don't otherwise matter. - blueprint->time = 23; - blueprint->prevTime = 42; + blueprint.time = 23; + blueprint.prevTime = 42; - blueprint->status = state; + blueprint.status = state; // Rotate wall if needed. - if (blueprint->pStructureType->type == REF_WALL || blueprint->pStructureType->type == REF_GATE) + if (blueprint.pStructureType->type == REF_WALL || blueprint.pStructureType->type == REF_GATE) { - WallOrientation scanType = structChooseWallTypeBlueprint(map_coord(blueprint->pos.xy())); + WallOrientation scanType = structChooseWallTypeBlueprint(map_coord(blueprint.pos.xy())); unsigned type = wallType(scanType); if (scanType != WallConnectNone) { - blueprint->rot.direction = wallDir(scanType); - blueprint->sDisplay.imd = blueprint->pStructureType->pIMD[std::min(type, blueprint->pStructureType->pIMD.size() - 1)]; + blueprint.rot.direction = wallDir(scanType); + blueprint.sDisplay.imd = blueprint.pStructureType->pIMD[std::min(type, blueprint.pStructureType->pIMD.size() - 1)]; } } - return blueprint; + auto& stableBlueprint = GlobalStructContainer().emplace(std::move(blueprint)); + + return &stableBlueprint; } static Vector2i defaultAssemblyPointPos(STRUCTURE *psBuilding) @@ -7216,3 +7213,9 @@ LineBuild calcLineBuild(STRUCTURE_STATS const *stats, uint16_t direction, Vector { return calcLineBuild(stats->size(direction), stats->type, pos, pos2); } + +StructContainer& GlobalStructContainer() +{ + static StructContainer instance; + return instance; +} diff --git a/src/structure.h b/src/structure.h index 8056d7491ce..efe34f9bec1 100644 --- a/src/structure.h +++ b/src/structure.h @@ -496,4 +496,10 @@ struct LineBuild LineBuild calcLineBuild(Vector2i size, STRUCTURE_TYPE type, Vector2i pos, Vector2i pos2); LineBuild calcLineBuild(STRUCTURE_STATS const *stats, uint16_t direction, Vector2i pos, Vector2i pos2); +// Split the struct storage into pages containing 256 structs, disable slot reuse +// to guard against memory-related issues when some object pointers won't get +// updated properly, e.g. when transitioning between the base and offworld missions. +using StructContainer = PagedEntityContainer; +StructContainer& GlobalStructContainer(); + #endif // __INCLUDED_SRC_STRUCTURE_H__