[PATCH v4 0/1] MR384: vkd3d: Replace the descriptor object cache with a thread-local implementation.
The PE build uses FlsAlloc(), which for our purposes makes no difference vs TlsAlloc(), and allows the use of a destruction callback. -- v4: vkd3d: Replace the descriptor object cache with a thread-local implementation. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/384
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d/device.c | 102 +++++++++++++++++++++++++++++----- libs/vkd3d/resource.c | 110 ++++++++++++++++++++----------------- libs/vkd3d/vkd3d_private.h | 78 +++++++++++++++++++++++--- 3 files changed, 217 insertions(+), 73 deletions(-) diff --git a/libs/vkd3d/device.c b/libs/vkd3d/device.c index 2d7051f3..66703520 100644 --- a/libs/vkd3d/device.c +++ b/libs/vkd3d/device.c @@ -2434,28 +2434,92 @@ static void device_init_descriptor_pool_sizes(struct d3d12_device *device) device->vk_pool_count = 6; }; -static void vkd3d_desc_object_cache_init(struct vkd3d_desc_object_cache *cache, size_t size) +static void vkd3d_desc_object_cache_init(struct vkd3d_desc_object_cache *cache, size_t size, + struct desc_rebalance *rebalance) { - memset(cache, 0, sizeof(*cache)); cache->size = size; + cache->rebalance = rebalance; } static void vkd3d_desc_object_cache_cleanup(struct vkd3d_desc_object_cache *cache) { - union d3d12_desc_object u; - unsigned int i; - void *next; + size_t i; + + for (i = 0; i < cache->count; ++i) + vkd3d_free(cache->data[i]); + vkd3d_free(cache->data); +} + +static void desc_rebalance_init(struct desc_rebalance *rebalance) +{ + vkd3d_mutex_init(&rebalance->mutex); +} + +static +#ifdef _WIN32 +WINAPI +#endif +void global_tls_key_destroy(void *value) +{ + struct desc_object_caches *caches = value; - for (i = 0; i < ARRAY_SIZE(cache->heads); ++i) + if (caches) { - for (u.object = cache->heads[i].head; u.object; u.object = next) - { - next = u.header->next; - vkd3d_free(u.object); - } + vkd3d_desc_object_cache_cleanup(&caches->view_desc_cache); + vkd3d_desc_object_cache_cleanup(&caches->cbuffer_desc_cache); + vkd3d_free(caches); } } +struct desc_rebalance view_desc_rebalance; +struct desc_rebalance cbuffer_desc_rebalance; + +static int global_tls_key_get(struct vkd3d_tls_key *key) +{ + static struct vkd3d_mutex key_cache_mutex = {VKD3D_MUTEX_INITIALIZER}; + static struct vkd3d_tls_key tls_key; + static bool tls_initialised; + int rc = 0; + + vkd3d_mutex_lock(&key_cache_mutex); + + if (!tls_initialised) + { + rc = vkd3d_tls_key_create(&tls_key, global_tls_key_destroy); + desc_rebalance_init(&view_desc_rebalance); + desc_rebalance_init(&cbuffer_desc_rebalance); + tls_initialised = true; + } + + vkd3d_mutex_unlock(&key_cache_mutex); + + *key = tls_key; + return rc; +} + +struct desc_object_caches *device_get_desc_object_caches(struct d3d12_device *device) +{ + struct desc_object_caches *caches = vkd3d_tls_key_get_value(&device->tls_key); + + if (caches) + return caches; + + if (!(caches = vkd3d_calloc(1, sizeof(*caches)))) + { + ERR("Failed to allocate caches.\n"); + /* Returning null here is not worth the cost of handling it in callers. */ + abort(); + } + + vkd3d_desc_object_cache_init(&caches->view_desc_cache, sizeof(struct vkd3d_view), &view_desc_rebalance); + vkd3d_desc_object_cache_init(&caches->cbuffer_desc_cache, sizeof(struct vkd3d_cbuffer_desc), + &cbuffer_desc_rebalance); + + vkd3d_tls_key_set_value(&device->tls_key, caches); + + return caches; +} + /* ID3D12Device */ static inline struct d3d12_device *impl_from_ID3D12Device5(ID3D12Device5 *iface) { @@ -2520,8 +2584,6 @@ static ULONG STDMETHODCALLTYPE d3d12_device_Release(ID3D12Device5 *iface) vkd3d_render_pass_cache_cleanup(&device->render_pass_cache, device); d3d12_device_destroy_pipeline_cache(device); d3d12_device_destroy_vkd3d_queues(device); - vkd3d_desc_object_cache_cleanup(&device->view_desc_cache); - vkd3d_desc_object_cache_cleanup(&device->cbuffer_desc_cache); VK_CALL(vkDestroyDevice(device->vk_device, NULL)); if (device->parent) IUnknown_Release(device->parent); @@ -4234,6 +4296,7 @@ static HRESULT d3d12_device_init(struct d3d12_device *device, { const struct vkd3d_vk_device_procs *vk_procs; HRESULT hr; + int rc; device->ID3D12Device5_iface.lpVtbl = &d3d12_device_vtbl; device->refcount = 1; @@ -4276,8 +4339,15 @@ static HRESULT d3d12_device_init(struct d3d12_device *device, device->blocked_queue_count = 0; vkd3d_mutex_init(&device->blocked_queues_mutex); - vkd3d_desc_object_cache_init(&device->view_desc_cache, sizeof(struct vkd3d_view)); - vkd3d_desc_object_cache_init(&device->cbuffer_desc_cache, sizeof(struct vkd3d_cbuffer_desc)); + /* Store a copy of the TLS key in the device object. The object is often accessed + * for other reasons after the key is read, so reading it is less likely to cause + * an additional cache miss than reading the global value would be. */ + if ((rc = global_tls_key_get(&device->tls_key)) < 0) + { + ERR("Failed to allocate TLS key, rc %d\n.", rc); + hr = E_FAIL; + goto out_cleanup_descriptor_heap_layouts; + } device_init_descriptor_pool_sizes(device); @@ -4286,6 +4356,8 @@ static HRESULT d3d12_device_init(struct d3d12_device *device, return S_OK; +out_cleanup_descriptor_heap_layouts: + vkd3d_vk_descriptor_heap_layouts_cleanup(device); out_cleanup_uav_clear_state: vkd3d_uav_clear_state_cleanup(&device->uav_clear_state, device); out_destroy_null_resources: diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c index 8898c247..fc8a719b 100644 --- a/libs/vkd3d/resource.c +++ b/libs/vkd3d/resource.c @@ -2290,72 +2290,77 @@ ULONG vkd3d_resource_decref(ID3D12Resource *resource) return d3d12_resource_decref(impl_from_ID3D12Resource(resource)); } -#define HEAD_INDEX_MASK (ARRAY_SIZE(cache->heads) - 1) +static const unsigned int REBALANCE_SIZE = 64; +/* Rebalance threshold expands by one extra REBALANCE_SIZE per (threshold / REBALANCE_DIVISOR). */ +static const unsigned int REBALANCE_DIVISOR = 512; +/* Avoid some initial reallocations. */ +static const unsigned int REBALANCE_MIN_ALLOCATION = REBALANCE_SIZE * 16; -/* Objects are cached so that vkd3d_view_incref() can safely check the refcount of an - * object freed by another thread. This could be implemented as a single atomic linked - * list, but it requires handling the ABA problem, which brings issues with cross-platform - * support, compiler support, and non-universal x86-64 support for 128-bit CAS. */ static void *vkd3d_desc_object_cache_get(struct vkd3d_desc_object_cache *cache) { - union d3d12_desc_object u; - unsigned int i; + struct desc_rebalance *rebalance; + void *object; - STATIC_ASSERT(!(ARRAY_SIZE(cache->heads) & HEAD_INDEX_MASK)); + if (cache->count) + return cache->data[--cache->count]; - i = (vkd3d_atomic_increment(&cache->next_index)) & HEAD_INDEX_MASK; - for (;;) - { - if (vkd3d_atomic_compare_exchange(&cache->heads[i].spinlock, 0, 1)) - { - if ((u.object = cache->heads[i].head)) - { - vkd3d_atomic_decrement(&cache->free_count); - cache->heads[i].head = u.header->next; - vkd3d_atomic_exchange(&cache->heads[i].spinlock, 0); - return u.object; - } - vkd3d_atomic_exchange(&cache->heads[i].spinlock, 0); - } - /* Keeping a free count avoids uncertainty over when this loop should terminate, - * which could result in excess allocations gradually increasing without limit. */ - if (cache->free_count < ARRAY_SIZE(cache->heads)) - return vkd3d_malloc(cache->size); + /* If the cache is empty then a previously sent rebalance chunk should not have been sent. + * Raise the threshold for sending chunks. */ + cache->rebalance_threshold += cache->threshold_expansion; + cache->threshold_expansion = 0; - i = (i + 1) & HEAD_INDEX_MASK; + rebalance = cache->rebalance; + vkd3d_mutex_lock(&rebalance->mutex); + + if (rebalance->count) + { + assert(rebalance->count >= REBALANCE_SIZE); + rebalance->count -= REBALANCE_SIZE; + vkd3d_array_reserve((void **)&cache->data, &cache->capacity, REBALANCE_SIZE, sizeof(*cache->data)); + memcpy(cache->data, &rebalance->data[rebalance->count], REBALANCE_SIZE * sizeof(*cache->data)); + cache->count = REBALANCE_SIZE - 1; + object = cache->data[REBALANCE_SIZE - 1]; + } + else + { + object = vkd3d_malloc(cache->size); } + + vkd3d_mutex_unlock(&rebalance->mutex); + return object; } static void vkd3d_desc_object_cache_push(struct vkd3d_desc_object_cache *cache, void *object) { - union d3d12_desc_object u = {object}; - unsigned int i; - void *head; + struct desc_rebalance *rebalance; - /* Using the same index as above may result in a somewhat uneven distribution, - * but the main objective is to avoid costly spinlock contention. */ - i = (vkd3d_atomic_increment(&cache->next_index)) & HEAD_INDEX_MASK; - for (;;) - { - if (vkd3d_atomic_compare_exchange(&cache->heads[i].spinlock, 0, 1)) - break; - i = (i + 1) & HEAD_INDEX_MASK; - } + vkd3d_array_reserve((void **)&cache->data, &cache->capacity, max(cache->count + 1, REBALANCE_SIZE), + sizeof(*cache->data)); + cache->data[cache->count++] = object; - head = cache->heads[i].head; - u.header->next = head; - cache->heads[i].head = u.object; - vkd3d_atomic_exchange(&cache->heads[i].spinlock, 0); - vkd3d_atomic_increment(&cache->free_count); -} + if (cache->count < cache->rebalance_threshold + REBALANCE_SIZE) + return; -#undef HEAD_INDEX_MASK + cache->count -= REBALANCE_SIZE; + cache->threshold_expansion = REBALANCE_SIZE * (1 + cache->rebalance_threshold / REBALANCE_DIVISOR); + + rebalance = cache->rebalance; + vkd3d_mutex_lock(&rebalance->mutex); + + vkd3d_array_reserve((void **)&rebalance->data, &rebalance->capacity, + max(rebalance->count + REBALANCE_SIZE, REBALANCE_MIN_ALLOCATION), sizeof(*rebalance->data)); + memcpy(&rebalance->data[rebalance->count], &cache->data[cache->count], REBALANCE_SIZE * sizeof(*rebalance->data)); + rebalance->count += REBALANCE_SIZE; + + vkd3d_mutex_unlock(&rebalance->mutex); +} static struct vkd3d_cbuffer_desc *vkd3d_cbuffer_desc_create(struct d3d12_device *device) { + struct desc_object_caches *caches = device_get_desc_object_caches(device); struct vkd3d_cbuffer_desc *desc; - if (!(desc = vkd3d_desc_object_cache_get(&device->cbuffer_desc_cache))) + if (!(desc = vkd3d_desc_object_cache_get(&caches->cbuffer_desc_cache))) return NULL; desc->h.magic = VKD3D_DESCRIPTOR_MAGIC_CBV; @@ -2368,11 +2373,12 @@ static struct vkd3d_cbuffer_desc *vkd3d_cbuffer_desc_create(struct d3d12_device static struct vkd3d_view *vkd3d_view_create(uint32_t magic, VkDescriptorType vk_descriptor_type, enum vkd3d_view_type type, struct d3d12_device *device) { + struct desc_object_caches *caches = device_get_desc_object_caches(device); struct vkd3d_view *view; assert(magic); - if (!(view = vkd3d_desc_object_cache_get(&device->view_desc_cache))) + if (!(view = vkd3d_desc_object_cache_get(&caches->view_desc_cache))) { ERR("Failed to allocate descriptor object.\n"); return NULL; @@ -2390,6 +2396,7 @@ static struct vkd3d_view *vkd3d_view_create(uint32_t magic, VkDescriptorType vk_ static void vkd3d_view_destroy(struct vkd3d_view *view, struct d3d12_device *device) { const struct vkd3d_vk_device_procs *vk_procs = &device->vk_procs; + struct desc_object_caches *caches = device_get_desc_object_caches(device); TRACE("Destroying view %p.\n", view); @@ -2411,7 +2418,7 @@ static void vkd3d_view_destroy(struct vkd3d_view *view, struct d3d12_device *dev if (view->v.vk_counter_view) VK_CALL(vkDestroyBufferView(device->vk_device, view->v.vk_counter_view, NULL)); - vkd3d_desc_object_cache_push(&device->view_desc_cache, view); + vkd3d_desc_object_cache_push(&caches->view_desc_cache, view); } void vkd3d_view_decref(void *view, struct d3d12_device *device) @@ -2424,7 +2431,10 @@ void vkd3d_view_decref(void *view, struct d3d12_device *device) if (u.header->magic != VKD3D_DESCRIPTOR_MAGIC_CBV) vkd3d_view_destroy(u.view, device); else - vkd3d_desc_object_cache_push(&device->cbuffer_desc_cache, u.object); + { + struct desc_object_caches *caches = device_get_desc_object_caches(device); + vkd3d_desc_object_cache_push(&caches->cbuffer_desc_cache, u.object); + } } static inline void d3d12_desc_replace(struct d3d12_desc *dst, void *view, struct d3d12_device *device) diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h index 2e9845df..97fa6b11 100644 --- a/libs/vkd3d/vkd3d_private.h +++ b/libs/vkd3d/vkd3d_private.h @@ -205,11 +205,18 @@ struct vkd3d_mutex CRITICAL_SECTION lock; }; +#define VKD3D_MUTEX_INITIALIZER {NULL, -1, 0, 0, 0, 0} + struct vkd3d_cond { CONDITION_VARIABLE cond; }; +struct vkd3d_tls_key +{ + unsigned int key; +}; + static inline void vkd3d_mutex_init(struct vkd3d_mutex *lock) { InitializeCriticalSection(&lock->lock); @@ -255,6 +262,26 @@ static inline void vkd3d_cond_destroy(struct vkd3d_cond *cond) { } +typedef PFLS_CALLBACK_FUNCTION vkd3d_tls_destructor_fn; + +static inline int vkd3d_tls_key_create(struct vkd3d_tls_key *key, vkd3d_tls_destructor_fn destructor) +{ + if ((key->key = FlsAlloc(destructor)) == FLS_OUT_OF_INDEXES) + return -1; + return 0; +} + +static inline int vkd3d_tls_key_set_value(const struct vkd3d_tls_key *key, void *value) +{ + FlsSetValue(key->key, value); + return 0; +} + +static inline void *vkd3d_tls_key_get_value(const struct vkd3d_tls_key *key) +{ + return FlsGetValue(key->key); +} + static inline unsigned int vkd3d_atomic_increment(unsigned int volatile *x) { return InterlockedIncrement((LONG volatile *)x); @@ -300,11 +327,17 @@ struct vkd3d_mutex pthread_mutex_t lock; }; +#define VKD3D_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER + struct vkd3d_cond { pthread_cond_t cond; }; +struct vkd3d_tls_key +{ + pthread_key_t key; +}; static inline void vkd3d_mutex_init(struct vkd3d_mutex *lock) { @@ -387,6 +420,23 @@ static inline void vkd3d_cond_destroy(struct vkd3d_cond *cond) ERR("Could not destroy the condition variable, error %d.\n", ret); } +typedef void (*vkd3d_tls_destructor_fn)(void *); + +static inline int vkd3d_tls_key_create(struct vkd3d_tls_key *key, vkd3d_tls_destructor_fn destructor) +{ + return pthread_key_create(&key->key, destructor); +} + +static inline int vkd3d_tls_key_set_value(const struct vkd3d_tls_key *key, void *value) +{ + return pthread_setspecific(key->key, value); +} + +static inline void *vkd3d_tls_key_get_value(const struct vkd3d_tls_key *key) +{ + return pthread_getspecific(key->key); +} + # if HAVE_SYNC_SUB_AND_FETCH static inline unsigned int vkd3d_atomic_decrement(unsigned int volatile *x) { @@ -1694,20 +1744,31 @@ struct vkd3d_uav_clear_state HRESULT vkd3d_uav_clear_state_init(struct vkd3d_uav_clear_state *state, struct d3d12_device *device); void vkd3d_uav_clear_state_cleanup(struct vkd3d_uav_clear_state *state, struct d3d12_device *device); -struct desc_object_cache_head +struct desc_rebalance { - void *head; - unsigned int spinlock; + void **data; + size_t capacity; + size_t count; + struct vkd3d_mutex mutex; }; struct vkd3d_desc_object_cache { - struct desc_object_cache_head heads[16]; - unsigned int next_index; - unsigned int free_count; + void **data; + size_t capacity; + size_t count; + size_t rebalance_threshold; + size_t threshold_expansion; + struct desc_rebalance *rebalance; size_t size; }; +struct desc_object_caches +{ + struct vkd3d_desc_object_cache view_desc_cache; + struct vkd3d_desc_object_cache cbuffer_desc_cache; +}; + #define VKD3D_DESCRIPTOR_POOL_COUNT 6 /* ID3D12Device */ @@ -1722,11 +1783,11 @@ struct d3d12_device PFN_vkd3d_signal_event signal_event; size_t wchar_size; + struct vkd3d_tls_key tls_key; + struct vkd3d_gpu_va_allocator gpu_va_allocator; struct vkd3d_mutex mutex; - struct vkd3d_desc_object_cache view_desc_cache; - struct vkd3d_desc_object_cache cbuffer_desc_cache; struct vkd3d_render_pass_cache render_pass_cache; VkPipelineCache vk_pipeline_cache; @@ -1779,6 +1840,7 @@ struct vkd3d_queue *d3d12_device_get_vkd3d_queue(struct d3d12_device *device, D3 bool d3d12_device_is_uma(struct d3d12_device *device, bool *coherent); void d3d12_device_mark_as_removed(struct d3d12_device *device, HRESULT reason, const char *message, ...) VKD3D_PRINTF_FUNC(3, 4); +struct desc_object_caches *device_get_desc_object_caches(struct d3d12_device *device); struct d3d12_device *unsafe_impl_from_ID3D12Device5(ID3D12Device5 *iface); static inline HRESULT d3d12_device_query_interface(struct d3d12_device *device, REFIID iid, void **object) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/384
Not sure why the CI mingw build needs braces around `VKD3D_MUTEX_INITIALIZER` while my local one doesn't, but adding them fixed the CI build. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/384#note_48447
Giovanni Mascellani (@giomasce) commented about libs/vkd3d/device.c:
+ static struct vkd3d_mutex key_cache_mutex = {VKD3D_MUTEX_INITIALIZER}; + static struct vkd3d_tls_key tls_key; + static bool tls_initialised; + int rc = 0; + + vkd3d_mutex_lock(&key_cache_mutex); + + if (!tls_initialised) + { + rc = vkd3d_tls_key_create(&tls_key, global_tls_key_destroy); + desc_rebalance_init(&view_desc_rebalance); + desc_rebalance_init(&cbuffer_desc_rebalance); + tls_initialised = true; + } + + vkd3d_mutex_unlock(&key_cache_mutex); I guess this can be done more efficiently with `pthread_once()` and `InitOnceExecuteOnce()`. Though it's probably not a bottleneck anyway.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/384#note_48506
Giovanni Mascellani (@giomasce) commented about libs/vkd3d/vkd3d_private.h:
{ }
+typedef PFLS_CALLBACK_FUNCTION vkd3d_tls_destructor_fn; + +static inline int vkd3d_tls_key_create(struct vkd3d_tls_key *key, vkd3d_tls_destructor_fn destructor) I'd move the logging inside `vkd3d_tls_key_create()`, so that each implementation can meaningfully log its own error value (either the returned value or `GetLastError()`), at which point `vkd3d_tls_key_create()` can just return a `bool`.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/384#note_48507
Giovanni Mascellani (@giomasce) commented about libs/vkd3d/vkd3d_private.h:
{ }
+typedef PFLS_CALLBACK_FUNCTION vkd3d_tls_destructor_fn; + +static inline int vkd3d_tls_key_create(struct vkd3d_tls_key *key, vkd3d_tls_destructor_fn destructor) +{ + if ((key->key = FlsAlloc(destructor)) == FLS_OUT_OF_INDEXES) + return -1; + return 0; +} + +static inline int vkd3d_tls_key_set_value(const struct vkd3d_tls_key *key, void *value) Similarly to mutex functions, I'd assume that we cannot reasonably react to `vkd3d_tls_key_set_value()` and `vkd3d_tls_key_get_value()` failing, so I think they should return `void` and log whatever error they happen to step into. The "backend" functions should probably never fail, and if they do it means that the whole process is probably in a huge mess.
Notice also that `FlsSetValue()` can return an error, which I think should be logged, and `FlsGetValue()` can return an error too, which can be detected by calling `SetLastError()` before calling `FlsGetValue()`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/384#note_48508
Giovanni Mascellani (@giomasce) commented about libs/vkd3d/device.c:
- for (i = 0; i < ARRAY_SIZE(cache->heads); ++i) + if (caches) { - for (u.object = cache->heads[i].head; u.object; u.object = next) - { - next = u.header->next; - vkd3d_free(u.object); - } + vkd3d_desc_object_cache_cleanup(&caches->view_desc_cache); + vkd3d_desc_object_cache_cleanup(&caches->cbuffer_desc_cache); + vkd3d_free(caches); } }
+struct desc_rebalance view_desc_rebalance; +struct desc_rebalance cbuffer_desc_rebalance; They should probably be `static`.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/384#note_48509
The rebalancing algorithm seems quite complicated, so we really it that way? Could you explain the reasoning behind it? A simpler alternative would be to fix some number N (say, 64), and each time a thread reaches N it donates N/2 to the rebalance, and each time a thread is empty it fetches N/2 from the balance (or allocates). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/384#note_48510
On Thu Oct 12 13:59:14 2023 +0000, Giovanni Mascellani wrote:
I guess this can be done more efficiently with `pthread_once()` and `InitOnceExecuteOnce()`. Though it's probably not a bottleneck anyway. Using those would require conditional compilation or a wrapper callback as they take different function definitions. Overall the mutex takes less code. Having a `vkd3d_once()` would be convenient if we ever need one-time initialisation elsewhere, but any such code could probably just be combined into a common initialisation function.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/384#note_48612
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)