Make VulkanProgram a ThreadSafeResource#9788
Make VulkanProgram a ThreadSafeResource#9788anishmgoyal wants to merge 2 commits intogoogle:mainfrom
Conversation
anishmgoyal
force-pushed
the
makeProgramsThreadSafe
branch
from
f47200e to
71924d1
Compare
|
@poweifeng @phoenixxxx @pixelflinger @rafadevai fyi note - the VulkanProgram was moved from VulkanHandles to VulkanAsyncHandles to follow existing patterns. the only code change in the program class itself is the shift from using the base class Resource to using the base class ThreadSafeResource. |
anishmgoyal
changed the title
anishmgoyal
changed the title
| mResources.push_back(resource); | ||
| } | ||
|
|
||
| inline void acquire(fvkmemory::resource_ptr<fvkmemory::ThreadSafeResource> resource) { |
There was a problem hiding this comment.
why this is needed? is it for the VulkanProgram?
There was a problem hiding this comment.
yes. really, we just need the vector to hold resources, be they threadsafe or not. it doesn't interact with them in any other way other than to hold and later free.
There was a problem hiding this comment.
maybe use template as a shorthand.
|
Have we considered any solution that doesnt require the change to threadsaferesource? To me performance is always a big consideration, most use cases right now dont support parallel compilation, so they are going to pay a penalty here. |
this is a very fair point. i don't see a situation where we avoid locking though - the very nature of parallel compilation essentially requires that we're going to be operating on multiple threads when interacting with programs. even if we were to inc() / dec() on the main thread, we'd have to add a lock to every frame to check if there are pending tasks. there's no way to synchronize multiple threads without a lock or atomic of some sort. to your point, we are adding some atomic ref counts to every frame by doing this, which is a cost because that atomic will not make it into the cache. that said, we could prevent performance hits in synchronous compilation cases by having completely separate codepaths. one option is to have two separate program classes, one for parallel compilation and one for synchronous. |
| uint32_t mRangeCount; | ||
| }; | ||
|
|
||
| struct VulkanProgram : public HwProgram, fvkmemory::ThreadSafeResource { |
There was a problem hiding this comment.
Can you add a comment as to why this needs to be a ThreadSafeResource?
There was a problem hiding this comment.
Maybe also mention that all the methods still need to be called on the backend thread. "ThreadSafety" only pertains to the reference counter.
With the introduction of cache prewarming, handles for vulkan programs will be managed on a separate thread in the backend along with the main backend thread. This requires an atomic refcount to ensure accuracy and prevent race conditions.
anishmgoyal
force-pushed
the
makeProgramsThreadSafe
branch
from
71924d1 to
0a8abd6
Compare
4 participants
With the introduction of cache prewarming, handles for vulkan programs will be managed on a separate thread in the backend along with the main backend thread. This requires an atomic refcount to ensure accuracy and prevent race conditions.