Store the residency set in MVKPhysicalDevice. #2448
Conversation
Since the MTLQueues are shared between MVKDevices, an application that creates many devices (CTS, in particular) may run out of the number of residency sets that can be added to a single MTLQueue. This patch keeps a single residency set in MVKPhysicalDevice, which is added once to each queue and shared as the queues are. We could instead attach each MVKDevice's residency set to every created MTLCommandBuffer, but that may incur a performance overhead of processing the entries each time, which would affect all applications. Fixes KhronosGroup#2444
|
|
There was a problem hiding this comment.
Attaching once to the MTLQueue seems like the more efficient thing to do, but on the other hand Metal presumably caches things and does not do big intersections repeatedly when the same set is attached to all command buffers. If we care about having separate sets for each device that's something to consider.
I assume another way of fixing this would be to stick with your existing design around MVKDevice, and simply call removeResidencySet: in the MVKQueue destructor? That would be a good RAII complement to the existing addResidencySet: in the MVKQueue constructor, and would seem to better track to the idea of tracking separate sets per device.
If you want to stick with this design, around physical device, I've added some minor review recommendations.
Also, I'm not sure why it's asking you to renew your CLA, but please do so, before we can merge any changes.
| @@ -830,9 +839,9 @@ class MVKDevice : public MVKDispatchableVulkanAPIObject { | |||
| void makeResident(id allocation) {} | |||
| #else | |||
| void makeResident(id<MTLAllocation> allocation) { | |||
There was a problem hiding this comment.
Since we're moving the residency set to MVKPhysicalDevice, these functions will be more consistent if moved there too.
Then elsewhere, just call getPhysicalDevice()->makeResident() where you now call getDevice()->makeResident().
| #if MVK_XCODE_16 | ||
| if (_isUsingMetalArgumentBuffers && _metalFeatures.residencySets) { | ||
| MTLResidencySetDescriptor *setDescriptor; | ||
| setDescriptor = [MTLResidencySetDescriptor new]; |
There was a problem hiding this comment.
No biggie, but I'm never a fan of uninitialized vars. It would read nicer if these two lines were combined.
| _mtlResidencySet = [_mtlDevice newResidencySetWithDescriptor:setDescriptor | ||
| error:&error]; | ||
| if (error) { | ||
| reportMessage(MVK_CONFIG_LOG_LEVEL_ERROR, "Error allocating residency set: %s", error.description.UTF8String); |
There was a problem hiding this comment.
Prefer to use reportError(VK_ERROR_INITIALIZATION_FAILED, ...) instead.
Right, I guess I assumed the devices were created in parallel before I noticed the queues are shared, and they clearly just retain the sets. If we're OK with having the 32 live devices limit (which seems fine) that's better. I created a new PR, let's shelve this one for now. |
@billhollings Attaching once to the MTLQueue seems like the more efficient thing to do, but on the other hand Metal presumably caches things and does not do big intersections repeatedly when the same set is attached to all command buffers. If we care about having separate sets for each device that's something to consider.
Fixes #2444