From 6b2d44a58c54f704c5d26c1acc14f0594aa4f806 Mon Sep 17 00:00:00 2001 From: Aaron Fenyes Date: Mon, 18 Nov 2024 16:08:06 -0800 Subject: [PATCH 1/6] Assign each element a serial number For future thread-safety, keep the next serial number in a static atomic variable. --- app-proto/src/assembly.rs | 9 ++++++++- app-proto/src/outline.rs | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/app-proto/src/assembly.rs b/app-proto/src/assembly.rs index 35b4417..a00081a 100644 --- a/app-proto/src/assembly.rs +++ b/app-proto/src/assembly.rs @@ -1,7 +1,7 @@ use nalgebra::{DMatrix, DVector}; use rustc_hash::FxHashMap; use slab::Slab; -use std::collections::BTreeSet; +use std::{collections::BTreeSet, sync::atomic::{AtomicU64, Ordering}}; use sycamore::prelude::*; use web_sys::{console, wasm_bindgen::JsValue}; /* DEBUG */ @@ -13,6 +13,8 @@ pub type ConstraintKey = usize; pub type ElementColor = [f32; 3]; +static NEXT_ELEMENT_SERIAL: AtomicU64 = AtomicU64::new(0); + #[derive(Clone, PartialEq)] pub struct Element { pub id: String, @@ -20,6 +22,10 @@ pub struct Element { pub color: ElementColor, pub representation: Signal>, pub constraints: Signal>, + + // a serial number, assigned by `Element::new`, that uniquely identifies + // each element (until `NEXT_ELEMENT_SERIAL` wraps around) + pub serial: u64, // the configuration matrix column index that was assigned to this element // last time the assembly was realized @@ -39,6 +45,7 @@ impl Element { color: color, representation: create_signal(representation), constraints: create_signal(BTreeSet::default()), + serial: NEXT_ELEMENT_SERIAL.fetch_add(1, Ordering::SeqCst), column_index: 0 } } diff --git a/app-proto/src/outline.rs b/app-proto/src/outline.rs index ee1603f..e2cf49c 100644 --- a/app-proto/src/outline.rs +++ b/app-proto/src/outline.rs @@ -200,7 +200,7 @@ pub fn Outline() -> View { view=|(key, elt)| view! { ElementOutlineItem(key=key, element=elt) }, - key=|(key, _)| key.clone() + key=|(_, elt)| elt.serial ) } } -- 2.43.0 From 7bc3a9eeae91f719d441dff0eeb1bef6c6e1f89a Mon Sep 17 00:00:00 2001 From: Aaron Fenyes Date: Mon, 18 Nov 2024 20:29:53 -0800 Subject: [PATCH 2/6] Make the next element serial number thread-local In the last revision, the next element serial number was thread-safe, but that might be overdoing it. I suspect that elements will only ever be created from the main thread. --- app-proto/src/assembly.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/app-proto/src/assembly.rs b/app-proto/src/assembly.rs index a00081a..bfbb2b8 100644 --- a/app-proto/src/assembly.rs +++ b/app-proto/src/assembly.rs @@ -1,7 +1,7 @@ use nalgebra::{DMatrix, DVector}; use rustc_hash::FxHashMap; use slab::Slab; -use std::{collections::BTreeSet, sync::atomic::{AtomicU64, Ordering}}; +use std::{cell::Cell, collections::BTreeSet}; use sycamore::prelude::*; use web_sys::{console, wasm_bindgen::JsValue}; /* DEBUG */ @@ -13,7 +13,9 @@ pub type ConstraintKey = usize; pub type ElementColor = [f32; 3]; -static NEXT_ELEMENT_SERIAL: AtomicU64 = AtomicU64::new(0); +thread_local! { + static NEXT_ELEMENT_SERIAL: Cell = Cell::new(0); +} #[derive(Clone, PartialEq)] pub struct Element { @@ -39,13 +41,17 @@ impl Element { color: ElementColor, representation: DVector ) -> Element { + // take the next serial number + let serial = NEXT_ELEMENT_SERIAL.get(); + NEXT_ELEMENT_SERIAL.set(serial.wrapping_add(1)); + Element { id: id, label: label, color: color, representation: create_signal(representation), constraints: create_signal(BTreeSet::default()), - serial: NEXT_ELEMENT_SERIAL.fetch_add(1, Ordering::SeqCst), + serial: serial, column_index: 0 } } -- 2.43.0 From b0bd31a9da72ecb135db79618da7ebf52c21568f Mon Sep 17 00:00:00 2001 From: Aaron Fenyes Date: Thu, 21 Nov 2024 15:55:40 -0800 Subject: [PATCH 3/6] Go back to atomic for next element serial number This reverts commit 7bc3a9eeae91f719d441dff0eeb1bef6c6e1f89a. I'd hoped that `thread_local!` would force our code to be single- threaded, but apparently it doesn't. With a global mutable static, it seems like we have to include some kind of thread-safety to avoid `unsafe` code, and an atomic provides the kind of safety we actually want. --- app-proto/src/assembly.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/app-proto/src/assembly.rs b/app-proto/src/assembly.rs index bfbb2b8..a00081a 100644 --- a/app-proto/src/assembly.rs +++ b/app-proto/src/assembly.rs @@ -1,7 +1,7 @@ use nalgebra::{DMatrix, DVector}; use rustc_hash::FxHashMap; use slab::Slab; -use std::{cell::Cell, collections::BTreeSet}; +use std::{collections::BTreeSet, sync::atomic::{AtomicU64, Ordering}}; use sycamore::prelude::*; use web_sys::{console, wasm_bindgen::JsValue}; /* DEBUG */ @@ -13,9 +13,7 @@ pub type ConstraintKey = usize; pub type ElementColor = [f32; 3]; -thread_local! { - static NEXT_ELEMENT_SERIAL: Cell = Cell::new(0); -} +static NEXT_ELEMENT_SERIAL: AtomicU64 = AtomicU64::new(0); #[derive(Clone, PartialEq)] pub struct Element { @@ -41,17 +39,13 @@ impl Element { color: ElementColor, representation: DVector ) -> Element { - // take the next serial number - let serial = NEXT_ELEMENT_SERIAL.get(); - NEXT_ELEMENT_SERIAL.set(serial.wrapping_add(1)); - Element { id: id, label: label, color: color, representation: create_signal(representation), constraints: create_signal(BTreeSet::default()), - serial: serial, + serial: NEXT_ELEMENT_SERIAL.fetch_add(1, Ordering::SeqCst), column_index: 0 } } -- 2.43.0 From c5f09b99b393d59dd8fb184526ad4c39620b3f15 Mon Sep 17 00:00:00 2001 From: Aaron Fenyes Date: Thu, 21 Nov 2024 16:03:58 -0800 Subject: [PATCH 4/6] Add reminder to reconsider global element serials --- app-proto/src/assembly.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app-proto/src/assembly.rs b/app-proto/src/assembly.rs index a00081a..92eaa62 100644 --- a/app-proto/src/assembly.rs +++ b/app-proto/src/assembly.rs @@ -13,6 +13,11 @@ pub type ConstraintKey = usize; pub type ElementColor = [f32; 3]; +/* KLUDGE */ +// we should reconsider this design when we build a system for switching between +// assemblies. at that point, we might want to switch to hierarchical keys, +// where each each element has a key that identifies it within its assembly and +// each assembly has a key that identifies it within the sesssion static NEXT_ELEMENT_SERIAL: AtomicU64 = AtomicU64::new(0); #[derive(Clone, PartialEq)] -- 2.43.0 From 133b725053f8c925f416d7e6e7f6f91b11be62b7 Mon Sep 17 00:00:00 2001 From: Aaron Fenyes Date: Thu, 21 Nov 2024 16:42:43 -0800 Subject: [PATCH 5/6] Panic if we run out of serial numbers --- app-proto/src/assembly.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/app-proto/src/assembly.rs b/app-proto/src/assembly.rs index 92eaa62..8cf3bb3 100644 --- a/app-proto/src/assembly.rs +++ b/app-proto/src/assembly.rs @@ -44,13 +44,24 @@ impl Element { color: ElementColor, representation: DVector ) -> Element { + // take the next serial number, panicking if that was the last number we + // had left. the technique we use to panic on overflow is taken from + // _Rust Atomics and Locks_, by Mara Bos + // + // https://marabos.nl/atomics/atomics.html#example-handle-overflow + // + let serial = NEXT_ELEMENT_SERIAL.fetch_update( + Ordering::SeqCst, Ordering::SeqCst, + |serial| serial.checked_add(1) + ).expect("Out of serial numbers for elements"); + Element { id: id, label: label, color: color, representation: create_signal(representation), constraints: create_signal(BTreeSet::default()), - serial: NEXT_ELEMENT_SERIAL.fetch_add(1, Ordering::SeqCst), + serial: serial, column_index: 0 } } -- 2.43.0 From a5fd6545e05868f673f5f774a371c53798703779 Mon Sep 17 00:00:00 2001 From: Aaron Fenyes Date: Thu, 21 Nov 2024 16:47:45 -0800 Subject: [PATCH 6/6] Remove caveat about `NEXT_ELEMENT_SERIAL` wrapping Commit 133b725 should make it impossible for the serial numbers to wrap. --- app-proto/src/assembly.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app-proto/src/assembly.rs b/app-proto/src/assembly.rs index 8cf3bb3..59cba41 100644 --- a/app-proto/src/assembly.rs +++ b/app-proto/src/assembly.rs @@ -29,7 +29,7 @@ pub struct Element { pub constraints: Signal>, // a serial number, assigned by `Element::new`, that uniquely identifies - // each element (until `NEXT_ELEMENT_SERIAL` wraps around) + // each element pub serial: u64, // the configuration matrix column index that was assigned to this element -- 2.43.0