diff --git a/crates/scheduler/full_integration_plan.md b/crates/scheduler/full_integration_plan.md deleted file mode 100644 index 24a71dc94093bd8684884f5cd810be37baeece70..0000000000000000000000000000000000000000 --- a/crates/scheduler/full_integration_plan.md +++ /dev/null @@ -1,229 +0,0 @@ -# GPUI Scheduler Integration - -This document describes the integration of GPUI's async execution with the scheduler crate, including architecture, design decisions, and lessons learned. - -## Goal - -Unify GPUI's async execution with the scheduler crate, eliminating duplicate blocking/scheduling logic and enabling deterministic testing. - ---- - -## Architecture - -``` -┌──────────────────────────────────────────────────────────────────┐ -│ GPUI │ -│ │ -│ ┌────────────────────────┐ ┌──────────────────────────────┐ │ -│ │ gpui::Background- │ │ gpui::ForegroundExecutor │ │ -│ │ Executor │ │ - inner: scheduler:: │ │ -│ │ - scheduler: Arc< │ │ ForegroundExecutor │ │ -│ │ dyn Scheduler> │ │ - dispatcher: Arc │ │ -│ │ - dispatcher: Arc │ └──────────────┬───────────────┘ │ -│ └───────────┬────────────┘ │ │ -│ │ │ │ -│ │ (creates temporary │ (wraps) │ -│ │ scheduler::Background- │ │ -│ │ Executor when spawning) │ │ -│ │ │ │ -│ │ ┌───────────────────────────┘ │ -│ │ │ │ -│ ▼ ▼ │ -│ ┌─────────────────────────────────────────────────────────────┐ │ -│ │ Arc │ │ -│ └──────────────────────────┬──────────────────────────────────┘ │ -│ │ │ -│ ┌────────────────┴────────────────┐ │ -│ │ │ │ -│ ▼ ▼ │ -│ ┌───────────────────────┐ ┌───────────────────────────┐ │ -│ │ PlatformScheduler │ │ TestScheduler │ │ -│ │ (production) │ │ (deterministic tests) │ │ -│ └───────────────────────┘ └───────────────────────────┘ │ -└──────────────────────────────────────────────────────────────────┘ -``` - -### Scheduler Trait - -The scheduler crate provides: -- `Scheduler` trait with `block()`, `schedule_foreground()`, `schedule_background_with_priority()`, `timer()`, `clock()` -- `TestScheduler` implementation for deterministic testing -- `ForegroundExecutor` and `BackgroundExecutor` that wrap `Arc` -- `Task` type with `ready()`, `is_ready()`, `detach()`, `from_async_task()` - -### PlatformScheduler - -`PlatformScheduler` in GPUI (`crates/gpui/src/platform_scheduler.rs`): -- Implements `Scheduler` trait for production use -- Wraps `PlatformDispatcher` (Mac, Linux, Windows) -- Uses `parking::Parker` for blocking operations -- Uses `dispatch_after` for timers -- Provides a `PlatformClock` that delegates to the dispatcher - -### GPUI Executors - -GPUI's executors (`crates/gpui/src/executor.rs`): -- `gpui::ForegroundExecutor` wraps `scheduler::ForegroundExecutor` internally -- `gpui::BackgroundExecutor` holds `Arc` directly -- Select `TestScheduler` or `PlatformScheduler` based on dispatcher type -- Wrap `scheduler::Task` in a thin `gpui::Task` that adds `detach_and_log_err()` -- Use `Scheduler::block()` for all blocking operations - ---- - -## Design Decisions - -### Key Design Principles - -1. **No optional fields**: Both test and production paths use the same executor types with different `Scheduler` implementations underneath. - -2. **Scheduler owns blocking logic**: The `Scheduler::block()` method handles all blocking, including timeout and task stepping (for tests). - -3. **GPUI Task wrapper**: Thin wrapper around `scheduler::Task` that adds `detach_and_log_err()` which requires `&App`. - -### Foreground Priority Not Supported - -`ForegroundExecutor::spawn_with_priority` accepts a priority parameter but ignores it. This is acceptable because: -- macOS (primary platform) ignores foreground priority anyway -- TestScheduler runs foreground tasks in order -- There are no external callers of this method in the codebase - -### Session IDs for Foreground Isolation - -Each `ForegroundExecutor` gets a `SessionId` to prevent reentrancy when blocking. This ensures that when blocking on a future, we don't run foreground tasks from the same session. - -### Runtime Scheduler Selection - -In test builds, we check `dispatcher.as_test()` to choose between `TestScheduler` and `PlatformScheduler`. This allows the same executor types to work in both test and production environments. - -### Profiler Integration - -The profiler task timing infrastructure continues to work because: -- `PlatformScheduler::schedule_background_with_priority` calls `dispatcher.dispatch()` -- `PlatformScheduler::schedule_foreground` calls `dispatcher.dispatch_on_main_thread()` -- All platform dispatchers wrap task execution with profiler timing - ---- - -## Intentional Removals - -### `spawn_labeled` and `deprioritize` - -**What was removed**: -- `BackgroundExecutor::spawn_labeled(label: TaskLabel, future)` -- `BackgroundExecutor::deprioritize(label: TaskLabel)` -- `TaskLabel` type - -**Why**: These were only used in a few places for test ordering control. The new priority-weighted scheduling in `TestScheduler` provides similar functionality through `Priority::High/Medium/Low`. - -**Migration**: Use `spawn()` instead of `spawn_labeled()`. For test ordering, use explicit synchronization (channels, etc.) or priority levels. - -### `start_waiting` / `finish_waiting` Debug Methods - -**What was removed**: -- `BackgroundExecutor::start_waiting()` -- `BackgroundExecutor::finish_waiting()` -- Associated `waiting_backtrace` tracking in TestDispatcher - -**Why**: The new `TracingWaker` in `TestScheduler` provides better debugging capability. Run tests with `PENDING_TRACES=1` to see backtraces of all pending futures when parking is forbidden. - -### Realtime Priority - -**What was removed**: `Priority::Realtime` variant and associated OS thread spawning. - -**Why**: There were no in-tree call sites using realtime priority. The correctness/backpressure semantics are non-trivial: -- Blocking enqueue risks stalling latency-sensitive threads -- Non-blocking enqueue implies dropping runnables under load, which breaks correctness for general futures - -Rather than ship ambiguous or risky semantics, we removed the API until there is a concrete in-tree use case. - ---- - -## Lessons Learned - -These lessons were discovered during integration testing and represent important design constraints. - -### 1. Never Cache `Entity` in Process-Wide Statics - -**Problem**: `gpui::Entity` is a handle tied to a particular `App`'s entity-map. Storing an `Entity` in a process-wide static (`OnceLock`, `LazyLock`, etc.) and reusing it across different `App` instances causes: -- "used a entity with the wrong context" panics -- `Option::unwrap()` failures in leak-detection clone paths -- Nondeterministic behavior depending on test ordering - -**Solution**: Cache plain data (env var name, URL, etc.) in statics, and create `Entity` per-`App`. - -**Guideline**: Never store `gpui::Entity` or other `App`-context-bound handles in process-wide statics unless explicitly keyed by `App` identity. - -### 2. `block_with_timeout` Behavior Depends on Tick Budget - -**Problem**: In `TestScheduler`, "timeout" behavior depends on an internal tick budget (`timeout_ticks`), not just elapsed wall-clock time. During the allotted ticks, the scheduler can poll futures and step other tasks. - -**Implications**: -- A future can complete "within a timeout" in tests due to scheduler progress, even without explicit `advance_clock()` -- Yielding does not advance time -- If a test needs time to advance, it must do so explicitly via `advance_clock()` - -**For deterministic timeout tests**: Set `scheduler.set_timeout_ticks(0..=0)` to prevent any scheduler stepping during timeout, then explicitly advance time. - -### 3. Realtime Priority Must Panic in Tests - -**Problem**: `Priority::Realtime` spawns dedicated OS threads outside the test scheduler, which breaks determinism and causes hangs/flakes. - -**Solution**: The test dispatcher's `spawn_realtime` implementation panics with a clear message. This is an enforced invariant, not an implementation detail. - ---- - -## Test Helpers - -Test-only methods on `BackgroundExecutor`: -- `block_test()` - for running async tests synchronously -- `advance_clock()` - move simulated time forward -- `tick()` - run one task -- `run_until_parked()` - run all ready tasks -- `allow_parking()` / `forbid_parking()` - control parking behavior -- `simulate_random_delay()` - yield randomly for fuzzing -- `rng()` - access seeded RNG -- `set_block_on_ticks()` - configure timeout tick range for block operations - ---- - -## Code Quality Notes - -### `dispatch_after` Panics in TestDispatcher - -This is intentional: -```rust -fn dispatch_after(&self, _duration: Duration, _runnable: RunnableVariant) { - panic!( - "dispatch_after should not be called in tests. \ - Use BackgroundExecutor::timer() which uses the scheduler's native timer." - ); -} -``` - -In tests, `TestScheduler::timer()` creates native timers without using `dispatch_after`. Any code hitting this panic has a bug. - ---- - -## Files Changed - -Key files modified during integration: - -- `crates/scheduler/src/scheduler.rs` - `Scheduler::block()` signature takes `Pin<&mut dyn Future>` and returns `bool` -- `crates/scheduler/src/executor.rs` - Added `from_async_task()` -- `crates/scheduler/src/test_scheduler.rs` - Deterministic scheduling implementation -- `crates/gpui/src/executor.rs` - Rewritten to use scheduler executors -- `crates/gpui/src/platform_scheduler.rs` - New file implementing `Scheduler` for production -- `crates/gpui/src/platform/test/dispatcher.rs` - Simplified to delegate to TestScheduler -- `crates/gpui/src/platform.rs` - Simplified `RunnableVariant`, removed `TaskLabel` -- Platform dispatchers (mac/linux/windows) - Removed label parameter from dispatch - ---- - -## Future Considerations - -1. **Foreground priority support**: If needed, add `schedule_foreground_with_priority` to the `Scheduler` trait. - -2. **Profiler integration in scheduler**: Could move task timing into the scheduler crate for more consistent profiling. - -3. **Additional test utilities**: The `TestScheduler` could be extended with more debugging/introspection capabilities. \ No newline at end of file