Timer optimize - #1
Conversation
…tions, and fixed type signatures.
…legate execution.
…python 3.14 deprecation warnings, and pass code quality checks
…ing()/uncancel() methods
- subprocess.py: Add returncode property and _wait() async method for asyncio compatibility - transport.py: Add _recv_pending flag to prevent duplicate recv submissions - loop.py: Fix accept/connect flows, increase default buffer count to 512 - benchmark_suite.py: Add Plotly HTML chart generation - BENCHMARK.md: Add comprehensive benchmark analysis - test_fastapi.py, test_starlette.py: Refactor to use httpx.AsyncClient + ASGITransport - conftest.py: Add global pytest configuration with buffer limits - task.rs: Add _loop getter for anyio compatibility - pyproject.toml: Add httpx, plotly, kaleido to dev dependencies - buf_ring.rs, fixed_fd.rs: New modules for provided buffer ring and registered FDs - Various verify tests for multishot accept, pbuf_ring, registered FDs Test Results: 85 passed, 11 failed (known limitations), 3 skipped
- scheduler.rs: Replace Mutex<VecDeque> with crossbeam-channel for lock-free push/drain - lib.rs: Merge Ring lock acquisitions (submit + drain_completions in single lock) - loop.py: Skip epoll.poll when ready tasks exist (fast path for gather/sleep) Results: - gather(100): 454µs → 387µs (~15% improvement) - sleep_conc_100: 577µs → 469µs (~19% improvement) Note: Remaining ~2-3x gap vs uvloop is inherent PyO3/Python call overhead, not Rust-side locking.
- ARCHITECTURE.md: Update Native Task Scheduling section for Phase 3+10 - Document lock-free MPSC scheduler (crossbeam-channel) - Add merged Ring lock and epoll skip optimizations - Update SOTA table with new optimizations - Revise Future Work section - README.md: Update project status and benchmarks - Change phase to Phase 10 (Lock-Free Scheduler) - Update key features with lock-free scheduler - Refresh benchmark numbers (sleep 2.8x, semaphore 3.2x, wait_for 2.3x)
ARCHITECTURE.md: - Added 'Performance Bottleneck Analysis (Phase 11)' section - Documented PyO3 call overhead as root cause (~500-1000ns × 8-12 calls per step) - Listed attempted optimizations that didn't improve performance - Noted architecture implication: need 90%+ Rust migration to match uvloop README.md: - Updated project status to Phase 11 - Split benchmarks into 'Single-Task Latency' (wins) and 'High-Concurrency' (loses) - Added honest performance comparison with explanation
- Removed _enter_task/_leave_task from run_step hot path (major win) - Implemented direct scheduler push (avoiding call_soon overhead) - Inlined wakeup caching in run_step logic - Moved run_step to internal impl block to handle Rust types - Result: sleep_conc_100 29% faster (419µs), gather(100) 8% faster (380µs)
- Fixed TimerHandle cancellation crash in Rust scheduler (src/lib.rs) - Updated README.md: Phase 15 status, latest benchmark results (2.9x sleep(0)) - Updated ARCHITECTURE.md: Added Phase 14 stress testing documentation - cargo fmt && clippy: All checks pass - Core tests: 27/27 PASS - Benchmarks: uringcore 4.26µs sleep(0) vs uvloop 12.53µs
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive timer optimization for the uringcore event loop, introducing native Rust-based timer scheduling and modernizing the asyncio integration patterns. The changes include migrating from Python-based timer heaps to lock-free Rust schedulers, implementing native futures and tasks, and updating tests to use modern factory patterns instead of deprecated event loop policies.
Key Changes:
- Native timer implementation using Rust BinaryHeap with lock-free scheduling
- Replacement of Python event loop policy pattern with modern
new_event_loop()factory - Introduction of native
UringTask,UringFuture, andUringHandletypes for performance - Buffer configuration via environment variables (URINGCORE_BUFFER_COUNT, URINGCORE_BUFFER_SIZE)
Reviewed changes
Copilot reviewed 51 out of 52 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/timer.rs | New native timer heap implementation using BinaryHeap for efficient timer scheduling |
| src/task.rs | Native UringTask implementation with optimized coroutine stepping |
| src/scheduler.rs | Lock-free task scheduler using crossbeam-channel MPSC |
| src/ring.rs | Extended io_uring operations with timer support and multishot operations |
| src/lib.rs | Integration of new modules and updated buffer initialization |
| python/uringcore/loop.py | Refactored event loop to use Rust-native scheduling and timer management |
| python/uringcore/init.py | Added new_event_loop factory function for Python 3.11+ compatibility |
| tests/* | Updated all tests to use modern event loop factory pattern |
| benchmarks/benchmark_suite.py | Expanded benchmark suite with additional test cases |
Comments suppressed due to low confidence (2)
python/uringcore/subprocess.py:1
- The broad exception handler on line 241 catches all exceptions but treats them all as if
wait_forfailed. This could mask unexpected errors. Consider being more specific about which exceptions warrant the manual timeout fallback.
"""Subprocess transport and protocol support for uringcore."""
python/uringcore/loop.py:1
- This code snippet appears to have a syntax error -
set_descriptionis being called with incorrect syntax. The string "timeout" should not be passed as a positional argument in this manner. This line will fail at runtime.
"""UringEventLoop: Pure io_uring asyncio event loop.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self._paused: | ||
| self._paused = False | ||
| self._loop._core.resume_reading(self._fd) | ||
| # Always rearm receive when resuming or starting |
There was a problem hiding this comment.
The comment on line 82 states "Always rearm receive when resuming or starting" but _rearm_recv() is only called when self._paused was previously True (line 79-80). Consider clarifying the comment or adjusting the logic to match the description.
| # Always rearm receive when resuming or starting | |
| # Ensure receive is (re)armed when reading is enabled |
| self._loop.remove_reader(self._pidfd) | ||
| try: | ||
| os.close(self._pidfd) | ||
| except OSError: | ||
| pass | ||
| self._pidfd = None | ||
|
|
||
| # Process has exited, wait() should return immediately | ||
| try: | ||
| # WNOHANG shouldn't be needed if pidfd signaled, but safer | ||
| # Actually for standard Popen, just wait() is fine as it reaps. | ||
| returncode = self._proc.wait() | ||
| self._process_exited(returncode) | ||
| except Exception: | ||
| # Should not happen | ||
| pass |
There was a problem hiding this comment.
The indentation appears inconsistent here with an extra space before self._loop.remove_reader. This should align with the surrounding code at the same indentation level.
| self._loop.remove_reader(self._pidfd) | |
| try: | |
| os.close(self._pidfd) | |
| except OSError: | |
| pass | |
| self._pidfd = None | |
| # Process has exited, wait() should return immediately | |
| try: | |
| # WNOHANG shouldn't be needed if pidfd signaled, but safer | |
| # Actually for standard Popen, just wait() is fine as it reaps. | |
| returncode = self._proc.wait() | |
| self._process_exited(returncode) | |
| except Exception: | |
| # Should not happen | |
| pass | |
| self._loop.remove_reader(self._pidfd) | |
| try: | |
| os.close(self._pidfd) | |
| except OSError: | |
| pass | |
| self._pidfd = None | |
| # Process has exited, wait() should return immediately | |
| try: | |
| # WNOHANG shouldn't be needed if pidfd signaled, but safer | |
| # Actually for standard Popen, just wait() is fine as it reaps. | |
| returncode = self._proc.wait() | |
| self._process_exited(returncode) | |
| except Exception: | |
| # Should not happen | |
| pass |
| """Test simple subprocess execution.""" | ||
| proc = await asyncio.create_subprocess_exec( | ||
| sys.executable, "-c", "print('hello')", | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| ) | ||
|
|
||
| stdout, stderr = await proc.communicate() | ||
|
|
||
| assert proc.returncode == 0 | ||
| assert b"hello" in stdout | ||
| async def check(): |
There was a problem hiding this comment.
The parameter name loop is inconsistent with the fixture name event_loop. While this works via the loop fixture (line 141-142), using event_loop directly would be clearer and avoid the additional indirection.
| from uringcore import new_event_loop | ||
| return new_event_loop(buffer_count=buffer_count, buffer_size=buffer_size) |
There was a problem hiding this comment.
The import statement is inside the function definition. While this works, consider moving the import to the top of the function scope (line 361) alongside the other imports for better readability and consistency.
No description provided.