Skip to content

Commit dda8feb

Browse files
committed
platform_darwin.m: port OSAtomicCompareAndSwap32 to retro_atomic.h
Apple deprecated <libkern/OSAtomic.h> in macOS 10.12 (2016) in favour of C11 <stdatomic.h>. platform_darwin.m had two callers of OSAtomicCompareAndSwap32 left for the GCD-based filesystem watcher's "did anything change since last check?" flag. Replace both with retro_atomic.h primitives. Counter pattern, not CAS-flag ----------------------------- The previous design used a 32-bit flag set/cleared via CAS: - setter: OSAtomicCompareAndSwap32(0, 1, &has_changes) from the GCD vnode event handler - reader: OSAtomicCompareAndSwap32(1, 0, &has_changes) from main thread; return value answers "did at least one event fire since the last call?" The reader's CAS is load-bearing -- it has to read-and-clear atomically or risk losing a notification fired between the read and the clear. retro_atomic.h does not expose a CAS primitive; its docstring explicitly says "no compare-exchange, add only when a real caller needs it." Rather than add CAS to retro_atomic.h for this single caller, restructure the field as a monotonic counter: - setter: retro_atomic_fetch_add_int(&event_count, 1) - reader: acquire-load event_count; compare to last_seen cursor; update last_seen; return whether they differ. last_seen is main-thread-only state, no atomic primitive needed. This is strictly more flexible than the flag. The "did anything change?" semantic is preserved exactly (now != last_seen <=> at least one event), and the count is available if a future caller wants to batch events or report event volume. Multi-reader fan-out becomes possible (each reader owns its own last_seen cursor) without further changes. Wraparound: 32-bit signed int at GCD vnode-event rates wraps in centuries. The (now != last_seen) comparison remains correct across wraparound under modular int arithmetic. Backwards / forwards compatibility ---------------------------------- darwin_watch_data_t is file-private to platform_darwin.m -- not declared in any header, no external code touches it. The public path_change_data_t wrapper (frontend/frontend_driver.h) is unchanged. The frontend_ctx_driver vtable entry (check_for_path_changes) keeps its signature and bool return. Caller-observable semantic is identical: returns true exactly once per "at least one event since last call", false otherwise, no notifications lost. The internal state difference (counter vs flag) is invisible across the function boundary. Performance ----------- Reader path (called ~60 Hz from runloop.c when video_shader_watch_files is enabled, Apple-only): - Pre: one atomic CAS (1, 0). - Post: one acquire-load + two non-atomic ops. Net: strictly cheaper. ldar vs cas/ldaxr-stlxr loop on AArch64. Setter path (GCD vnode event handler, sub-millisecond/day total): - Pre: one CAS (may no-op if flag is already set). - Post: one fetch_add (always stores). Net: one extra store on the cold path. Below measurement noise at the actual event rate. Cache footprint: +4 bytes per watcher (last_seen). event_count and last_seen sit on the same cache line; setter invalidates it on every event, reader reloads next call. At 60 Hz reader vs. sparse setter, max ~3 microseconds/sec of cache-miss cost -- negligible, and the function is not on a hot path regardless. Field type and initialisation ----------------------------- Field changes from `volatile int32_t has_changes` to `retro_atomic_int_t event_count`. The retro_atomic_int_t is int-sized and int-aligned across all 7 retro_atomic.h backends (verified via the gfx_thumbnail port and retro_atomic_test), so no struct-layout impact -- but the type itself is backend-dependent: _Atomic int under C11, std::atomic<int> under C++11, plain int under MSVC, volatile int on the fallback. Plain assignment to _Atomic int is illegal under C11 stdatomic, so the calloc-then-assign-zero idiom is replaced with retro_atomic_int_init. The calloc remains; the explicit init is belt-and-braces (zero-bit-pattern is a valid initial state for every backend, but the API requires the init for type-system reasons under C11). Verified -------- - retro_atomic.h still has no CAS / exchange primitive after this commit. Audit confirms platform_darwin.m was the last OSAtomic* caller in the tree (gfx/gfx_thumbnail.h's mention is documentation referencing the backend-cascade list, not a call site). - Smoke test mirroring the changed code blocks compiles and runs clean under: gcc -O2/-Wall/-Werror, clang -x objective-c, clang -x objective-c++ -std=c++11 (CXX_BUILD-style), forced C11 stdatomic backend. Runtime: events fire -> reader returns true once, then false; subsequent events -> true once again. Idempotent on no-events. - Full platform_darwin.m compilation requires Apple SDK (Mach + Foundation + GCD + IOKit) and is not exercised on Linux CI; verification on macOS hardware needed before merge.
1 parent b894479 commit dda8feb

1 file changed

Lines changed: 74 additions & 13 deletions

File tree

frontend/drivers/platform_darwin.m

Lines changed: 74 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#ifdef HAVE_GCD
2828
#include <dispatch/dispatch.h>
2929
#include <defines/cocoa_defines.h>
30+
#include <retro_atomic.h>
3031
#endif
3132

3233
#include <CoreFoundation/CoreFoundation.h>
@@ -144,7 +145,7 @@
144145
* any already-dispatched event handler invocation keeps
145146
* running to completion on its target queue, which is the
146147
* global concurrent queue here. The event handler
147-
* dereferences &watch_data->has_changes, so we cannot free
148+
* dereferences &watch_data->event_count, so we cannot free
148149
* watch_data until we're sure no in-flight handler remains.
149150
* The cancel handler fires once all handler invocations
150151
* have drained, so waiting on this semaphore before free()
@@ -159,7 +160,27 @@
159160
dispatch_queue_t queue; /* Dispatch queue for event handlers */
160161
darwin_watch_entry_t *watches; /* Array of watch entries */
161162
size_t watch_count; /* Number of active watches */
162-
volatile int32_t has_changes; /* Atomic flag indicating changes occurred */
163+
/* Monotonic event counter. Setter (GCD event handler thread)
164+
* increments via retro_atomic_fetch_add_int when the
165+
* filesystem reports an event; reader (main thread, in
166+
* frontend_darwin_check_for_path_changes) acquire-loads it
167+
* and compares against last_seen below.
168+
*
169+
* The previous design used a binary flag set/cleared via
170+
* OSAtomicCompareAndSwap32, but Apple deprecated OSAtomic.h
171+
* in 10.12 (2016) in favour of <stdatomic.h>. Replacing the
172+
* flag with a counter is also strictly more flexible: the
173+
* "did anything change?" semantic is preserved exactly
174+
* (now != last_seen), and the count is available if a future
175+
* caller wants to batch events. retro_atomic.h provides the
176+
* portable fetch_add / load_acquire primitives needed; no
177+
* compare-exchange operation is necessary, because the
178+
* monotonic counter never needs to be read-and-cleared
179+
* atomically (last_seen is main-thread-only state). */
180+
retro_atomic_int_t event_count;
181+
/* Reader-side cursor. Touched only by the main thread in
182+
* frontend_darwin_check_for_path_changes; not atomic. */
183+
int last_seen;
163184
int flags; /* Event flags to monitor */
164185
} darwin_watch_data_t;
165186
#endif
@@ -929,16 +950,16 @@ static void darwin_watch_data_free(darwin_watch_data_t *watch_data)
929950
* handler to fire. dispatch_source_cancel is
930951
* asynchronous - it only marks the source as
931952
* cancelled. Any already-dispatched event handler
932-
* invocation (the one that reads
933-
* &watch_data->has_changes) keeps running to
953+
* invocation (the one that increments
954+
* &watch_data->event_count) keeps running to
934955
* completion on the global concurrent queue. The
935956
* cancel handler is guaranteed to fire AFTER all
936957
* pending event handler invocations have drained,
937958
* so dispatch_semaphore_wait(cancel_sem, FOREVER)
938959
* is the standard 'wait until source is fully
939960
* quiesced' pattern. Without this wait a racing
940961
* event handler would NUL-deref or, worse, write
941-
* into freed memory for has_changes. */
962+
* into freed memory for event_count. */
942963
dispatch_source_cancel(watch_data->watches[i].source);
943964
if (watch_data->watches[i].cancel_sem)
944965
{
@@ -989,7 +1010,12 @@ static void frontend_darwin_watch_path_for_changes(
9891010
watch_data->watches = (darwin_watch_entry_t*)calloc(
9901011
list->size, sizeof(darwin_watch_entry_t));
9911012
watch_data->flags = flags;
992-
watch_data->has_changes = 0;
1013+
/* watch_data was calloc'd, so event_count and last_seen are
1014+
* already zero-bit; but plain assignment to a
1015+
* retro_atomic_int_t is illegal under the C11 stdatomic
1016+
* backend, so use the init helper for the atomic field. */
1017+
retro_atomic_int_init(&watch_data->event_count, 0);
1018+
watch_data->last_seen = 0;
9931019

9941020
if (!watch_data->watches)
9951021
{
@@ -1052,12 +1078,25 @@ static void frontend_darwin_watch_path_for_changes(
10521078

10531079
if (source)
10541080
{
1055-
/* Set up event handler - sets atomic flag when changes
1056-
* occur. This block captures watch_data by pointer and
1057-
* the cancel-handler synchronisation below is what keeps
1058-
* the capture safe against the teardown path. */
1081+
/* Set up event handler - bump the atomic event
1082+
* counter when a filesystem event fires. This
1083+
* block captures watch_data by pointer and the
1084+
* cancel-handler synchronisation below is what
1085+
* keeps the capture safe against the teardown
1086+
* path.
1087+
*
1088+
* fetch_add (rather than the previous CAS(0, 1))
1089+
* always stores, but the rate is bounded by GCD
1090+
* vnode events (sub-millisecond/day in normal
1091+
* use), so the extra store cost is below
1092+
* measurement noise. In return we get a
1093+
* monotonic counter that the reader can compare
1094+
* against its last_seen cursor without losing
1095+
* notifications, and we drop the dependency on
1096+
* OSAtomic.h which Apple deprecated in 10.12. */
10591097
dispatch_source_set_event_handler(source, ^{
1060-
OSAtomicCompareAndSwap32(0, 1, &watch_data->has_changes);
1098+
retro_atomic_fetch_add_int(
1099+
&watch_data->event_count, 1);
10611100
});
10621101

10631102
/* Cancel handler signals cancel_sem. darwin_watch_
@@ -1122,8 +1161,30 @@ static bool frontend_darwin_check_for_path_changes(
11221161

11231162
watch_data = (darwin_watch_data_t*)(change_data->data);
11241163

1125-
/* Atomically read and clear the flag */
1126-
return OSAtomicCompareAndSwap32(1, 0, &watch_data->has_changes);
1164+
/* Acquire-load the producer's counter and compare against
1165+
* our reader-side cursor. If they differ, at least one
1166+
* event fired since the last call -- update the cursor and
1167+
* return true.
1168+
*
1169+
* No CAS is needed because last_seen is main-thread-only
1170+
* state. The acquire-load pairs with the producer's
1171+
* fetch_add (which has acq_rel semantics in retro_atomic.h),
1172+
* so any data the producer published before its increment
1173+
* is visible to us by the time we read here.
1174+
*
1175+
* The counter is monotonic and never reset, so over a long
1176+
* enough run it would wrap around -- but on a 32-bit signed
1177+
* int at filesystem-event rates this takes hundreds of
1178+
* years. The `now != last_seen` comparison remains correct
1179+
* across wraparound under modular int arithmetic; any non-
1180+
* zero (now - last_seen) means the producer advanced. */
1181+
{
1182+
int now = retro_atomic_load_acquire_int(
1183+
&watch_data->event_count);
1184+
bool changed = (now != watch_data->last_seen);
1185+
watch_data->last_seen = now;
1186+
return changed;
1187+
}
11271188
}
11281189
#endif
11291190

0 commit comments

Comments
 (0)