ITADN

[XLA:CPU] Possible perf regression in `Worker::Parallelize`

#42518Openzdryan 创建于 2026-05-13
Z
zdryancommented
Apologies if I'm misreading the code but I'd appreciate someone with context confirming or telling me I've missed something. ## Observation At [`work_queue.h:240`](https://github.com/openxla/xla/blob/main/xla/backends/cpu/runtime/work_queue.h#L240): ```cpp work_queue(num_work_items, /*num_partitions=*/this->count_down.count()) ``` In the original [9d95a71119](https://github.com/openxla/xla/commit/9d95a71119), `count_down` counted **workers**, so this line correctly meant `num_partitions = num_workers`. Then [5db7c22b15](https://github.com/openxla/xla/commit/5db7c22b15) ("Make Worker::Parallelize deadlock-proof") repurposed `count_down` to count **items**: ```diff - tsl::CountDownAsyncValueRef<tsl::Chain> count_down(num_workers); + tsl::CountDownAsyncValueRef<tsl::Chain> count_down(num_tasks); ``` which makes sense as a correctness fix, but unless I'm missing something the partition-count line wasn't updated. `count_down.count()` now returns `num_tasks`, so we end up with one `ABSL_CACHELINE_ALIGNED` partition per item: 64B each, `FixedArray<Partition, 32>` spills to heap for any job > 32 items, and every `Pop` walks to a fresh cache line via work-stealing wrap-around. Could be deliberate for stealing granularity, but the line reads like it intended the old semantics. ## Impact Added a small `BM_WorkerParallelize` micro (noop body, AMD EPYC 9634 SMT, 3 trials × 20 reps × min-time 0.3 s): | `num_work_items` | `partitions = items` (current) | `partitions = workers` | diff | |---:|---:|---:|---:| | 128 | 45,412 ns | 16,251 ns | −64% | | 256 | 65,045 ns | 22,824 ns | −65% | | 512 | 74,796 ns | 35,227 ns | −53% | | 1024 | 82,599 ns | 52,293 ns | −37% | ## Proposed Fix Substitute `num_workers` within `ParallelizeContext`: ```diff count_down(std::move(count_down)), - work_queue(num_work_items, /*num_partitions=*/this->count_down.count()), + work_queue(num_work_items, /*num_partitions=*/num_workers), parallel_work(std::forward<ParallelWork>(parallel_work)) {} ``` `WorkQueue` already supports `num_partitions != num_work_items` - [`work_queue_test.cc:43-81`](https://github.com/openxla/xla/blob/main/xla/backends/cpu/runtime/work_queue_test.cc#L43) covers `(14 items, 4 partitions)` etc., so I don't believe this breaks anything. Happy to send a PR with the fix and the micro if this is wanted. Filing the issue first in case I've misread the design intent. Perhaps @ezhulenev best positioned / most familiar with change?
2 条评论