Repository: google/filament
File: filament/backend/src/vulkan/VulkanStagePool.cpp (C++)

Code before:

// Note: these are temporary values, they will be configurable.
static constexpr uint32_t MAX_EMPTY_STAGES_TO_RETAIN = 1;
constexpr uint32_t STAGE_SIZE = 1048576;

}// namespace

VulkanStage::~VulkanStage() {
    for (auto pair : mSegments) {
        pair.second->clearRecycleFn();
    }
}

fvkmemory::resource_ptr<VulkanStage::Segment> VulkanStage::acquireSegment(
        fvkmemory::ResourceManager* resManager, uint32_t numBytes) {
    auto segment = fvkmemory::resource_ptr<Segment>::construct(
        resManager, this, numBytes, mCurrentOffset, [this](uint32_t offset) {
            mSegments.erase(offset);
    });
    mSegments.insert({mCurrentOffset, segment.get()});
    mCurrentOffset += numBytes;
    return segment;
}

VulkanStagePool::VulkanStagePool(VmaAllocator allocator, fvkmemory::ResourceManager* resManager, VulkanCommands* commands)
    : mAllocator(allocator),
      mResManager(resManager),
      mCommands(commands) {}

fvkmemory::resource_ptr<VulkanStage::Segment> VulkanStagePool::acquireStage(uint32_t numBytes) {
    // First check if a stage segment exists whose capacity is greater than or
    // equal to the requested size.
    auto iter = mStages.lower_bound(numBytes);

    VulkanStage* pStage;
    if (iter != mStages.end()) {
        pStage = iter->second;
        mStages.erase(iter);
    } else {
        pStage = allocateNewStage(std::max(numBytes, STAGE_SIZE));
    }

    // Note: this allocation updates `currentOffset` and `segments` within
    // the parent stage. When destroyed, it will update `segments`.
    fvkmemory::resource_ptr<VulkanStage::Segment> pSegment = pStage->acquireSegment(mResManager, numBytes);

    // Update the stage's metadata, and reinsert it with the remaining segment
    // capacity.
    uint32_t spaceRemaining = pStage->capacity() - pStage->currentOffset();
    mStages.insert({ spaceRemaining, pStage });


Review comment (suggestion):
So I was reading about nonCoherentAtomSize in https://registry.khronos.org/vulkan/specs/latest/man/html/VkPhysicalDeviceLimits.html

"nonCoherentAtomSize is the size and alignment in bytes that bounds concurrent access to [host-mapped device memory](https://registry.khronos.org/vulkan/specs/latest/html/vkspec.html#memory-device-hostaccess). The value must be a power of two."

Do you think we need to this into account? I feel like we do but I'm not 100% sure about it.
its possible some adjacent allocations might be writing when another is being read and perhaps this will cause some kinda of problem? Buffer content should be fine since we are setting the proper offset and size

@phoenixxxx @poweifeng 



Code after:

// Note: these are temporary values, they will be configurable.
static constexpr uint32_t MAX_EMPTY_STAGES_TO_RETAIN = 1;
constexpr uint32_t STAGE_SIZE = 1048576;

}// namespace

fvkmemory::resource_ptr<VulkanStage::Segment> VulkanStage::acquireSegment(
        fvkmemory::ResourceManager* resManager, uint32_t numBytes) {
    auto segment = fvkmemory::resource_ptr<Segment>::construct(
        resManager, this, numBytes, mCurrentOffset, [this](uint32_t offset) {
            mSegments.erase(offset);
    });
    mSegments.insert({mCurrentOffset, segment.get()});
    mCurrentOffset += numBytes;
    return segment;
}

VulkanStagePool::VulkanStagePool(VmaAllocator allocator, fvkmemory::ResourceManager* resManager,
        VulkanCommands* commands, const VkPhysicalDeviceLimits* deviceLimits)
    : mAllocator(allocator),
      mResManager(resManager),
      mCommands(commands),
      mDeviceLimits(deviceLimits) {}

fvkmemory::resource_ptr<VulkanStage::Segment> VulkanStagePool::acquireStage(uint32_t numBytes) {
    // Apply alignment to the byte count to ensure that, when we later flush
    // data written by the host, we only flush the atoms that we modified, and
    // no adjacent atoms.
    numBytes = alignToNonCoherentAtomSize(numBytes);

    // First check if a stage segment exists whose capacity is greater than or
    // equal to the requested size.
    auto iter = mStages.lower_bound(numBytes);

    VulkanStage* pStage;
    if (iter != mStages.end()) {
        pStage = iter->second;
        mStages.erase(iter);
    } else {
        pStage = allocateNewStage(std::max(numBytes, STAGE_SIZE));
    }

    // Note: this allocation updates `currentOffset` and `segments` within
    // the parent stage. When destroyed, it will update `segments`.
    fvkmemory::resource_ptr<VulkanStage::Segment> pSegment = pStage->acquireSegment(mResManager, numBytes);

    // Update the stage's metadata, and reinsert it with the remaining segment
    // capacity.
    uint32_t spaceRemaining = pStage->capacity() - pStage->currentOffset();
    mStages.insert({ spaceRemaining, pStage });