api: Added SDL_CreateTemporaryProperties() for more-efficient throwaway props. #14440
Conversation
|
Let's hold this for 3.6 |
icculus
force-pushed
the
sdl3-temporary-properties
branch
from
84e5dba to
bff3fde
Compare
icculus
force-pushed
the
sdl3-temporary-properties
branch
from
bff3fde to
446e676
Compare
|
Rebased to fix conflicts and changed the Let's decide if we want to do this. |
|
I'm kind of inclined not to do this. It adds some complexity to SDL, it doesn't significantly reduce the complexity of the application code, and properties are intended by design not to be in performance critical paths. Maybe we should take the original example and compare the allocations before and after to see how much this improves things? |
This diff to log allocations and locks... diff --git a/src/stdlib/SDL_malloc.c b/src/stdlib/SDL_malloc.c
index 5fb7c1c9b..2eb3c4d4c 100644
--- a/src/stdlib/SDL_malloc.c
+++ b/src/stdlib/SDL_malloc.c
@@ -6452,6 +6452,7 @@ void *SDL_malloc(size_t size)
{
void *mem;
+printf("SDL_malloc(%d)", (int) size);
if (!size) {
size = 1;
}
@@ -6463,6 +6464,8 @@ void *SDL_malloc(size_t size)
SDL_OutOfMemory();
}
+printf(" -> %p\n", mem);
+
return mem;
}
@@ -6470,6 +6473,8 @@ void *SDL_calloc(size_t nmemb, size_t size)
{
void *mem;
+printf("SDL_calloc(%d, %d)", (int) nmemb, (int) size);
+
if (!nmemb || !size) {
nmemb = 1;
size = 1;
@@ -6482,6 +6487,7 @@ void *SDL_calloc(size_t nmemb, size_t size)
SDL_OutOfMemory();
}
+printf(" -> %p\n", mem);
return mem;
}
@@ -6489,6 +6495,8 @@ void *SDL_realloc(void *ptr, size_t size)
{
void *mem;
+printf("SDL_realloc(%p, %d)", ptr, (int) size);
+
if (!size) {
size = 1;
}
@@ -6500,6 +6508,8 @@ void *SDL_realloc(void *ptr, size_t size)
SDL_OutOfMemory();
}
+printf(" -> %p\n", mem);
+
return mem;
}
@@ -6509,6 +6519,8 @@ void SDL_free(void *ptr)
return;
}
+printf("SDL_free(%p)\n", ptr);
+
s_mem.free_func(ptr);
DECREMENT_ALLOCATION_COUNT();
}
diff --git a/src/thread/pthread/SDL_sysmutex.c b/src/thread/pthread/SDL_sysmutex.c
index 3e04d2150..5d0dedb22 100644
--- a/src/thread/pthread/SDL_sysmutex.c
+++ b/src/thread/pthread/SDL_sysmutex.c
@@ -60,6 +60,7 @@ void SDL_DestroyMutex(SDL_Mutex *mutex)
void SDL_LockMutex(SDL_Mutex *mutex) SDL_NO_THREAD_SAFETY_ANALYSIS // clang doesn't know about NULL mutexes
{
+printf("LockMutex(%p)\n", mutex);
if (mutex) {
#ifdef FAKE_RECURSIVE_MUTEX
pthread_t this_thread = pthread_self();
@@ -125,6 +126,7 @@ bool SDL_TryLockMutex(SDL_Mutex *mutex)
void SDL_UnlockMutex(SDL_Mutex *mutex) SDL_NO_THREAD_SAFETY_ANALYSIS // clang doesn't know about NULL mutexes
{
+printf("UnlockMutex(%p)\n", mutex);
if (mutex) {
#ifdef FAKE_RECURSIVE_MUTEX
// We can only unlock the mutex if we own it
diff --git a/src/thread/pthread/SDL_sysrwlock.c b/src/thread/pthread/SDL_sysrwlock.c
index ed7f5482e..ecdd1a8ba 100644
--- a/src/thread/pthread/SDL_sysrwlock.c
+++ b/src/thread/pthread/SDL_sysrwlock.c
@@ -55,6 +55,7 @@ void SDL_DestroyRWLock(SDL_RWLock *rwlock)
void SDL_LockRWLockForReading(SDL_RWLock *rwlock) SDL_NO_THREAD_SAFETY_ANALYSIS // clang doesn't know about NULL mutexes
{
+printf("LockRWLockForReading(%p)\n", rwlock);
if (rwlock) {
const int rc = pthread_rwlock_rdlock(&rwlock->id);
SDL_assert(rc == 0); // assume we're in a lot of trouble if this assert fails.
@@ -63,6 +64,7 @@ void SDL_LockRWLockForReading(SDL_RWLock *rwlock) SDL_NO_THREAD_SAFETY_ANALYSIS
void SDL_LockRWLockForWriting(SDL_RWLock *rwlock) SDL_NO_THREAD_SAFETY_ANALYSIS // clang doesn't know about NULL mutexes
{
+printf("LockRWLockForWriting(%p)\n", rwlock);
if (rwlock) {
const int rc = pthread_rwlock_wrlock(&rwlock->id);
SDL_assert(rc == 0); // assume we're in a lot of trouble if this assert fails.
@@ -105,6 +107,7 @@ bool SDL_TryLockRWLockForWriting(SDL_RWLock *rwlock)
void SDL_UnlockRWLock(SDL_RWLock *rwlock) SDL_NO_THREAD_SAFETY_ANALYSIS // clang doesn't know about NULL mutexes
{
+printf("UnlockRWLock(%p)\n", rwlock);
if (rwlock) {
const int rc = pthread_rwlock_unlock(&rwlock->id);
SDL_assert(rc == 0); // assume we're in a lot of trouble if this assert fails....and this test program... #include <stdio.h>
#include <SDL3/SDL.h>
#include <SDL3/SDL_main.h>
int main(int argc, char **argv)
{
SDL_Init(0);
printf("TEST START\n");
#if 1
SDL_PropertiesID props = SDL_CreateProperties();
SDL_SetPointerProperty(props, SDL_PROP_PROCESS_CREATE_ARGS_POINTER, argv);
SDL_SetNumberProperty(props, SDL_PROP_PROCESS_CREATE_STDIN_NUMBER, SDL_PROCESS_STDIO_NULL);
SDL_SetNumberProperty(props, SDL_PROP_PROCESS_CREATE_STDOUT_NUMBER, SDL_PROCESS_STDIO_NULL);
SDL_SetNumberProperty(props, SDL_PROP_PROCESS_CREATE_STDERR_NUMBER, SDL_PROCESS_STDIO_NULL);
SDL_SetBooleanProperty(props, SDL_PROP_PROCESS_CREATE_BACKGROUND_BOOLEAN, true);
#else
const SDL_TemporaryPropertyItem prop_items[] = {
{ SDL_PROP_PROCESS_CREATE_ARGS_POINTER, SDL_PROPERTY_TYPE_POINTER, { .p = argv } },
{ SDL_PROP_PROCESS_CREATE_STDIN_NUMBER, SDL_PROPERTY_TYPE_NUMBER, { .n = SDL_PROCESS_STDIO_NULL } },
{ SDL_PROP_PROCESS_CREATE_STDOUT_NUMBER, SDL_PROPERTY_TYPE_NUMBER, { .n = SDL_PROCESS_STDIO_NULL } },
{ SDL_PROP_PROCESS_CREATE_STDERR_NUMBER, SDL_PROPERTY_TYPE_NUMBER, { .n = SDL_PROCESS_STDIO_NULL } },
{ SDL_PROP_PROCESS_CREATE_BACKGROUND_BOOLEAN, SDL_PROPERTY_TYPE_BOOLEAN, { .b = true } }
};
SDL_PropertiesID props = SDL_CreateTemporaryProperties(prop_items, SDL_arraysize(prop_items));
#endif
SDL_DestroyProperties(props);
printf("TEST END\n");
SDL_Quit();
return 0;
}So this just creates a Properties set, adds some stuff to it, and deletes it. Here's the relevant output for the normal path: Here's the output for the temporary properties version: (the calloc() and free() are to make the property object; all those locks are to manage the hashtable of SDL_PropertiesIDs. We could avoid those if we reserve some piece of the ID space for temporary objects and never put them in the global table at all, but let's not get ahead of ourselves.) Since this is just creation/destruction and not actually using a properties group (to avoid catching allocations and locks done by SDL_CreateProcessWithProperties() or whatever), here's what a single use looks like: SDL_CopyProperties() is almost identical between the two (one extra lock or so). This is because temporary objects are read-only and can't be copied into, so most of this specific overhead is dealing with the normal Properties object you'd be copying into. SDL_Get*Property() looks like this in normal operation: ...and like this with a temporary property set: (which is, again, the global property lock while it converts the SDL_PropertiesID into a struct.) So it's definitely less locking and malloc pressure, but I'm also willing to believe that it isn't actually a bottleneck in real world use, and don't have serious data on that at the moment. |
|
@willvale, do you have a real world example that shows the benefit (or lack thereof) of this PR? |
|
I'm just making some changes so that I can try this out - been distracted integrating/fixing another open source library! |
|
See below for my results for My specific use (currently) is when I'm building game data which includes ink scripts, I need to shell out to a couple of tools to compile each script. Normally I run the data build as part of launching the game and aim to keep the time of that build as low as possible so I can iterate quickly with a full tools pipeline. So for me, having a measurably faster debug configuration is worthwhile as that's the one I usually run. But it's not a deal-breaker. Opinionated bit: _Really, the reason I opened the ticket was to try and discourage blanket use of SDL_Properties as they're surprisingly expensive. IMO when considering if an API should take properties, the default should be "no" unless it's something that's called once at startup or on e.g. a mode change, and needs to take and/or return platform-dependent properties. At the moment the uses which look worrying are in SDL_IOStream as mentioned above, SDL_Thread, and the renderer textures. But the more code that relies on this mechanism, the harder it will be to change course if it ends up causing death by 1000 cuts._ Full fat properties:Debug build actual timings for CreateProperties/CreateProcessWithProperties/DestroyProperties Release build actual timings for CreateProperties/CreateProcessWithProperties/DestroyProperties Temporary properties:Debug build actual timings for CreateTemporaryProperties/CreateProcessWithProperties/DestroyProperties Release build actual timings CreateTemporaryProperties/CreateProcessWithProperties/DestroyProperties |
3 participants
This is an attempt at a limited-but-lower-overhead Properties object that could be useful for throwaway SDL_PropertiesID...namely, the type we use for
SDL_Create*WithPropertiesfunctions.And example of updating testyuv.c to use it:
If following the rules for these objects (read-only, thread unsafe, etc), they could be used in other situations, as they can be used anywhere an SDL_PropertiesID is expected.
Fixes #14436.