DescriptorPool: O(1) recycled-descriptor accounting in available() - #1724
Conversation
|
I have merged this PR as a branch: origin/nolankramer-master I have tested on large 3D Tiles database that I have access to, it's a client dataset so I can't share it, and it initially works fine by then I get some rendering errors on some tiles and Vulkan debug errors: I see similar errors before with VSG master, but last month after lots of investigation made some revisions to SharedObjects usage to tighten up the thread control of state that address these issues, so apply this PR to master that has these changes has a regression back to what it looks like before these fixes. This PRs changes have no direct link, other than the state that SharedObjects manages at times, so it might be just a slight change in conditions that triggers a wider issue that my SharedObjects changes just disguised. |
|
I now don't think this PR is triggering the Vulkan debug errors, as I'm seeing them on VSG mast and this branch now I'm back using the older OS varant - Kubuntu 22.04. I had a disk failure with my Kubuntu 26.04 instance forcing me to juggle a bit. To help investigate the DescriptorPools costs addressed by this PR I've created a DescriptorPool_Metrics branch from nolankramer-master branch. In the modified src/vsg/vk/DescriptorPool.cpp file the new performance optimizations associated with this PR can be toggled on/off by setting NEW_OPTIMIZATION define to 0 or 1 i.e. Use the old DesccriptorPool approach: #define NEW_OPTIMIZATION 0Use the new DescriptorPool optimization from this PR: #define NEW_OPTIMIZATION 1The DescriptorPool constructor and desctructor also have some code to report some basic timing and usage stats that look like: info: DescriptorPool::DescriptorPool() 0x5985c5283a80 As part of this testing I have also modified vsgviewer and vsgXchange::Tile3D/gltf to allow me disable applying the default SharedObjects to vsg::Options as the 3D Tiles databases I was testing were no deleting the DesctriptorPool on exit suggesting some kinda of circular reference or uncleaned up objects. This is a separate issue I need to look into, but don't to check too much debugging hacks into vsgExamples/vsgXchange for now. I'll need to decide how best to proceed on that front. @nolankramer I'm hoping that you'll be able to check the DescriptorPool_Metrics branch and then test against your application and data so I can get a sense of just what the usage is on your case and the overheads of the new and old code paths in vsg::DescriptorPool. |
|
@robertosfield Thanks for looking into finding out if it was my change. Really appreciate that I'll build that branch into our app and get back to you with the before vs after. Since I opened this, our architecture has changed to pool descriptors more intelligently, but there should still be some useful data |
|
one approach I have been considering is to just free descriptor sets back
into the Vulkan descriptor pool rather than reuse them with the VSG's
scheme.
…On Mon, 17 Aug 2026, 21:01 Nolan Kramer, ***@***.***> wrote:
*nolankramer* left a comment (vsg-dev/VulkanSceneGraph#1724)
<#1724 (comment)>
@robertosfield <https://github.com/robertosfield> Thanks for looking into
finding out if it was my change. Really appreciate that
I'll build that branch into our app and get back to you with the before vs
after. Since I opened this, our architecture has changed to pool
descriptors more intelligently, but there should still be some useful data
—
Reply to this email directly, view it on GitHub
<#1724?email_source=notifications&email_token=AAKEGUDFBCR6Y7V3FSQEDGD5KNQBVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZRHE3DAMRWGMZKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5319602632>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKEGUHSMYT6JFCPF5Q4VXD5KNQBVAVCNFSNUABFKJSXA33TNF2G64TZHMYTIOBWGA4TAMBUHNEXG43VMU5TINZXGE2TAOBUGI4KC5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AAKEGUCG2DISP6TJIVBM5535KNQBVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZRHE3DAMRWGMZKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/AAKEGUA72WPK4EVZX3QQLID5KNQBVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZRHE3DAMRWGMZKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I have now merge this PR branch with VSG master. I made a tweak to unify some of the code to avoid duplication with commit: 81021ee. |
Description
available()walked the entire_recyclingListon every call to tally up the descriptors held in freed sets. Under high churn, scenes that stream in and evict a lot of geometry freed thousands of per-object descriptor sets. That list gets long, and sinceDescriptorPools::reserve()callsavailable()once per pool, compile time scales with the number of recycled sets. I saw it ramp past 10ms and stay there after a big spike.This fix keeps a running per-type total of what's in the recycling list, updated as sets are freed and reused, so
available()just reads it. O(number of descriptor types) instead of O(recycled sets).Does not change behavior - strictly a performance improvement for scenes the churn descriptor sets rapidly.
Also added a quick doc string to
compile()andcompileTask()that they don't throw, and users should instead expect to read the returnedCompileResultType of change
Please delete options that are not relevant.
How Has This Been Tested?
In my own app - works as-expected.
Test Configuration:
Checklist: