Skip to content

Commit a8d11c6

Browse files
committed
addressing more comments.
1 parent b4d6226 commit a8d11c6

File tree

6 files changed

+42
-91
lines changed

6 files changed

+42
-91
lines changed

attachments/simple_engine/engine.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ class Engine {
261261

262262
PhysicsScaling physicsScaling;
263263

264-
// Pending ball creation data (to avoid memory pool constraints during rendering)
264+
// Pending ball creation data
265265
struct PendingBall {
266266
glm::vec3 spawnPosition;
267267
glm::vec3 throwDirection;

attachments/simple_engine/memory_pool.cpp

Lines changed: 32 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -23,17 +23,15 @@ bool MemoryPool::initialize() {
2323
PoolType::VERTEX_BUFFER,
2424
128 * 1024 * 1024, // 128MB blocks (doubled)
2525
4096, // 4KB allocation units
26-
vk::MemoryPropertyFlagBits::eDeviceLocal,
27-
16 // Max 16 blocks (2GB total, quadrupled)
26+
vk::MemoryPropertyFlagBits::eDeviceLocal
2827
);
2928

3029
// Index buffer pool: Medium allocations, device-local (increased for large models like bistro)
3130
configurePool(
3231
PoolType::INDEX_BUFFER,
3332
64 * 1024 * 1024, // 64MB blocks (doubled)
3433
2048, // 2KB allocation units
35-
vk::MemoryPropertyFlagBits::eDeviceLocal,
36-
8 // Max 8 blocks (512MB total, quadrupled)
34+
vk::MemoryPropertyFlagBits::eDeviceLocal
3735
);
3836

3937
// Uniform buffer pool: Small allocations, host-visible
@@ -42,8 +40,7 @@ bool MemoryPool::initialize() {
4240
PoolType::UNIFORM_BUFFER,
4341
4 * 1024 * 1024, // 4MB blocks
4442
64, // 64B allocation units (aligned to nonCoherentAtomSize)
45-
vk::MemoryPropertyFlagBits::eHostVisible | vk::MemoryPropertyFlagBits::eHostCoherent,
46-
4 // Max 4 blocks (16MB total)
43+
vk::MemoryPropertyFlagBits::eHostVisible | vk::MemoryPropertyFlagBits::eHostCoherent
4744
);
4845

4946
// Staging buffer pool: Variable allocations, host-visible
@@ -52,17 +49,15 @@ bool MemoryPool::initialize() {
5249
PoolType::STAGING_BUFFER,
5350
16 * 1024 * 1024, // 16MB blocks
5451
64, // 64B allocation units (aligned to nonCoherentAtomSize)
55-
vk::MemoryPropertyFlagBits::eHostVisible | vk::MemoryPropertyFlagBits::eHostCoherent,
56-
4 // Max 4 blocks (64MB total)
52+
vk::MemoryPropertyFlagBits::eHostVisible | vk::MemoryPropertyFlagBits::eHostCoherent
5753
);
5854

5955
// Texture image pool: Large allocations, device-local (significantly increased for large models like bistro)
6056
configurePool(
6157
PoolType::TEXTURE_IMAGE,
6258
256 * 1024 * 1024, // 256MB blocks (doubled)
6359
4096, // 4KB allocation units
64-
vk::MemoryPropertyFlagBits::eDeviceLocal,
65-
24 // Max 24 blocks (6GB total, 6x increase)
60+
vk::MemoryPropertyFlagBits::eDeviceLocal
6661
);
6762

6863
return true;
@@ -76,14 +71,12 @@ void MemoryPool::configurePool(
7671
const PoolType poolType,
7772
const vk::DeviceSize blockSize,
7873
const vk::DeviceSize allocationUnit,
79-
const vk::MemoryPropertyFlags properties,
80-
const uint32_t maxBlocks) {
74+
const vk::MemoryPropertyFlags properties) {
8175

8276
PoolConfig config;
8377
config.blockSize = blockSize;
8478
config.allocationUnit = allocationUnit;
8579
config.properties = properties;
86-
config.maxBlocks = maxBlocks;
8780

8881
poolConfigs[poolType] = config;
8982
}
@@ -126,16 +119,16 @@ std::unique_ptr<MemoryPool::MemoryBlock> MemoryPool::createMemoryBlock(PoolType
126119

127120
uint32_t memoryTypeIndex = findMemoryType(memRequirements.memoryTypeBits, config.properties);
128121

129-
// Allocate the memory block
122+
// Allocate the memory block using the device-required size
130123
vk::MemoryAllocateInfo allocInfo{
131-
.allocationSize = blockSize,
124+
.allocationSize = memRequirements.size,
132125
.memoryTypeIndex = memoryTypeIndex
133126
};
134127

135128
// Create MemoryBlock with proper initialization to avoid default constructor issues
136129
auto block = std::unique_ptr<MemoryBlock>(new MemoryBlock{
137130
.memory = vk::raii::DeviceMemory(device, allocInfo),
138-
.size = blockSize,
131+
.size = memRequirements.size,
139132
.used = 0,
140133
.memoryTypeIndex = memoryTypeIndex,
141134
.isMapped = false,
@@ -147,24 +140,23 @@ std::unique_ptr<MemoryPool::MemoryBlock> MemoryPool::createMemoryBlock(PoolType
147140
// Map memory if it's host-visible
148141
block->isMapped = (config.properties & vk::MemoryPropertyFlagBits::eHostVisible) != vk::MemoryPropertyFlags{};
149142
if (block->isMapped) {
150-
block->mappedPtr = block->memory.mapMemory(0, blockSize);
143+
block->mappedPtr = block->memory.mapMemory(0, memRequirements.size);
151144
} else {
152145
block->mappedPtr = nullptr;
153146
}
154147

155-
// Initialize a free list
156-
const size_t numUnits = blockSize / config.allocationUnit;
148+
// Initialize a free list based on the actual allocated size
149+
const size_t numUnits = static_cast<size_t>(block->size / config.allocationUnit);
157150
block->freeList.resize(numUnits, true); // All units initially free
158151

159152

160153
return block;
161154
}
162155

163-
MemoryPool::MemoryBlock* MemoryPool::findSuitableBlock(PoolType poolType, vk::DeviceSize size, vk::DeviceSize alignment) {
156+
std::pair<MemoryPool::MemoryBlock*, size_t> MemoryPool::findSuitableBlock(PoolType poolType, vk::DeviceSize size, vk::DeviceSize alignment) {
164157
auto poolIt = pools.find(poolType);
165158
if (poolIt == pools.end()) {
166-
pools[poolType] = std::vector<std::unique_ptr<MemoryBlock>>();
167-
poolIt = pools.find(poolType);
159+
poolIt = pools.try_emplace( poolType ).first;
168160
}
169161

170162
auto& poolBlocks = poolIt->second;
@@ -178,47 +170,39 @@ MemoryPool::MemoryBlock* MemoryPool::findSuitableBlock(PoolType poolType, vk::De
178170
for (const auto& block : poolBlocks) {
179171
// Find consecutive free units
180172
size_t consecutiveFree = 0;
173+
size_t startUnitCandidate = 0;
181174
for (size_t i = 0; i < block->freeList.size(); ++i) {
182175
if (block->freeList[i]) {
176+
if (consecutiveFree == 0) {
177+
startUnitCandidate = i;
178+
}
183179
consecutiveFree++;
184180
if (consecutiveFree >= requiredUnits) {
185-
return block.get();
181+
return {block.get(), startUnitCandidate};
186182
}
187183
} else {
188184
consecutiveFree = 0;
189185
}
190186
}
191187
}
192188

193-
// No suitable block found, create a new one if we haven't reached the limit AND rendering is not active
194-
if (poolBlocks.size() < config.maxBlocks) {
195-
if (renderingActive) {
196-
std::cerr << "ERROR: Attempted to create new memory block during rendering! Pool type: "
197-
<< static_cast<int>(poolType) << ", required size: " << alignedSize << std::endl;
198-
std::cerr << "This violates the constraint that no new memory should be allocated during PBR rendering." << std::endl;
199-
return nullptr;
200-
}
201-
202-
try {
203-
auto newBlock = createMemoryBlock(poolType, alignedSize);
204-
poolBlocks.push_back(std::move(newBlock));
205-
std::cout << "Created new memory block during initialization (pool type: "
206-
<< static_cast<int>(poolType) << ")" << std::endl;
207-
return poolBlocks.back().get();
208-
} catch (const std::exception& e) {
209-
std::cerr << "Failed to create new memory block: " << e.what() << std::endl;
210-
return nullptr;
211-
}
189+
// No suitable block found; create a new one on demand (no hard limits, allowed during rendering)
190+
try {
191+
auto newBlock = createMemoryBlock(poolType, alignedSize);
192+
poolBlocks.push_back(std::move(newBlock));
193+
std::cout << "Created new memory block (pool type: "
194+
<< static_cast<int>(poolType) << ")" << std::endl;
195+
return {poolBlocks.back().get(), 0};
196+
} catch (const std::exception& e) {
197+
std::cerr << "Failed to create new memory block: " << e.what() << std::endl;
198+
return {nullptr, 0};
212199
}
213-
214-
std::cerr << "Memory pool exhausted for pool type " << static_cast<int>(poolType) << std::endl;
215-
return nullptr;
216200
}
217201

218202
std::unique_ptr<MemoryPool::Allocation> MemoryPool::allocate(PoolType poolType, vk::DeviceSize size, vk::DeviceSize alignment) {
219203
std::lock_guard<std::mutex> lock(poolMutex);
220204

221-
MemoryBlock* block = findSuitableBlock(poolType, size, alignment);
205+
auto [block, startUnit] = findSuitableBlock(poolType, size, alignment);
222206
if (!block) {
223207
return nullptr;
224208
}
@@ -229,30 +213,6 @@ std::unique_ptr<MemoryPool::Allocation> MemoryPool::allocate(PoolType poolType,
229213
const vk::DeviceSize alignedSize = ((size + alignment - 1) / alignment) * alignment;
230214
const size_t requiredUnits = (alignedSize + config.allocationUnit - 1) / config.allocationUnit;
231215

232-
// Find consecutive free units
233-
size_t startUnit = 0;
234-
size_t consecutiveFree = 0;
235-
bool found = false;
236-
237-
for (size_t i = 0; i < block->freeList.size(); ++i) {
238-
if (block->freeList[i]) {
239-
if (consecutiveFree == 0) {
240-
startUnit = i;
241-
}
242-
consecutiveFree++;
243-
if (consecutiveFree >= requiredUnits) {
244-
found = true;
245-
break;
246-
}
247-
} else {
248-
consecutiveFree = 0;
249-
}
250-
}
251-
252-
if (!found) {
253-
return nullptr;
254-
}
255-
256216
// Mark units as used
257217
for (size_t i = startUnit; i < startUnit + requiredUnits; ++i) {
258218
block->freeList[i] = false;
@@ -427,14 +387,13 @@ bool MemoryPool::preAllocatePools() {
427387
std::lock_guard<std::mutex> lock(poolMutex);
428388

429389
try {
430-
std::cout << "Pre-allocating memory pools to prevent allocation during rendering..." << std::endl;
390+
std::cout << "Pre-allocating initial memory blocks for pools..." << std::endl;
431391

432392
// Pre-allocate at least one block for each pool type
433393
for (const auto& [poolType, config] : poolConfigs) {
434394
auto poolIt = pools.find(poolType);
435395
if (poolIt == pools.end()) {
436-
pools[poolType] = std::vector<std::unique_ptr<MemoryBlock>>();
437-
poolIt = pools.find(poolType);
396+
poolIt = pools.try_emplace( poolType ).first;
438397
}
439398

440399
auto& poolBlocks = poolIt->second;

attachments/simple_engine/memory_pool.h

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include <unordered_map>
77
#include <mutex>
88
#include <cstdint>
9+
#include <utility>
910

1011
/**
1112
* @brief Memory pool allocator for Vulkan resources
@@ -62,7 +63,6 @@ class MemoryPool {
6263
vk::DeviceSize blockSize; // Size of each memory block
6364
vk::DeviceSize allocationUnit; // Minimum allocation unit
6465
vk::MemoryPropertyFlags properties; // Memory properties
65-
uint32_t maxBlocks; // Maximum number of blocks
6666
};
6767

6868
// Memory pools for different types
@@ -72,13 +72,13 @@ class MemoryPool {
7272
// Thread safety
7373
mutable std::mutex poolMutex;
7474

75-
// Rendering state tracking to prevent pool growth during rendering
75+
// Optional rendering state flag (no allocation restrictions enforced)
7676
bool renderingActive = false;
7777

7878
// Helper methods
7979
uint32_t findMemoryType(uint32_t typeFilter, vk::MemoryPropertyFlags properties) const;
8080
std::unique_ptr<MemoryBlock> createMemoryBlock(PoolType poolType, vk::DeviceSize size);
81-
MemoryBlock* findSuitableBlock(PoolType poolType, vk::DeviceSize size, vk::DeviceSize alignment);
81+
std::pair<MemoryBlock*, size_t> findSuitableBlock(PoolType poolType, vk::DeviceSize size, vk::DeviceSize alignment);
8282

8383
public:
8484
/**
@@ -165,30 +165,28 @@ class MemoryPool {
165165
* @param blockSize Size of each memory block
166166
* @param allocationUnit Minimum allocation unit
167167
* @param properties Memory properties
168-
* @param maxBlocks Maximum number of blocks
169168
*/
170169
void configurePool(
171170
PoolType poolType,
172171
vk::DeviceSize blockSize,
173172
vk::DeviceSize allocationUnit,
174-
vk::MemoryPropertyFlags properties,
175-
uint32_t maxBlocks
173+
vk::MemoryPropertyFlags properties
176174
);
177175

178176
/**
179-
* @brief Pre-allocate memory pools to prevent allocation during rendering
177+
* @brief Pre-allocate initial memory blocks for configured pools
180178
* @return True if pre-allocation was successful
181179
*/
182180
bool preAllocatePools();
183181

184182
/**
185-
* @brief Set rendering active state to prevent pool growth
183+
* @brief Set rendering active state flag (informational only)
186184
* @param active Whether rendering is currently active
187185
*/
188186
void setRenderingActive(bool active);
189187

190188
/**
191-
* @brief Check if rendering is currently active
189+
* @brief Check if rendering is currently active (informational only)
192190
* @return True if rendering is active
193191
*/
194192
bool isRenderingActive() const;

attachments/simple_engine/renderer_core.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ bool Renderer::Initialize(const std::string& appName, bool enableValidationLayer
8383
return false;
8484
}
8585

86-
// Pre-allocate memory pools to prevent allocation during rendering
86+
// Optionally pre-allocate initial memory blocks for pools
8787
if (!memoryPool->preAllocatePools()) {
8888
std::cerr << "Failed to pre-allocate memory pools" << std::endl;
8989
return false;

attachments/simple_engine/renderer_rendering.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -505,7 +505,7 @@ void Renderer::updateUniformBufferInternal(uint32_t currentImage, Entity* entity
505505

506506
// Render the scene
507507
void Renderer::Render(const std::vector<std::unique_ptr<Entity>>& entities, CameraComponent* camera, ImGuiSystem* imguiSystem) {
508-
// Set rendering active to prevent memory pool growth during rendering
508+
// Mark rendering as active (informational flag for systems that care)
509509
if (memoryPool) {
510510
memoryPool->setRenderingActive(true);
511511
}

attachments/simple_engine/renderer_resources.cpp

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1420,12 +1420,6 @@ std::pair<vk::raii::Buffer, vk::raii::DeviceMemory> Renderer::createBuffer(
14201420
throw std::runtime_error("Memory pool not available - cannot create buffer");
14211421
}
14221422

1423-
// Check if we're trying to allocate during rendering
1424-
if (memoryPool->isRenderingActive()) {
1425-
std::cerr << "ERROR: Attempted to create buffer during rendering! Size: " << size << " bytes" << std::endl;
1426-
std::cerr << "This violates the constraint that no new memory should be allocated during rendering." << std::endl;
1427-
throw std::runtime_error("Buffer creation attempted during rendering - this is not allowed");
1428-
}
14291423

14301424
// Only allow direct allocation for staging buffers (temporary, host-visible)
14311425
if (!(properties & vk::MemoryPropertyFlagBits::eHostVisible)) {

0 commit comments

Comments
 (0)