From 9fddf0b6aa242c7f0b98985ecff15e56983fdc6f Mon Sep 17 00:00:00 2001 From: meh Date: Tue, 22 Sep 2026 11:55:57 +0700 Subject: [PATCH 1/3] Stop the drain loop paying half a second for a retry The engine's drain loop responds to a gapped event stream by sleeping 500 ms and trying the collection again. That is not waiting for work to finish: it is waiting for an event that the escalation after `COLLECT_WHOLE_QUEUE_AFTER_ATTEMPTS` makes unnecessary anyway, so the only thing the sleep buys is not spinning while that plays out. Half a second is far more than that is worth. Measured in a downstream daemon, release build, settled store, timing SIGTERM to process exit against the number of indexed files: 600 files 0.356 / 0.348 / 0.365 s 700 files 0.418 s 800 files 1.445 / 1.418 s 1000 files 1.480 s 1200 files 1.600 / 1.572 / 1.556 s 1400 files 2.030 s A step of 1.03 s between 700 and 800 files, then a plateau that rises only at the underlying linear rate of about 0.06 s per 100 files. A page drain cannot step by a second and then flatten. The riser is two of these sleeps. A shutdown that caught a write in flight cost the same as a settled one, 1.572 against 1.556, which is what says the hole is the ordinary event-order inversion rather than an unfinished write. It is worst during `close`, where it is futile by construction. Closing refuses every push from then on, so the event the hole is waiting for can never be queued, and the loop can only finish through the escalation. Every sleep on that path is time bought for nothing. 100 ms, and the constant is named rather than inlined so the next person can see what it is and what it costs. **This is the interim number, not the right mechanism.** The loop should wait on `progress_notify`, which is already signalled both when the queue drains and when the lifecycle moves, `close` included, so it would wake on the event instead of guessing at it. `wait_for_ops` already waits that way. Doing it here means reshaping the loop, which is a larger change than this one, so the constant comes down now and the mechanism follows separately. No behaviour changes beyond the wait: the collection, the escalation and `validate_events` refusing a stream with a hole are all untouched. --- src/persistence/task.rs | 53 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/src/persistence/task.rs b/src/persistence/task.rs index fbb70ffa..22fe528a 100644 --- a/src/persistence/task.rs +++ b/src/persistence/task.rs @@ -72,6 +72,27 @@ const MAX_BATCH_OPERATIONS: usize = 512; /// contiguous prefix than a partial one, never something unsafe. const COLLECT_WHOLE_QUEUE_AFTER_ATTEMPTS: usize = 4; +/// How long the drain loop waits before retrying a collection that found a hole. +/// +/// **This is a backoff, not a barrier, and the number is interim.** It was 500 ms, which +/// is far longer than the wait is ever worth: the loop is not waiting for work to finish, +/// it is waiting for an event that a later escalation will make unnecessary anyway, so the +/// only thing the constant buys is not spinning while that plays out. +/// +/// It was measured downstream as the dominant cost of shutting a daemon down. A settled +/// store stepped from 0.42 s at 700 indexed files to 1.45 s at 800 and then flattened, +/// rising after that only at the underlying linear rate of about 0.06 s per 100 files. A +/// page drain cannot step by a second and then flatten, and the riser was two of these +/// sleeps. During `close` it is worse than merely long: closing refuses every push, so the +/// event the hole waits for can never be queued and each sleep is time bought for nothing. +/// +/// The right mechanism is to wait on `progress_notify`, which is signalled both when the +/// queue drains and when the lifecycle moves, `close` included, so the loop would wake on +/// the event rather than guess at it. `wait_for_ops` already waits that way. Doing it here +/// means reshaping this loop, so this reduces the constant now and leaves the mechanism to +/// its own change. +const RETRY_BACKOFF: Duration = Duration::from_millis(100); + #[derive(Debug)] struct PersistenceLifecycle { state: ParkingMutex, @@ -2140,7 +2161,37 @@ impl // Sleeping on the escalating retries instead charged // 500 ms for each step towards the fallback that fixes // them, which is how a 0.03s drain became 290s. - nagoya::sleep(Duration::from_millis(500)).await; + // + // **500 ms was far too long, and this is the interim + // number rather than the right mechanism.** Measured + // downstream: shutdown of a settled daemon stepped from + // 0.42 s at 700 indexed files to 1.45 s at 800 and then + // flattened, rising after that only at the underlying + // linear rate of about 0.06 s per 100 files. A page + // drain cannot step by a second and then flatten, so the + // riser was two of these sleeps. A mid-write shutdown + // cost the same as a settled one, which is what says the + // hole is the ordinary event-order inversion rather than + // an unfinished write. + // + // It is worst during `close`, where it is futile by + // construction: closing refuses every push from then on, + // so the event this hole waits for can never be queued, + // and the loop can only finish through the escalation + // that `COLLECT_WHOLE_QUEUE_AFTER_ATTEMPTS` already + // guarantees. Every sleep on that path is time bought + // for nothing. + // + // The proper fix is to wait on progress rather than on a + // clock: `progress_notify` is notified both when the + // queue drains and when the lifecycle moves, including + // by `close`, so a notified wait with this as a backstop + // would wake on the event instead of guessing at it. + // `wait_for_ops` already does exactly that. That is a + // larger change to this loop than a release wants to + // carry, so this reduces the constant now and leaves the + // mechanism for its own change. + nagoya::sleep(RETRY_BACKOFF).await; } } else if let Some(page_ids) = pending_reclaim.take() { // `get_first_op_id_available() == None` is only sufficient From a3635fc8ebaddc17c82a25a7855942e8df39ba6b Mon Sep 17 00:00:00 2001 From: meh Date: Tue, 22 Sep 2026 12:20:54 +0700 Subject: [PATCH 2/3] Wait for the push that carries a missing event, not for a clock The drain loop deferred on a gap in the event stream by sleeping. The previous commit cut that sleep from 500ms to 100ms, which was the interim number and not the mechanism. This is the mechanism. Queue::await_more_operations registers on the queue's own wake-up and returns the instant an operation is pushed, which is the only way the missing event can arrive. Two things end it early: - A non-empty queue. The push may have landed while the collection that found the hole was still running, so the event is already here and waiting for it waits for something that has happened. The waiter is registered before the queue is read, so a push landing between the two wakes it rather than being missed. - Any state other than Running. This one is provable, not a heuristic: a push takes the lifecycle lock and is refused unless the state is Running, and begin_close takes the same lock, so once Closing has been observed here no further push can ever be accepted. The event cannot arrive, and every moment spent waiting for it is bought for nothing. That was the shutdown case: a settled store stepped from 0.42s at 700 indexed files to 1.45s at 800 and then flattened, and the riser was two of these sleeps. What is left of the number is a backstop against a wake lost to a race with no later push to deliver another, and it is no longer a constant. It is PersistenceConfig::event_gap_wait_cap, a provided method defaulting to DEFAULT_EVENT_GAP_WAIT_CAP, so no implementor breaks and a table whose producers are genuinely slower than the default assumes can say so rather than having one number decide for every table at once. It is read once when the table starts: a parameter that changed under a running drain loop would be worse to explain than one that is fixed. GIVE_UP_AFTER_ATTEMPTS counts deferrals, not time, and its doc claimed "about a minute" from 500ms x 120. That stopped being true, so it now states the relationship instead: the cap times the count is the worst case before a stalled gap fails the table. --- src/persistence/mod.rs | 27 ++++++ src/persistence/operation/batch.rs | 16 ++-- src/persistence/task.rs | 131 +++++++++++++++++------------ 3 files changed, 116 insertions(+), 58 deletions(-) diff --git a/src/persistence/mod.rs b/src/persistence/mod.rs index e1d1ccf2..618d57bf 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -118,10 +118,37 @@ mod space; mod task; // TODO: remove this +/// Backstop for [`PersistenceConfig::event_gap_wait_cap`]. +/// +/// The worker waits on the queue's own wake-up, so this is only reached when no +/// push arrives at all and no wake was delivered. It is short because nothing is +/// bought by making it long: the wait exists to avoid spinning, not to give a +/// producer time, and the producer's own push is what ends it. +pub const DEFAULT_EVENT_GAP_WAIT_CAP: core::time::Duration = core::time::Duration::from_millis(100); + pub trait PersistenceConfig { fn table_path(&self) -> &str; fn version(&self) -> u32; + + /// Longest the persistence worker parks waiting for an index event that is + /// missing from the queue. + /// + /// The wait itself is event-driven: the worker registers on the queue's + /// wake-up and returns the moment an operation is pushed, so on a table + /// whose producers are live this cap is never reached and the value does + /// not matter. It bounds the one case the wake cannot cover, a wake lost + /// to a race, and it is the only place a number is still guessed. + /// + /// Override it per table when this table's producers are slower than the + /// default assumes -- a remote or batch producer that can genuinely be a + /// second between pushes -- or shorten it when a stall must surface fast. + /// Note what it does *not* bound: the worker gives up on a gap after a + /// fixed number of waits, so this cap multiplied by that count is the + /// worst-case time before a stalled gap fails the table. + fn event_gap_wait_cap(&self) -> core::time::Duration { + DEFAULT_EVENT_GAP_WAIT_CAP + } } /// Controls the consistency checks applied while loading persisted state. diff --git a/src/persistence/operation/batch.rs b/src/persistence/operation/batch.rs index 22903ec1..3771d381 100644 --- a/src/persistence/operation/batch.rs +++ b/src/persistence/operation/batch.rs @@ -23,11 +23,17 @@ use crate::prelude::{Order, SelectQueryExecutor}; /// fails the table. /// /// A gap is usually transient: the operation carrying the missing id has been -/// pushed but not yet batched, or its producer has not reached its push. Each -/// deferral sleeps 500ms in the worker loop, so this is about a minute of -/// waiting. The previous value of eight was about four seconds, which a -/// producer descheduled under load can lose, and the engine then blamed a -/// permanent bug for what was a slow thread. +/// pushed but not yet batched, or its producer has not reached its push. +/// +/// This counts deferrals, not time, and the difference matters now that the +/// worker waits on the push rather than on a clock. A deferral that ends +/// because the awaited operation arrived costs whatever the producer took; one +/// that ends on the backstop costs +/// [`PersistenceConfig::event_gap_wait_cap`](crate::persistence::PersistenceConfig::event_gap_wait_cap), +/// so that cap times this count is the worst case before a stalled gap fails +/// the table. It was raised from eight for a reason that still holds: eight was +/// about four seconds, which a producer descheduled under load can lose, and +/// the engine then blamed a permanent bug for what was a slow thread. /// /// Widening the collection is a *separate* decision, taken far sooner and /// tracked by the analyzer's own no-progress counter. This one only decides diff --git a/src/persistence/task.rs b/src/persistence/task.rs index 22fe528a..c6e6d7e1 100644 --- a/src/persistence/task.rs +++ b/src/persistence/task.rs @@ -72,26 +72,21 @@ const MAX_BATCH_OPERATIONS: usize = 512; /// contiguous prefix than a partial one, never something unsafe. const COLLECT_WHOLE_QUEUE_AFTER_ATTEMPTS: usize = 4; -/// How long the drain loop waits before retrying a collection that found a hole. -/// -/// **This is a backoff, not a barrier, and the number is interim.** It was 500 ms, which -/// is far longer than the wait is ever worth: the loop is not waiting for work to finish, -/// it is waiting for an event that a later escalation will make unnecessary anyway, so the -/// only thing the constant buys is not spinning while that plays out. -/// -/// It was measured downstream as the dominant cost of shutting a daemon down. A settled -/// store stepped from 0.42 s at 700 indexed files to 1.45 s at 800 and then flattened, -/// rising after that only at the underlying linear rate of about 0.06 s per 100 files. A -/// page drain cannot step by a second and then flatten, and the riser was two of these -/// sleeps. During `close` it is worse than merely long: closing refuses every push, so the -/// event the hole waits for can never be queued and each sleep is time bought for nothing. -/// -/// The right mechanism is to wait on `progress_notify`, which is signalled both when the -/// queue drains and when the lifecycle moves, `close` included, so the loop would wake on -/// the event rather than guess at it. `wait_for_ops` already waits that way. Doing it here -/// means reshaping this loop, so this reduces the constant now and leaves the mechanism to -/// its own change. -const RETRY_BACKOFF: Duration = Duration::from_millis(100); +// History of the wait the drain loop no longer performs, kept because the number it +// used is a tempting thing to reintroduce. +// +// The loop used to sleep a flat 500 ms whenever a collection found a hole in the event +// stream. That was a clock standing in for an event, and it was measured downstream as +// the dominant cost of shutting a daemon down: a settled store stepped from 0.42 s at +// 700 indexed files to 1.45 s at 800 and then flattened, rising after that only at the +// underlying linear rate of about 0.06 s per 100 files. A page drain cannot step by a +// second and then flatten; the riser was two of these sleeps. +// +// It is now `Queue::await_more_operations`, which waits on the queue's own wake-up and +// returns the instant the missing operation is pushed. The one number left is a +// backstop against a lost wake, is reached only when nothing is pushed at all, and is a +// per-table parameter rather than a constant: `PersistenceConfig::event_gap_wait_cap`, +// defaulting to `crate::persistence::DEFAULT_EVENT_GAP_WAIT_CAP`. #[derive(Debug)] struct PersistenceLifecycle { @@ -1792,6 +1787,55 @@ impl Queue Option> { if let Some(v) = self.queue.lock().pop_front() { self.len.fetch_sub(1, Ordering::Release); @@ -2053,6 +2097,11 @@ impl AvailableIndexes: Copy + Clone + Debug + Hash + Eq + Send + Sync + 'static, { let table_path = engine.config().table_path().to_owned(); + // Read once, here, rather than per wait: the worker owns the engine + // from this point on, and a parameter that could change under a + // running drain loop would be a worse thing to explain than a + // parameter that is fixed when the table starts. + let event_gap_wait_cap = engine.config().event_gap_wait_cap(); let lifecycle = Arc::new(PersistenceLifecycle::new()); let queue = Arc::new(Queue::new(lifecycle.clone(), &table_path)); @@ -2158,40 +2207,16 @@ impl // Only here is waiting the right thing: collection has // already taken the whole queue and the stream still // has a hole, so the event it needs is not yet queued. - // Sleeping on the escalating retries instead charged - // 500 ms for each step towards the fallback that fixes - // them, which is how a 0.03s drain became 290s. - // - // **500 ms was far too long, and this is the interim - // number rather than the right mechanism.** Measured - // downstream: shutdown of a settled daemon stepped from - // 0.42 s at 700 indexed files to 1.45 s at 800 and then - // flattened, rising after that only at the underlying - // linear rate of about 0.06 s per 100 files. A page - // drain cannot step by a second and then flatten, so the - // riser was two of these sleeps. A mid-write shutdown - // cost the same as a settled one, which is what says the - // hole is the ordinary event-order inversion rather than - // an unfinished write. - // - // It is worst during `close`, where it is futile by - // construction: closing refuses every push from then on, - // so the event this hole waits for can never be queued, - // and the loop can only finish through the escalation - // that `COLLECT_WHOLE_QUEUE_AFTER_ATTEMPTS` already - // guarantees. Every sleep on that path is time bought - // for nothing. + // Waiting on the escalating retries instead charged for + // every step towards the fallback that fixes them, + // which is how a 0.03s drain became 290s. // - // The proper fix is to wait on progress rather than on a - // clock: `progress_notify` is notified both when the - // queue drains and when the lifecycle moves, including - // by `close`, so a notified wait with this as a backstop - // would wake on the event instead of guessing at it. - // `wait_for_ops` already does exactly that. That is a - // larger change to this loop than a release wants to - // carry, so this reduces the constant now and leaves the - // mechanism for its own change. - nagoya::sleep(RETRY_BACKOFF).await; + // This waits on the push that would carry the missing + // event, not on a clock. See + // `Queue::await_more_operations` for why a non-empty + // queue and a non-`Running` lifecycle each end it at + // once, and for what the cap is still there to cover. + engine_queue.await_more_operations(event_gap_wait_cap).await; } } else if let Some(page_ids) = pending_reclaim.take() { // `get_first_op_id_available() == None` is only sufficient From 4a1422b1cf4a059fabf6fed6a2bc83e5a7192f86 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 23 Sep 2026 05:43:38 +0700 Subject: [PATCH 3/3] Release this as 1.10.0-beta2 All three published crates move together: worktable, worktable_codegen and worktable_dsl are one release, and the path dependencies between them carry the version, so bumping one alone would publish a crate asking for a sibling that does not exist at that version. The fleet takes `worktable = "^1.10.0-beta1"`, and a caret over a prerelease does not admit another prerelease: ^1.10.0-beta1 matches 1.10.0-beta1 and nothing else in the beta series. So every consumer needs its requirement moved to beta2 explicitly, which is the pass that follows this. --- Cargo.toml | 6 +++--- codegen/Cargo.toml | 4 ++-- dsl/Cargo.toml | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4d8c2aae..94ab8bc7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ members = ["codegen", "dsl", "examples", "performance_measurement", "performance [package] name = "worktable" -version = "1.10.0-beta1" +version = "1.10.0-beta2" edition = "2024" authors = ["Handy-caT"] license = "MIT" @@ -147,12 +147,12 @@ walkdir = { version = "2", optional = true } # These pre-release workspace crates move as one train. The explicit caret # keeps the dependency policy consistent while the local path selects this # checkout during validation. -worktable_codegen = { path = "codegen", version = "^1.10.0-beta1" } +worktable_codegen = { path = "codegen", version = "^1.10.0-beta2" } # Re-exported below. Each generated table carries its declaration as a const # whose documentation says to read it with `worktable_dsl::Schema::parse`; that # instruction is only true if a plain `worktable` dependency can reach the # crate. -worktable_dsl = { path = "dsl", version = "^1.10.0-beta1", optional = true } +worktable_dsl = { path = "dsl", version = "^1.10.0-beta2", optional = true } [target.'cfg(unix)'.dependencies] libc = { version = "^0.2", default-features = false } diff --git a/codegen/Cargo.toml b/codegen/Cargo.toml index c70533c1..b8b73ab7 100644 --- a/codegen/Cargo.toml +++ b/codegen/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "worktable_codegen" -version = "1.10.0-beta1" +version = "1.10.0-beta2" edition = "2024" license = "MIT" description = "Proc-macro companion crate for worktable: the worktable! macro and its derives." @@ -26,7 +26,7 @@ proc-macro = true # a declaration. See its crate docs for why that needed a separate crate. # Name the reviewed prerelease while accepting compatible schema-model and # validator updates. Release checks verify the resolved generated API. -worktable_dsl = { path = "../dsl", version = "^1.10.0-beta1" } +worktable_dsl = { path = "../dsl", version = "^1.10.0-beta2" } # Test-only. As a normal dependency this proc-macro crate put `rkyv` with its # default features into the graph, which turned on `rkyv/std` for the target # build too and dragged `ptr_meta` with it. The generated code names `rkyv` diff --git a/dsl/Cargo.toml b/dsl/Cargo.toml index 7aecf63b..861aabe8 100644 --- a/dsl/Cargo.toml +++ b/dsl/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "worktable_dsl" -version = "1.10.0-beta1" +version = "1.10.0-beta2" edition = "2024" license = "MIT" description = "The worktable! schema language: its model and parser, readable outside the proc macro"