Capture autosave SRAM on the main thread

Replace concurrent live-SRAM reads and the uninitialized frame sequence with worker-requested snapshots. Preserve nested netplay scopes, service paused iterations, and keep disk I/O outside the publication lock. Retain failed writes for retry instead of advancing the saved-image baseline.

Add regression coverage for snapshot ownership during slow I/O, nested scopes, paused checks, unchanged data, write failures, compressed routing and shutdown.

autosave.h

@@ -23,20 +23,14 @@
 
 RETRO_BEGIN_DECLS
 
-/**
- * autosave_lock:
- *
- * Lock autosave.
- **/
+/* Main-thread SRAM scopes may nest; the outermost unlock publishes
+ * a requested snapshot after core execution and post-frame updates. */
 void autosave_lock(void);
-
-/**
- * autosave_unlock:
- *
- * Unlocks autosave.
- **/
 void autosave_unlock(void);
 
+/* Service requests on the main thread even when no core frames run. */
+void autosave_check(void);
+
 bool autosave_init(bool compress_files, unsigned autosave_interval);
 
 void autosave_deinit(void);

runloop.c

@@ -8474,6 +8474,11 @@ int runloop_iterate(void)
 #endif
    }
 
+#ifdef HAVE_THREADS
+   if (runloop_st->flags & RUNLOOP_FLAG_AUTOSAVE)
+      autosave_check();
+#endif
+
    switch ((enum runloop_state_enum)runloop_check_state(
             input_st, audio_st, video_st,
             uico_st,

save.c

@@ -61,157 +61,55 @@ struct autosave_st
 {
    autosave_t **list;
    unsigned num;
-};
-
-enum autosave_flags
-{
-   AUTOSAVE_FLAG_COMPRESS_FILES = (1 << 1),
-   AUTOSAVE_FLAG_DIRTY          = (1 << 2)
+   unsigned depth;
 };
 
 struct autosave
 {
-   /* The core's frame, published rather than locked: the main thread
-    * makes this odd before retro_run() and even after the post-core
-    * work, with a plain release store - no mutex, no read-modify-write
-    * - and the worker reads it either side of its snapshot. A snapshot
-    * that overlaps a frame may be torn, and the sequence is how that
-    * is noticed; it is dropped and retried. The main thread used to
-    * take every handle's mutex around retro_run(): a lock held across
-    * core execution, whose cache line moved between the emulation
-    * thread and the storage worker every frame. */
-   retro_atomic_size_t frame_seq;
    void *buffer;
+   void *snapshot;
    const void *retro_buffer;
    char *path;
-   slock_t *lock;
    slock_t *cond_lock;
    scond_t *cond;
    sthread_t *thread;
    size_t bufsize;
    unsigned interval;
-   /* Guarded by 'lock'.  AUTOSAVE_FLAG_QUIT is deliberately NOT kept
-    * here: it is guarded by cond_lock, and two locks protecting
-    * different bits of one byte do not protect the byte -- the
-    * read-modify-write covers all of it. */
-   uint8_t flags;
-   /* Guarded by cond_lock. */
-   uint8_t quit;
+   /* Only the main thread reads live SRAM. The worker requests a copy
+    * and waits until autosave_unlock() publishes it under cond_lock. */
+   retro_atomic_int_t snapshot_requested;
+   bool compress;
+   bool quit;
 };
 
 static struct autosave_st autosave_state;
 
-
-/**
- * autosave_thread:
- * @data            : pointer to autosave object
- *
- * Callback function for (threaded) autosave.
- *
- * Performance notes:
- *  - The dirty flag allows an early-out when the core
- *    has not touched SRAM since last check, avoiding a
- *    full memcmp on every wake-up.
- *  - When a comparison is needed we scan word-at-a-time
- *    to locate the first differing region, then only
- *    memcmp/memcpy from that point onward.
- *  - The file write happens entirely outside the lock,
- *    so the core is never stalled on disk I/O.
- **/
 static void autosave_thread(void *data)
 {
    autosave_t *save = (autosave_t*)data;
+   bool retry_write = false;
 
-   for (;;)
+   slock_lock(save->cond_lock);
+   while (!save->quit)
    {
-      bool differ       = false;
-      bool compress     = false;
-      size_t seq_before = 0;
+      retro_atomic_store_release_int(&save->snapshot_requested, 1);
+      while (!save->quit &&
+            retro_atomic_load_relaxed_int(&save->snapshot_requested))
+         scond_wait(save->cond, save->cond_lock);
 
-      /* Taken outside the frame, without the main thread's help: read
-       * the sequence, take the snapshot, read it again. Odd means the
-       * core is running now; a change means a frame began or ended
-       * while it was taken. */
-      seq_before = retro_atomic_load_acquire_size(&save->frame_seq);
+      if (save->quit)
+         break;
+      slock_unlock(save->cond_lock);
 
-      slock_lock(save->lock);
-
-      /* Fast path: if the core hasn't signalled a write
-       * since our last check, skip the expensive memcmp.
-       * Falls back to full comparison if the dirty flag
-       * was never set (conservative default). */
-      if (!(seq_before & 1) && (save->flags & AUTOSAVE_FLAG_DIRTY))
-      {
-         const size_t word_size = sizeof(size_t);
-         const size_t aligned   = save->bufsize / word_size;
-         const size_t remainder = save->bufsize % word_size;
-         const size_t *src_w    = (const size_t*)save->retro_buffer;
-         const size_t *dst_w    = (const size_t*)save->buffer;
-         size_t offset          = 0;
-         size_t i;
-
-         /* Word-at-a-time scan to find first difference.
-          * Avoids touching the entire buffer when only
-          * a small region changed (common for SRAM). */
-         for (i = 0; i < aligned; i++)
-         {
-            if (src_w[i] != dst_w[i])
-            {
-               differ = true;
-               offset = i * word_size;
-               break;
-            }
-         }
-
-         /* Check trailing bytes if no word-level diff found */
-         if (!differ && remainder > 0)
-         {
-            const unsigned char *src_b =
-               (const unsigned char*)save->retro_buffer + aligned * word_size;
-            const unsigned char *dst_b =
-               (const unsigned char*)save->buffer + aligned * word_size;
-            if (memcmp(dst_b, src_b, remainder) != 0)
-            {
-               differ = true;
-               offset = aligned * word_size;
-            }
-         }
-
-         /* Only copy from first difference onward */
-         if (differ)
-            memcpy((unsigned char*)save->buffer + offset,
-                   (const unsigned char*)save->retro_buffer + offset,
-                   save->bufsize - offset);
-
-         /* Clear dirty flag regardless — we've checked */
-         save->flags &= ~AUTOSAVE_FLAG_DIRTY;
-      }
-
-      /* COMPRESS_FILES never changes after autosave_new(), but it
-       * shares the byte with DIRTY, which the main thread sets under
-       * this lock.  Sample it here rather than reading save->flags
-       * again once the lock is dropped. */
-      compress = (save->flags & AUTOSAVE_FLAG_COMPRESS_FILES) != 0;
-
-      slock_unlock(save->lock);
-
-      /* A frame began or ended while the snapshot was taken: part of
-       * it may be from before that frame and part from after. Drop it
-       * and leave the buffer dirty, so the next interval retries; the
-       * file is only ever written from a snapshot known whole. */
-      if (differ && retro_atomic_load_acquire_size(&save->frame_seq) != seq_before)
-      {
-         differ = false;
-         slock_lock(save->lock);
-         save->flags |= AUTOSAVE_FLAG_DIRTY;
-         slock_unlock(save->lock);
-      }
-
-      if (differ)
+      /* The snapshot remains ours until the next request. Keep the
+       * last written image unchanged if opening or writing fails. */
+      if (retry_write ||
+            memcmp(save->buffer, save->snapshot, save->bufsize) != 0)
       {
          intfstream_t *file = NULL;
+         retry_write = true;
 
-         if (compress)
+         if (save->compress)
             file = intfstream_open_rzip_file(save->path,
                   RETRO_VFS_FILE_ACCESS_WRITE);
          else
@@ -220,32 +118,32 @@ static void autosave_thread(void *data)
 
          if (file)
          {
-            intfstream_write(file, save->buffer, save->bufsize);
-            intfstream_flush(file);
-            intfstream_close(file);
+            bool written = intfstream_write(file, save->snapshot,
+                  save->bufsize) == (int64_t)save->bufsize;
+            if (intfstream_flush(file) != 0)
+               written = false;
+            if (intfstream_close(file) != 0)
+               written = false;
             free(file);
+            if (written)
+            {
+               memcpy(save->buffer, save->snapshot, save->bufsize);
+               retry_write = false;
+            }
          }
       }
 
       slock_lock(save->cond_lock);
-
-      if (save->quit)
-      {
-         slock_unlock(save->cond_lock);
-         break;
-      }
-
-      scond_wait_timeout(save->cond,
-            save->cond_lock,
+      if (!save->quit)
+         scond_wait_timeout(save->cond, save->cond_lock,
 #if defined(_MSC_VER) && _MSC_VER <= 1200
-            save->interval * 1000000
+               save->interval * 1000000
 #else
-            save->interval * 1000000LL
+               save->interval * 1000000LL
 #endif
-            );
-
-      slock_unlock(save->cond_lock);
+               );
    }
+   slock_unlock(save->cond_lock);
 }
 
 /**
@@ -269,18 +167,17 @@ static autosave_t *autosave_new(const char *path,
    if (!handle)
       return NULL;
 
-   handle->flags                 = AUTOSAVE_FLAG_DIRTY;
-   handle->quit                  = 0;
+   handle->compress              = compress;
+   handle->quit                  = false;
+   retro_atomic_int_init(&handle->snapshot_requested, 0);
    handle->bufsize               = len;
    handle->interval              = interval;
    handle->buffer                = NULL;
-   handle->lock                  = NULL;
+   handle->snapshot              = NULL;
    handle->cond_lock             = NULL;
    handle->cond                  = NULL;
    handle->thread                = NULL;
 
-   if (compress)
-      handle->flags             |= AUTOSAVE_FLAG_COMPRESS_FILES;
    handle->retro_buffer          = data;
    /* Own the path string rather than borrowing it. The caller's
     * path comes from task_save_files->elems[i].data, freed by
@@ -306,15 +203,14 @@ static autosave_t *autosave_new(const char *path,
 
    memcpy(handle->buffer, handle->retro_buffer, handle->bufsize);
 
-   handle->lock                  = slock_new();
+   handle->snapshot              = malloc(len);
    handle->cond_lock             = slock_new();
    handle->cond                  = scond_new();
 
-   if (!handle->lock || !handle->cond_lock || !handle->cond)
+   if (!handle->snapshot || !handle->cond_lock || !handle->cond)
    {
       RARCH_ERR("[SRAM] Failed to initialize autosave synchronization primitives.\n");
-      if (handle->lock)
-         slock_free(handle->lock);
+      free(handle->snapshot);
       if (handle->cond_lock)
          slock_free(handle->cond_lock);
       if (handle->cond)
@@ -330,7 +226,7 @@ static autosave_t *autosave_new(const char *path,
    if (!handle->thread)
    {
       RARCH_ERR("[SRAM] Failed to create autosave thread.\n");
-      slock_free(handle->lock);
+      free(handle->snapshot);
       slock_free(handle->cond_lock);
       scond_free(handle->cond);
       free(handle->path);
@@ -351,12 +247,12 @@ static autosave_t *autosave_new(const char *path,
 static void autosave_free(autosave_t *handle)
 {
    slock_lock(handle->cond_lock);
-   handle->quit  = 1;
+   handle->quit  = true;
    slock_unlock(handle->cond_lock);
    scond_signal(handle->cond);
    sthread_join(handle->thread);
 
-   slock_free(handle->lock);
+   free(handle->snapshot);
    slock_free(handle->cond_lock);
    scond_free(handle->cond);
 
@@ -435,54 +331,42 @@ void autosave_deinit(void)
    autosave_state.num      = 0;
 }
 
-/**
- * autosave_lock:
- *
- * Lock autosave.
- **/
 void autosave_lock(void)
+{
+   autosave_state.depth++;
+}
+
+void autosave_check(void)
 {
    unsigned i;
 
+   if (autosave_state.depth)
+      return;
+
    for (i = 0; i < autosave_state.num; i++)
    {
       autosave_t *handle = autosave_state.list[i];
-      if (handle)
+      if (!handle)
+         continue;
+#ifdef RETRO_ATOMIC_LOCK_FREE
+      if (!retro_atomic_load_acquire_int(&handle->snapshot_requested))
+         continue;
+#endif
+      slock_lock(handle->cond_lock);
+      if (retro_atomic_load_relaxed_int(&handle->snapshot_requested))
       {
-         /* Odd: the core is running, and any snapshot overlapping
-          * this window is discarded by the worker. */
-         size_t seq = retro_atomic_load_acquire_size(&handle->frame_seq);
-         retro_atomic_store_release_size(&handle->frame_seq, seq + 1);
+         memcpy(handle->snapshot, handle->retro_buffer, handle->bufsize);
+         retro_atomic_store_release_int(&handle->snapshot_requested, 0);
+         scond_signal(handle->cond);
       }
+      slock_unlock(handle->cond_lock);
    }
 }
 
-/**
- * autosave_unlock:
- *
- * Unlocks autosave.
- * Also marks all buffers as dirty, since the core
- * may have written to SRAM while holding the lock.
- **/
 void autosave_unlock(void)
 {
-   unsigned i;
-
-   for (i = 0; i < autosave_state.num; i++)
-   {
-      autosave_t *handle = autosave_state.list[i];
-      if (handle)
-      {
-         /* Even: the frame is over and the buffer is stable until the
-          * next one begins. The dirty bit is set here without the
-          * mutex - the worker only clears it, and a lost update costs
-          * one more comparison, never a missed save: the write below
-          * is driven by the comparison, not by the bit. */
-         size_t seq = retro_atomic_load_acquire_size(&handle->frame_seq);
-         handle->flags |= AUTOSAVE_FLAG_DIRTY;
-         retro_atomic_store_release_size(&handle->frame_seq, seq + 1);
-      }
-   }
+   if (autosave_state.depth && !--autosave_state.depth)
+      autosave_check();
 }
 
 #endif

tests-other/Makefile.autosave_snapshot

@@ -0,0 +1,33 @@
+CC ?= cc
+ROOT := ..
+TARGET := autosave_snapshot_test
+SOURCES := autosave_snapshot_test.c $(ROOT)/libretro-common/rthreads/rthreads.c \
+           $(ROOT)/libretro-common/time/rtime.c
+CFLAGS += -std=gnu89 -Wall -Wextra -g -O1 -DHAVE_THREADS \
+          -ffunction-sections -fdata-sections \
+          -I$(ROOT) -I$(ROOT)/libretro-common/include
+LDFLAGS += -Wl,--gc-sections
+LDLIBS += -lpthread
+
+# PE linkers retain unrelated external save.c functions without LTO.
+ifneq ($(filter MSYS% MINGW% CYGWIN%,$(shell uname -s)),)
+CFLAGS += -flto
+endif
+
+ifneq ($(SANITIZER),)
+CFLAGS += -fsanitize=$(SANITIZER) -fno-omit-frame-pointer
+LDFLAGS += -fsanitize=$(SANITIZER)
+endif
+
+all: $(TARGET)
+
+$(TARGET): $(SOURCES) $(ROOT)/save.c $(ROOT)/autosave.h
+	$(CC) $(CFLAGS) $(SOURCES) $(LDFLAGS) $(LDLIBS) -o $@
+
+check: $(TARGET)
+	./$(TARGET)
+
+clean:
+	rm -f $(TARGET) $(TARGET).exe
+
+.PHONY: all check clean

tests-other/autosave_snapshot_test.c

@@ -0,0 +1,237 @@
+/* Build with function sections and --gc-sections: only the autosave
+ * portion of save.c is linked. Stream stubs control I/O failures. */
+#include <assert.h>
+#include <stdio.h>
+#include <retro_timers.h>
+#include "../save.c"
+
+#define SRAM_SIZE 17
+
+static slock_t *io_lock;
+static scond_t *io_cond;
+static unsigned opens;
+static unsigned writes;
+static unsigned compressed_opens;
+static bool hold_write;
+static unsigned fail_stage;
+static unsigned char disk_image[SRAM_SIZE];
+static autosave_t *current;
+static unsigned char ram_storage[SRAM_SIZE + 1];
+static unsigned char *ram = ram_storage + 1;
+
+void RARCH_ERR(const char *fmt, ...)
+{
+   (void)fmt;
+}
+
+intfstream_t *intfstream_open_file(const char *path,
+      unsigned mode, unsigned hints)
+{
+   bool fail;
+   (void)path;
+   (void)mode;
+   (void)hints;
+   slock_lock(io_lock);
+   opens++;
+   fail = fail_stage == 1;
+   slock_unlock(io_lock);
+   return fail ? NULL : (intfstream_t*)malloc(1);
+}
+
+intfstream_t *intfstream_open_rzip_file(const char *path, unsigned mode)
+{
+   slock_lock(io_lock);
+   compressed_opens++;
+   slock_unlock(io_lock);
+   return intfstream_open_file(path, mode, 0);
+}
+
+int64_t intfstream_write(intfstream_t *file, const void *data, uint64_t len)
+{
+   bool fail;
+   (void)file;
+   assert(len == SRAM_SIZE);
+   slock_lock(io_lock);
+   writes++;
+   scond_signal(io_cond);
+   while (hold_write)
+      scond_wait(io_cond, io_lock);
+   memcpy(disk_image, data, SRAM_SIZE);
+   fail = fail_stage == 2;
+   slock_unlock(io_lock);
+   return fail ? (int64_t)len - 1 : (int64_t)len;
+}
+
+int intfstream_flush(intfstream_t *file)
+{
+   int result;
+   (void)file;
+   slock_lock(io_lock);
+   result = fail_stage == 3 ? -1 : 0;
+   slock_unlock(io_lock);
+   return result;
+}
+
+int intfstream_close(intfstream_t *file)
+{
+   int result;
+   (void)file;
+   slock_lock(io_lock);
+   result = fail_stage == 4 ? -1 : 0;
+   slock_unlock(io_lock);
+   return result;
+}
+
+static void wait_requested(void)
+{
+   unsigned i;
+   for (i = 0; i < 5000; i++)
+   {
+      bool requested;
+      slock_lock(current->cond_lock);
+      requested = retro_atomic_load_relaxed_int(
+            &current->snapshot_requested) != 0;
+      slock_unlock(current->cond_lock);
+      if (requested)
+         return;
+      retro_sleep(1);
+   }
+   assert(!"snapshot request timed out");
+}
+
+static void capture_and_wait(void)
+{
+   autosave_lock();
+   autosave_unlock();
+   wait_requested();
+}
+
+static void start_save(bool compress)
+{
+   memset(ram, 0, SRAM_SIZE);
+   current = autosave_new("unused.srm", ram, SRAM_SIZE, 0, compress);
+   assert(current);
+   autosave_state.list = &current;
+   autosave_state.num  = 1;
+   wait_requested();
+}
+
+static void stop_save(void)
+{
+   /* No further frames: shutdown must wake a worker awaiting a copy. */
+   autosave_free(current);
+   current = NULL;
+   autosave_state.list = NULL;
+   autosave_state.num  = 0;
+}
+
+int main(void)
+{
+   unsigned i;
+   unsigned before;
+   io_lock = slock_new();
+   io_cond = scond_new();
+   assert(io_lock && io_cond);
+   start_save(false);
+
+   /* Netplay may nest an SRAM scope inside the runloop's frame. */
+   autosave_lock();
+   autosave_lock();
+   autosave_check();
+   autosave_unlock();
+   assert(retro_atomic_load_acquire_int(&current->snapshot_requested));
+   autosave_unlock();
+   wait_requested();
+
+   capture_and_wait();
+   assert(opens == 0);
+
+   /* Paused/menu iterations service requests without a core frame. */
+   memset(ram, 8, SRAM_SIZE);
+   autosave_check();
+   wait_requested();
+   assert(memcmp(disk_image, ram, SRAM_SIZE) == 0);
+   slock_lock(io_lock);
+   writes = 0;
+   slock_unlock(io_lock);
+
+   slock_lock(io_lock);
+   hold_write = true;
+   slock_unlock(io_lock);
+   memset(ram, 1, SRAM_SIZE);
+   autosave_lock();
+   autosave_unlock();
+   slock_lock(io_lock);
+   for (i = 0; !writes && i < 5; i++)
+      scond_wait_timeout(io_cond, io_lock, 1000000);
+   assert(writes == 1);
+   slock_unlock(io_lock);
+
+   /* A slow disk must neither stall frames nor observe subsequent SRAM. */
+   for (i = 0; i < 100; i++)
+   {
+      memset(ram, 2, SRAM_SIZE);
+      autosave_lock();
+      autosave_unlock();
+   }
+   slock_lock(io_lock);
+   hold_write = false;
+   scond_signal(io_cond);
+   slock_unlock(io_lock);
+   wait_requested();
+   for (i = 0; i < SRAM_SIZE; i++)
+      assert(disk_image[i] == 1);
+   capture_and_wait();
+   assert(memcmp(disk_image, ram, SRAM_SIZE) == 0);
+   before = opens;
+   capture_and_wait();
+   assert(opens == before);
+
+   /* An unchanged image must be retried after any I/O failure. */
+   for (i = 1; i <= 4; i++)
+   {
+      memset(ram, (int)i + 2, SRAM_SIZE);
+      slock_lock(io_lock);
+      fail_stage = i;
+      before = opens;
+      slock_unlock(io_lock);
+      capture_and_wait();
+      assert(opens == before + 1);
+      assert(memcmp(current->buffer, ram, SRAM_SIZE) != 0);
+      slock_lock(io_lock);
+      fail_stage = 0;
+      slock_unlock(io_lock);
+      capture_and_wait();
+      assert(opens == before + 2);
+      assert(memcmp(current->buffer, ram, SRAM_SIZE) == 0);
+   }
+   /* A partial write also needs repair if SRAM reverts to the last
+    * successfully saved image before the next request. */
+   memset(ram, 9, SRAM_SIZE);
+   slock_lock(io_lock);
+   fail_stage = 2;
+   before = opens;
+   slock_unlock(io_lock);
+   capture_and_wait();
+   memcpy(ram, current->buffer, SRAM_SIZE);
+   slock_lock(io_lock);
+   fail_stage = 0;
+   slock_unlock(io_lock);
+   capture_and_wait();
+   assert(opens == before + 2);
+   assert(memcmp(disk_image, ram, SRAM_SIZE) == 0);
+   stop_save();
+
+   start_save(true);
+   memset(ram, 7, SRAM_SIZE);
+   capture_and_wait();
+   assert(compressed_opens == 1);
+   stop_save();
+   /* Shutdown is also safe before the first snapshot is published. */
+   start_save(false);
+   stop_save();
+   scond_free(io_cond);
+   slock_free(io_lock);
+   puts("autosave snapshot tests passed");
+   return 0;
+}