From c39581d52ea5739397302abb59836cdcf037cb0a Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Sun, 14 Dec 2025 05:40:03 -0700 Subject: [PATCH] Remove unused execution tracking from TestScheduler and TestDispatcher The execution_hash/execution_count tracking was added speculatively but has no actual consumers. Removing to reduce complexity. --- crates/gpui/src/platform/test/dispatcher.rs | 53 +------- crates/scheduler/full_integration_plan.md | 143 ++------------------ crates/scheduler/plan.md | 11 +- crates/scheduler/src/test_scheduler.rs | 42 ------ 4 files changed, 19 insertions(+), 230 deletions(-) diff --git a/crates/gpui/src/platform/test/dispatcher.rs b/crates/gpui/src/platform/test/dispatcher.rs index f271b9f6af..6666ea9adc 100644 --- a/crates/gpui/src/platform/test/dispatcher.rs +++ b/crates/gpui/src/platform/test/dispatcher.rs @@ -7,7 +7,7 @@ use rand::prelude::*; use scheduler::{Clock, TestScheduler, TestSchedulerConfig}; use std::{ future::Future, - hash::{Hash, Hasher}, + hash::Hash, ops::RangeInclusive, pin::Pin, sync::{ @@ -90,13 +90,6 @@ struct TestDispatcherState { deprioritized_task_labels: HashSet, block_on_ticks: RangeInclusive, unparkers: Vec, - /// Tracks execution order for determinism verification. - /// This hash is updated each time a task is executed, incorporating - /// the task's source location. If two runs with the same seed produce - /// different hashes, there is non-determinism in the test. - execution_hash: u64, - /// Count of tasks executed, for debugging. - execution_count: u64, } impl TestDispatcher { @@ -122,8 +115,6 @@ impl TestDispatcher { deprioritized_task_labels: Default::default(), block_on_ticks: 0..=1000, unparkers: Default::default(), - execution_hash: 0, - execution_count: 0, }; TestDispatcher { @@ -297,7 +288,6 @@ impl TestDispatcher { drop(state); // Log task location before running (helps identify which task is executing) - // Also update execution hash for determinism tracking let location_str = match &runnable { RunnableVariant::Meta(r) => { let loc = r.metadata().location; @@ -306,24 +296,11 @@ impl TestDispatcher { RunnableVariant::Compat(_) => "compat-task".to_string(), }; - // Update execution hash with task location for determinism verification - { - let mut state = self.state.lock(); - let mut hasher = std::collections::hash_map::DefaultHasher::new(); - state.execution_hash.hash(&mut hasher); - location_str.hash(&mut hasher); - state.execution_count.hash(&mut hasher); - state.execution_hash = hasher.finish(); - state.execution_count += 1; - } - dispatcher_log!( - "tick() RUNNING {} task from {} | main_thread={} | exec_count={} exec_hash={:016x}", + "tick() RUNNING {} task from {} | main_thread={}", task_source, location_str, main_thread, - self.state.lock().execution_count, - self.state.lock().execution_hash ); match runnable { @@ -427,32 +404,6 @@ impl TestDispatcher { /// seed produce different execution hashes at the same point, there is /// non-determinism in the execution (likely from real OS threads, smol::spawn, /// or other sources outside TestDispatcher's control). - /// - /// # Example - /// ```ignore - /// let hash_before = dispatcher.execution_hash(); - /// // ... do some work ... - /// let hash_after = dispatcher.execution_hash(); - /// eprintln!("Execution hash: {} -> {} (count: {})", - /// hash_before, hash_after, dispatcher.execution_count()); - /// ``` - pub fn execution_hash(&self) -> u64 { - self.state.lock().execution_hash - } - - /// Returns the number of tasks executed so far. - pub fn execution_count(&self) -> u64 { - self.state.lock().execution_count - } - - /// Resets the execution hash and count. Useful for isolating - /// determinism checks to specific sections of a test. - pub fn reset_execution_tracking(&self) { - let mut state = self.state.lock(); - state.execution_hash = 0; - state.execution_count = 0; - } - /// Returns a reference to the underlying TestScheduler. /// This can be used for advanced testing scenarios that need direct scheduler access. pub fn scheduler(&self) -> &Arc { diff --git a/crates/scheduler/full_integration_plan.md b/crates/scheduler/full_integration_plan.md index fd1e5cf8f0..0e6daec92d 100644 --- a/crates/scheduler/full_integration_plan.md +++ b/crates/scheduler/full_integration_plan.md @@ -9,12 +9,7 @@ - Updated all platform dispatchers (Mac, Linux, Windows, Test) to remove `Scheduler` variant handling - Updated `PlatformScheduler` to use `RunnableVariant::Meta` instead of `RunnableVariant::Scheduler` -### 2. Added Execution Tracking to TestScheduler (Phase 3c) - DONE -- Added `execution_hash: u64` and `execution_count: u64` to `SchedulerState` -- Added `execution_hash()`, `execution_count()`, `reset_execution_tracking()` methods -- Execution tracking updates in `step()` after task selection, hashing source location - -### 3. Waiting Hints (Phase 3b) - SKIPPED +### 2. Waiting Hints (Phase 3b) - SKIPPED - NOT NEEDED: TestScheduler already has superior `pending_traces` system with `TracingWaker` - Automatically captures backtraces for ALL pending futures via `PENDING_TRACES=1` env var - More comprehensive than manual waiting hints @@ -26,11 +21,9 @@ - Add `is_ready()` method to scheduler's `Task` if needed - Keep `detach_and_log_err` as extension trait in GPUI (needs `App` context) -2. **Update TestDispatcher to delegate execution tracking** - Wire TestDispatcher's `execution_hash()` and `execution_count()` to call TestScheduler's new methods (currently TestDispatcher has its own tracking) - ### Medium-term: -3. **Eliminate RunnableVariant entirely (Phase 2a full)** - Remove `Compat` variant by converting timer callbacks to use `RunnableMeta` -4. **Delegate task queues to TestScheduler (Phase 2b)** - Most complex change +2. **Eliminate RunnableVariant entirely (Phase 2a full)** - Remove `Compat` variant by converting timer callbacks to use `RunnableMeta` +3. **Delegate task queues to TestScheduler (Phase 2b)** - Most complex change --- @@ -219,22 +212,7 @@ Some GPUI features should move to the scheduler crate to reduce duplication. **Problem**: When tests hang, GPUI captures waiting hints and backtraces for debugging. -**Solution**: Add optional debug info to scheduler's parking mechanism. - -**Steps**: -1. Add `set_waiting_hint(&str)` to `TestScheduler` -2. Add backtrace capture on park timeout -3. Wire through from GPUI's `BackgroundExecutor::set_waiting_hint` - -#### 3c: Execution Tracking - -**Problem**: `TestDispatcher` tracks execution hash/count for determinism verification. - -**Solution**: Add to `TestScheduler`. - -**Steps**: -1. Add `execution_hash()` and `execution_count()` to `TestScheduler` -2. Update on each task execution with source location hash +**Solution**: NOT NEEDED - TestScheduler already has superior `pending_traces` system with `TracingWaker` that automatically captures backtraces for ALL pending futures via `PENDING_TRACES=1` env var. --- @@ -320,125 +298,22 @@ pub enum RunnableVariant { - Convert to `RunnableVariant::Meta` - Update all dispatcher match arms -#### 2. Add execution tracking to TestScheduler (Phase 3c) +#### 2. Eliminate RunnableVariant::Compat (Phase 2a full) -Add to `TestScheduler` for determinism verification: - -```rust -// scheduler/src/test_scheduler.rs - -// Add to SchedulerState: -struct SchedulerState { - // ... existing fields ... - execution_hash: u64, - execution_count: u64, -} - -impl TestScheduler { - pub fn execution_hash(&self) -> u64 { - self.state.lock().execution_hash - } - - pub fn execution_count(&self) -> u64 { - self.state.lock().execution_count - } - - pub fn reset_execution_tracking(&self) { - let mut state = self.state.lock(); - state.execution_hash = 0; - state.execution_count = 0; - } -} - -// In step(), after selecting runnable: -fn step(&self) -> bool { - // ... select runnable ... - - // Update execution tracking - let mut state = self.state.lock(); - let mut hasher = DefaultHasher::new(); - state.execution_hash.hash(&mut hasher); - runnable.metadata().location.file().hash(&mut hasher); - runnable.metadata().location.line().hash(&mut hasher); - state.execution_count.hash(&mut hasher); - state.execution_hash = hasher.finish(); - state.execution_count += 1; - drop(state); - - runnable.run(); - true -} -``` - -Then update `TestDispatcher` to delegate: -```rust -// gpui/src/platform/test/dispatcher.rs -impl TestDispatcher { - pub fn execution_hash(&self) -> u64 { - self.scheduler.execution_hash() - } - - pub fn execution_count(&self) -> u64 { - self.scheduler.execution_count() - } -} -``` - -#### 3. Add waiting hints to TestScheduler (Phase 3b) - -```rust -// scheduler/src/test_scheduler.rs - -// Add to SchedulerState: -struct SchedulerState { - // ... existing fields ... - waiting_hint: Option, - waiting_backtrace: Option, -} - -impl TestScheduler { - pub fn set_waiting_hint(&self, hint: impl Into) { - self.state.lock().waiting_hint = Some(hint.into()); - } - - pub fn clear_waiting_hint(&self) { - let mut state = self.state.lock(); - state.waiting_hint = None; - state.waiting_backtrace = None; - } - - pub fn waiting_hint(&self) -> Option { - self.state.lock().waiting_hint.clone() - } -} - -// In park(), include hint in panic message: -fn park(&self, deadline: Option) -> bool { - let state = self.state.lock(); - if !state.allow_parking { - let hint = state.waiting_hint.as_deref().unwrap_or("no hint"); - panic!("test parked unexpectedly. hint: {}", hint); - } - // ... rest of parking logic ... -} -``` +Convert timer callbacks to use `RunnableMeta` instead of plain `Runnable`, eliminating the need for the `Compat` variant entirely. ### Medium-term (Higher Effort) -4. **Eliminate RunnableVariant** (Phase 2a full) - - Requires coordinated changes across all dispatchers - - Significant simplification - -5. **Unify Task types** (Phase 1) +3. **Unify Task types** (Phase 1) - After RunnableMeta unified, this becomes cleaner ### Long-term (Optional) -6. **Delegate task queues** (Phase 2b) +4. **Delegate task queues** (Phase 2b) - Most complex change - Only if clear benefit emerges -7. **Executor unification** (Phase 4) +5. **Executor unification** (Phase 4) - Defer unless concrete need arises ## Technical Considerations diff --git a/crates/scheduler/plan.md b/crates/scheduler/plan.md index f9e56d6d63..652580626c 100644 --- a/crates/scheduler/plan.md +++ b/crates/scheduler/plan.md @@ -46,15 +46,20 @@ This document outlines the integration of the `scheduler` crate into GPUI, provi ### Phase 2: GPUI Adapters ✅ - Created `PlatformScheduler` adapter implementing `Scheduler` trait -- Added `RunnableVariant::Scheduler` for scheduler crate's runnables - Updated all platform dispatchers (Mac, Linux, Windows, Test) +### Phase 2a: Unified RunnableMeta ✅ + +- Re-exported `RunnableMeta` from scheduler crate (removed GPUI's duplicate definition) +- Removed `RunnableVariant::Scheduler` variant (now uses `RunnableVariant::Meta` for both) +- Simplified all dispatcher match arms + ### Phase 3: Hybrid TestDispatcher Integration ✅ `TestDispatcher` uses `TestScheduler` internally for timing/clock/rng while keeping its own task queues for `RunnableVariant` handling. **Why hybrid approach:** -- GPUI uses `RunnableVariant` with 3 variants (Meta, Compat, Scheduler) +- GPUI uses `RunnableVariant` with 2 variants (Meta, Compat) - `TestScheduler` expects `Runnable` only - Hybrid allows unified timing without breaking existing behavior @@ -124,7 +129,7 @@ Full executor composition (GPUI executors wrapping scheduler executors) is **def ### GPUI Crate - `src/platform/platform_scheduler.rs` - New `PlatformScheduler` adapter -- `src/platform.rs` - `RunnableVariant::Scheduler`, platform_scheduler module +- `src/platform.rs` - Re-exports `RunnableMeta` from scheduler, platform_scheduler module - `src/platform/test/dispatcher.rs` - `TestScheduler` integration, `new(seed: u64)` - `src/platform/mac/dispatcher.rs` - `trampoline_scheduler`, handle new variant - `src/platform/linux/dispatcher.rs` - Handle new variant diff --git a/crates/scheduler/src/test_scheduler.rs b/crates/scheduler/src/test_scheduler.rs index 607f89f050..4f2fb14a2f 100644 --- a/crates/scheduler/src/test_scheduler.rs +++ b/crates/scheduler/src/test_scheduler.rs @@ -10,11 +10,9 @@ use rand::prelude::*; use std::{ any::type_name_of_val, collections::{BTreeMap, HashSet, VecDeque}, - collections::hash_map::DefaultHasher, env, fmt::Write, future::Future, - hash::{Hash, Hasher}, mem, ops::RangeInclusive, panic::{self, AssertUnwindSafe}, @@ -93,8 +91,6 @@ impl TestScheduler { capture_pending_traces: config.capture_pending_traces, pending_traces: BTreeMap::new(), next_trace_id: TraceId(0), - execution_hash: 0, - execution_count: 0, })), clock: Arc::new(TestClock::new()), thread: thread::current(), @@ -125,25 +121,6 @@ impl TestScheduler { self.state.lock().allow_parking } - /// Returns the current execution hash for determinism verification. - /// If two runs with the same seed produce different hashes, there is - /// non-determinism in the test. - pub fn execution_hash(&self) -> u64 { - self.state.lock().execution_hash - } - - /// Returns the number of tasks that have been executed. - pub fn execution_count(&self) -> u64 { - self.state.lock().execution_count - } - - /// Resets execution tracking state (hash and count). - pub fn reset_execution_tracking(&self) { - let mut state = self.state.lock(); - state.execution_hash = 0; - state.execution_count = 0; - } - /// Create a foreground executor for this scheduler pub fn foreground(self: &Arc) -> ForegroundExecutor { let session_id = { @@ -255,19 +232,6 @@ impl TestScheduler { }; if let Some(runnable) = runnable { - // Update execution tracking with task location for determinism verification - { - let location = runnable.runnable.metadata().location; - let mut state = self.state.lock(); - let mut hasher = DefaultHasher::new(); - state.execution_hash.hash(&mut hasher); - location.file().hash(&mut hasher); - location.line().hash(&mut hasher); - state.execution_count.hash(&mut hasher); - state.execution_hash = hasher.finish(); - state.execution_count += 1; - } - runnable.run(); return true; } @@ -519,12 +483,6 @@ struct SchedulerState { capture_pending_traces: bool, next_trace_id: TraceId, pending_traces: BTreeMap, - /// Tracks execution order for determinism verification. - /// This hash is updated each time a task is executed, incorporating - /// the task's source location. - execution_hash: u64, - /// Count of tasks executed, for debugging. - execution_count: u64, } const WAKER_VTABLE: RawWakerVTable = RawWakerVTable::new(