Avoid unnecessary heap allocation in SDL_CreateThread

SDL Development
1

Hi all.
I was looking at how SDL_CreateThread() and SDL_RunThread() make use of the thread_args structure passed along.

SDL_CreateThread() waits for SDL_RunThread() to signal its consumption of thread_args and then frees it.
Why not make it a local variable and save one heap allocation per thread creation?

Code:
diff -r e0038858fff3 src/thread/SDL_thread.c
— a/src/thread/SDL_thread.c Wed Sep 16 12:23:20 2015 +0200
+++ b/src/thread/SDL_thread.c Wed Sep 16 13:13:00 2015 +0200
@@ -316,7 +316,7 @@
#endif
{
SDL_Thread *thread;

  • thread_args *args;

  • thread_args args;
    int ret;

    /* Allocate memory for the thread info structure */
    @@ -329,7 +329,6 @@
    thread->status = -1;
    SDL_AtomicSet(&thread->state, SDL_THREAD_STATE_ALIVE);

  • /* Set up the arguments for the thread */
    if (name != NULL) {
    thread->name = SDL_strdup(name);
    if (thread->name == NULL) {
    @@ -340,37 +339,27 @@
    }

    /* Set up the arguments for the thread */

  • args = (thread_args *) SDL_malloc(sizeof(*args));

  • if (args == NULL) {

  •    SDL_OutOfMemory();
  • args.func = fn;
  • args.data = data;
  • args.info = thread;
  • args.wait = SDL_CreateSemaphore(0);
  • if (args.wait == NULL) {
    if (thread->name) {
    SDL_free(thread->name);
    }
    SDL_free(thread);
    return (NULL);
    }

  • args->func = fn;

  • args->data = data;

  • args->info = thread;

  • args->wait = SDL_CreateSemaphore(0);

  • if (args->wait == NULL) {

  •    if (thread->name) {

  •        SDL_free(thread->name);

  •    }

  •    SDL_free(thread);

  •    SDL_free(args);

  •    return (NULL);

  • }

    /* Create the thread and go! */
    #ifdef SDL_PASSED_BEGINTHREAD_ENDTHREAD

  • ret = SDL_SYS_CreateThread(thread, args, pfnBeginThread, pfnEndThread);

  • ret = SDL_SYS_CreateThread(thread, &args, pfnBeginThread, pfnEndThread);
    #else
  • ret = SDL_SYS_CreateThread(thread, args);
  • ret = SDL_SYS_CreateThread(thread, &args);
    #endif
    if (ret >= 0) {
    /* Wait for the thread function to use arguments */
  •    SDL_SemWait(args->wait);

  •    SDL_SemWait(args.wait);
    } else {
    /* Oops, failed. Gotta free everything */
    if (thread->name) {
    @@ -379,8 +368,7 @@
    SDL_free(thread);
    thread = NULL;
    }
  • SDL_DestroySemaphore(args->wait);
  • SDL_free(args);

  • SDL_DestroySemaphore(args.wait);

    /* Everything is running now */
    return (thread);------------------------
    – Sascha

2

It’s not unnecessary. The stack of a process should be protected and no other process should be able to access stack of that process!

3

.3lite wrote:

It’s not unnecessary. The stack of a process should be protected and no other process should be able to access stack of that process!
You are right.
SDL_CreateThread() on the other hand does not create processes. It creates threads sharing the same process.------------------------
– Sascha