Sync memory between emulator thread and UI

src/emulator.rs

@@ -7,7 +7,7 @@ use std::{
     sync::{
         atomic::{AtomicBool, Ordering},
         mpsc::{self, RecvError, TryRecvError},
-        Arc,
+        Arc, Weak,
     },
 };
 
@@ -17,7 +17,11 @@ use bytemuck::NoUninit;
 use egui_toast::{Toast, ToastKind, ToastOptions};
 use tracing::{error, warn};
 
-use crate::{audio::Audio, graphics::TextureSink};
+use crate::{
+    audio::Audio,
+    graphics::TextureSink,
+    memory::{MemoryRange, MemoryRegion},
+};
 use shrooms_vb_core::{Sim, StopReason, EXPECTED_FRAME_SIZE};
 pub use shrooms_vb_core::{VBKey, VBRegister, VBWatchpointType};
 
@@ -165,8 +169,10 @@ pub struct Emulator {
     renderers: HashMap<SimId, TextureSink>,
     messages: HashMap<SimId, mpsc::Sender<Toast>>,
     debuggers: HashMap<SimId, DebugInfo>,
+    watched_regions: HashMap<MemoryRange, Weak<MemoryRegion>>,
     eye_contents: Vec<u8>,
     audio_samples: Vec<f32>,
+    buffer: Vec<u8>,
 }
 
 impl Emulator {
@@ -189,8 +195,10 @@ impl Emulator {
             renderers: HashMap::new(),
             messages: HashMap::new(),
             debuggers: HashMap::new(),
+            watched_regions: HashMap::new(),
             eye_contents: vec![0u8; 384 * 224 * 2],
             audio_samples: Vec::with_capacity(EXPECTED_FRAME_SIZE),
+            buffer: vec![],
         })
     }
 
@@ -367,6 +375,10 @@ impl Emulator {
         }
     }
 
+    fn watch_memory(&mut self, range: MemoryRange, region: Weak<MemoryRegion>) {
+        self.watched_regions.insert(range, region);
+    }
+
     pub fn run(&mut self) {
         loop {
             let idle = self.tick();
@@ -391,6 +403,18 @@ impl Emulator {
                     }
                 }
             }
+            self.watched_regions.retain(|range, region| {
+                let Some(region) = region.upgrade() else {
+                    return false;
+                };
+                let Some(sim) = self.sims.get_mut(range.sim.to_index()) else {
+                    return false;
+                };
+                self.buffer.clear();
+                sim.read_memory(range.start, range.length, &mut self.buffer);
+                region.update(&self.buffer);
+                true
+            });
         }
     }
 
@@ -557,6 +581,9 @@ impl Emulator {
                 sim.write_memory(start, &buffer);
                 let _ = done.send(buffer);
             }
+            EmulatorCommand::WatchMemory(range, region) => {
+                self.watch_memory(range, region);
+            }
             EmulatorCommand::AddBreakpoint(sim_id, address) => {
                 let Some(sim) = self.sims.get_mut(sim_id.to_index()) else {
                     return;
@@ -647,6 +674,7 @@ pub enum EmulatorCommand {
     WriteRegister(SimId, VBRegister, u32),
     ReadMemory(SimId, u32, usize, Vec<u8>, oneshot::Sender<Vec<u8>>),
     WriteMemory(SimId, u32, Vec<u8>, oneshot::Sender<Vec<u8>>),
+    WatchMemory(MemoryRange, Weak<MemoryRegion>),
     AddBreakpoint(SimId, u32),
     RemoveBreakpoint(SimId, u32),
     AddWatchpoint(SimId, u32, usize, VBWatchpointType),

src/memory.rs

@@ -1,15 +1,18 @@
 use std::{
     collections::HashMap,
-    sync::{Arc, Mutex, MutexGuard},
+    fmt::Debug,
+    sync::{atomic::AtomicU64, Arc, RwLock, RwLockReadGuard, TryLockError, Weak},
 };
 
 use bytemuck::BoxBytes;
+use itertools::Itertools;
+use tracing::warn;
 
 use crate::emulator::{EmulatorClient, EmulatorCommand, SimId};
 
 pub struct MemoryMonitor {
     client: EmulatorClient,
-    regions: HashMap<MemoryRegion, Arc<Mutex<BoxBytes>>>,
+    regions: HashMap<MemoryRange, Weak<MemoryRegion>>,
 }
 
 impl MemoryMonitor {
@@ -21,20 +24,19 @@ impl MemoryMonitor {
     }
 
     pub fn view(&mut self, sim: SimId, start: u32, length: usize) -> MemoryView {
-        let region = MemoryRegion { sim, start, length };
+        let range = MemoryRange { sim, start, length };
-        let memory = self.regions.entry(region).or_insert_with(|| {
+        let region = self
-            let mut buf = aligned_memory(start, length);
+            .regions
-            let (tx, rx) = oneshot::channel();
+            .get(&range)
-            self.client
+            .and_then(|r| r.upgrade())
-                .send_command(EmulatorCommand::ReadMemory(sim, start, length, vec![], tx));
+            .unwrap_or_else(|| {
-            let bytes = pollster::block_on(rx).unwrap();
+                let region = Arc::new(MemoryRegion::new(start, length));
-            buf.copy_from_slice(&bytes);
+                self.regions.insert(range, Arc::downgrade(&region));
-            #[expect(clippy::arc_with_non_send_sync)] // TODO: remove after bytemuck upgrade
+                self.client
-            Arc::new(Mutex::new(buf))
+                    .send_command(EmulatorCommand::WatchMemory(range, Arc::downgrade(&region)));
-        });
+                region
-        MemoryView {
+            });
-            memory: memory.clone(),
+        MemoryView { region }
-        }
     }
 }
 
@@ -52,21 +54,17 @@ fn aligned_memory(start: u32, length: usize) -> BoxBytes {
 }
 
 pub struct MemoryView {
-    memory: Arc<Mutex<BoxBytes>>,
+    region: Arc<MemoryRegion>,
 }
-// SAFETY: BoxBytes is supposed to be Send+Sync, will be in a new version
-unsafe impl Send for MemoryView {}
 
 impl MemoryView {
     pub fn borrow(&self) -> MemoryRef<'_> {
-        MemoryRef {
+        self.region.borrow()
-            inner: self.memory.lock().unwrap(),
-        }
     }
 }
 
 pub struct MemoryRef<'a> {
-    inner: MutexGuard<'a, BoxBytes>,
+    inner: RwLockReadGuard<'a, BoxBytes>,
 }
 
 impl MemoryRef<'_> {
@@ -83,9 +81,102 @@ impl MemoryRef<'_> {
     }
 }
 
-#[derive(Clone, Copy, PartialEq, Eq, Hash)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
-struct MemoryRegion {
+pub struct MemoryRange {
-    sim: SimId,
+    pub sim: SimId,
-    start: u32,
+    pub start: u32,
-    length: usize,
+    pub length: usize,
+}
+
+const BUFFERS: usize = 4;
+pub struct MemoryRegion {
+    gens: [AtomicU64; BUFFERS],
+    bufs: [RwLock<BoxBytes>; BUFFERS],
+}
+impl Debug for MemoryRegion {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("MemoryRegion")
+            .field("gens", &self.gens)
+            .finish_non_exhaustive()
+    }
+}
+// SAFETY: BoxBytes is meant to be Send+Sync, will be in a future version
+unsafe impl Send for MemoryRegion {}
+// SAFETY: BoxBytes is meant to be Send+Sync, will be in a future version
+unsafe impl Sync for MemoryRegion {}
+
+impl MemoryRegion {
+    fn new(start: u32, length: usize) -> Self {
+        Self {
+            gens: std::array::from_fn(|i| AtomicU64::new(i as u64)),
+            bufs: std::array::from_fn(|_| RwLock::new(aligned_memory(start, length))),
+        }
+    }
+
+    pub fn borrow(&self) -> MemoryRef<'_> {
+        /*
+         * When reading memory, a thread will grab the newest buffer (with the highest gen)
+         * It will only fail to grab the lock if the writer already has it,
+         * but the writer prioritizes older buffers (with lower gens).
+         * So this method will only block if the writer produces three full buffers
+         * in the time it takes the reader to do four atomic reads and grab a lock.
+         * In the unlikely event this happens... just try again.
+         */
+        loop {
+            let newest_index = self
+                .gens
+                .iter()
+                .map(|i| i.load(std::sync::atomic::Ordering::Acquire))
+                .enumerate()
+                .max_by_key(|(_, gen)| *gen)
+                .map(|(i, _)| i)
+                .unwrap();
+            let inner = match self.bufs[newest_index].try_read() {
+                Ok(inner) => inner,
+                Err(TryLockError::Poisoned(e)) => e.into_inner(),
+                Err(TryLockError::WouldBlock) => {
+                    continue;
+                }
+            };
+            break MemoryRef { inner };
+        }
+    }
+
+    pub fn update(&self, data: &[u8]) {
+        let gens: Vec<u64> = self
+            .gens
+            .iter()
+            .map(|i| i.load(std::sync::atomic::Ordering::Acquire))
+            .collect();
+        let next_gen = gens.iter().max().unwrap() + 1;
+        let indices = gens
+            .into_iter()
+            .enumerate()
+            .sorted_by_key(|(_, val)| *val)
+            .map(|(i, _)| i);
+        for index in indices {
+            let mut lock = match self.bufs[index].try_write() {
+                Ok(inner) => inner,
+                Err(TryLockError::Poisoned(e)) => e.into_inner(),
+                Err(TryLockError::WouldBlock) => {
+                    continue;
+                }
+            };
+            lock.copy_from_slice(data);
+            self.gens[index].store(next_gen, std::sync::atomic::Ordering::Release);
+            return;
+        }
+        /*
+         * We have four buffers, and (at time of writing) only three threads interacting with memory:
+         * - The UI thread, reading small regions of memory
+         * - The "vram renderer" thread, reading large regions of memory
+         * - The emulation thread, writing memory every so often
+         * So it should be impossible for all four buffers to have a read lock at the same time,
+         *  and (because readers always read the newest buffer) at least one of the oldest three
+         *  buffers will be free the entire time we're in this method.
+         * TL;DR this should never happen.
+         * But if it does, do nothing. This isn't medical software, better to show stale data than crash.
+         */
+        warn!("all buffers were locked by a reader at the same time")
+    }
 }

src/window/vram/chardata.rs

@@ -359,100 +359,3 @@ impl CharDataRenderer {
         utils::parse_palette(palette, brts)
     }
 }
-
-/*
-struct CharDataLoader {
-    chardata: MemoryView,
-    brightness: MemoryView,
-    palettes: MemoryView,
-}
-
-impl CharDataLoader {
-    pub fn new(sim_id: SimId, memory: &mut MemoryMonitor) -> Self {
-        Self {
-            chardata: memory.view(sim_id, 0x00078000, 0x8000),
-            brightness: memory.view(sim_id, 0x0005f824, 8),
-            palettes: memory.view(sim_id, 0x0005f860, 16),
-        }
-    }
-
-    fn update_character(&self, image: &mut VramImage, palette: VramPalette, index: usize) {
-        if index >= 2048 {
-            return;
-        }
-        let palette = self.load_palette(palette);
-        let chardata = self.chardata.borrow();
-        let character = chardata.range::<u16>(index * 8, 8);
-        for (row, pixels) in character.iter().enumerate() {
-            for col in 0..8 {
-                let char = (pixels >> (col * 2)) & 0x03;
-                image.write((col, row), palette[char as usize]);
-            }
-        }
-    }
-
-    fn update_character_data(&self, image: &mut VramImage, palette: VramPalette) {
-        let palette = self.load_palette(palette);
-        let chardata = self.chardata.borrow();
-        for (row, pixels) in chardata.range::<u16>(0, 8 * 2048).iter().enumerate() {
-            let char_index = row / 8;
-            let row_index = row % 8;
-            let x = (char_index % 16) * 8;
-            let y = (char_index / 16) * 8 + row_index;
-            for col in 0..8 {
-                let char = (pixels >> (col * 2)) & 0x03;
-                image.write((x + col, y), palette[char as usize]);
-            }
-        }
-    }
-
-    fn load_palette(&self, palette: VramPalette) -> [u8; 4] {
-        let Some(offset) = palette.offset() else {
-            return utils::GENERIC_PALETTE;
-        };
-        let palette = self.palettes.borrow().read(offset);
-        let brightnesses = self.brightness.borrow();
-        let brts = brightnesses.range(0, 8);
-        utils::parse_palette(palette, brts)
-    }
-}
-
-impl VramImageLoader for CharDataLoader {
-    type Resource = CharDataResource;
-
-    fn id(&self) -> &str {
-        concat!(module_path!(), "::CharDataLoader")
-    }
-
-    fn add(&self, resource: &Self::Resource) -> Option<VramImage> {
-        match resource {
-            CharDataResource::Character { palette, index } => {
-                let mut image = VramImage::new(8, 8);
-                self.update_character(&mut image, *palette, *index);
-                Some(image)
-            }
-            CharDataResource::CharacterData { palette } => {
-                let mut image = VramImage::new(8 * 16, 8 * 128);
-                self.update_character_data(&mut image, *palette);
-                Some(image)
-            }
-        }
-    }
-
-    fn update<'a>(
-        &'a self,
-        resources: impl Iterator<Item = (&'a Self::Resource, &'a mut VramImage)>,
-    ) {
-        for (resource, image) in resources {
-            match resource {
-                CharDataResource::Character { palette, index } => {
-                    self.update_character(image, *palette, *index)
-                }
-                CharDataResource::CharacterData { palette } => {
-                    self.update_character_data(image, *palette)
-                }
-            }
-        }
-    }
-}
- */