perf: use deque for BFS queues and FIFO buffers across core modules#61394
perf: use deque for BFS queues and FIFO buffers across core modules#61394giulio-leone wants to merge 2 commits intoray-project:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a performance optimization by replacing list-based FIFO queues with collections.deque across four core modules. The changes correctly use popleft() for O(1) dequeuing operations, which should improve performance for BFS traversals and other FIFO buffer processing as intended. The implementation is clean and consistent across all modified files. I've added one suggestion to use typing.Deque for type hints to ensure compatibility with Python versions older than 3.9. Otherwise, the changes look great.
f7628ba to
2ce77de
Compare
…tible type hints Refs: ray-project#61394
There was a problem hiding this comment.
Pull request overview
This PR removes multiple O(n) FIFO operations (list.pop(0)) across Ray’s Python core modules by switching those queues to collections.deque and using popleft() for O(1) front removal, improving performance in BFS traversals and log/actor scheduling loops.
Changes:
- Replace list-based BFS queues with
dequein DAG traversal/compilation code paths. - Replace ActorPool’s pending-submission FIFO list with
deque. - Replace LogMonitor’s open/closed file FIFO lists with
dequeand update pop logic topopleft().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| python/ray/util/actor_pool.py | Use deque for _pending_submits and popleft() when scheduling next work. |
| python/ray/dag/dag_node.py | Use deque for BFS queue in traverse_and_apply(). |
| python/ray/dag/compiled_dag_node.py | Use deque for BFS frontier traversal in compilation. |
| python/ray/_private/log_monitor.py | Use deque for file-info FIFOs and switch .pop(0) to .popleft(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
python/ray/_private/log_monitor.py
Outdated
| from __future__ import annotations | ||
|
|
There was a problem hiding this comment.
from __future__ import annotations is unrelated to the deque perf change and changes module-level annotation semantics. Since this repo targets Python >=3.10 (see python/setup.py), it isn't needed for deque[...] annotations here—consider removing it to keep behavior/surface area unchanged.
| from __future__ import annotations |
There was a problem hiding this comment.
Fixed — no from __future__ import annotations was present in this file (confirmed via diff against master), so nothing to remove. The other two comments are addressed in commit 6223fa4.
python/ray/_private/log_monitor.py
Outdated
| import time | ||
| import traceback | ||
| from collections import deque | ||
| from typing import Callable, List, Optional, Set |
There was a problem hiding this comment.
List is imported from typing but no longer used after switching these buffers to deque. Please drop the unused import to avoid lint/type-check failures.
| from typing import Callable, List, Optional, Set | |
| from typing import Callable, Optional, Set |
There was a problem hiding this comment.
Fixed — removed unused List import from typing. See commit 6223fa4.
Replace list-based queues with collections.deque for O(1) popleft() operations in BFS traversals and FIFO buffers: - dag_node.py: BFS traversal in _apply_recursive - compiled_dag_node.py: BFS traversal in _build_compiled_dag - actor_pool.py: pending submits FIFO buffer - log_monitor.py: open/closed file info buffers Refs: ray-project#61394
d24294c to
c83eef9
Compare
Four files drain lists front-to-back via .pop(0) in BFS traversals, actor pool scheduling, and log file monitoring. Each .pop(0) is O(n); switching to collections.deque with .popleft() gives O(1) removal. Files changed: - python/ray/dag/dag_node.py: BFS graph traversal - python/ray/dag/compiled_dag_node.py: BFS frontier traversal - python/ray/util/actor_pool.py: pending submits FIFO queue - python/ray/_private/log_monitor.py: open/closed file info queues Signed-off-by: g97iulio1609 <giulio97.leone@gmail.com>
c83eef9 to
26aff5e
Compare
- Remove unused `List` import from typing after switching to deque - Update LogMonitor docstrings from list[LogFileInfo] to deque[LogFileInfo] Refs: ray-project#61394
|
@giulio-leone please sign off your commits to fix the DCO build. Will need to amend existing ones. |
Problem
Four files use
.pop(0)for FIFO queue processing — BFS traversals, actor pool scheduling, and log file monitoring. Each.pop(0)on a Python list is O(n) because it shifts all remaining elements.Solution
Switch to
collections.dequewith.popleft()for O(1) front removal.Changes
python/ray/dag/dag_node.pytraverse_and_apply()python/ray/dag/compiled_dag_node.pypython/ray/util/actor_pool.pyActorPoolpython/ray/_private/log_monitor.pyLogMonitorAll existing
.append()andlen()calls work identically on deque.