Refactor: flatten executing_reg_task_ids; add orch_to_sched control#325
Refactor: flatten executing_reg_task_ids; add orch_to_sched control#325poursoul wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the runtime's thread management by introducing a new environment variable, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces the PTO2_ORCH_TO_SCHED environment variable to control the transition of orchestrator threads to scheduler threads. The implementation correctly adds the feature flag and applies it to the relevant logic paths. The pull request also includes a significant and beneficial refactoring of executing_reg_task_ids_ to be a global array, which simplifies the code. I've found one potential issue where a profiling call seems to have been accidentally removed.
4f2d3a6 to
5deaaf3
Compare
executing_reg_task_ids refactor: - Core IDs are globally unique, so the per-thread dimension on executing_reg_task_ids_ was redundant and complicated reassignment - Remove executing_reg_task_ids parameter threading through check_running_cores_for_completion and dispatch_subtask_to_core - Simplify reassign_cores_for_all_threads by using a bool running lookup instead of collecting separate running/idle lists - Reorder CoreTypeTracker fields: counts before arrays - Add profiling instrumentation for core reassignment cost - Remove perf_aicpu_record_orch_phase from PHASE_END macro orch_to_sched control (PTO2_ORCH_TO_SCHED env var): - When disabled (default), orchestrator threads exit after orchestration without converting to scheduler threads or reassigning cores - Enable with PTO2_ORCH_TO_SCHED=1 to restore the previous behavior where orchestrators become schedulers after orchestration completes
hw-native-sys-bot
left a comment
There was a problem hiding this comment.
Review: commented-out code should be removed
| // // 这里的初始化可能不需要,因为状态没发生变化 | ||
| // memset(trackers_[i].core_idle, 0, sizeof(trackers_[i].core_idle)); | ||
| // // 这里的初始化可能不需要,因为对于没有重分配到其他核上的core,本身状态不变,而其他核上初始已经修改过了 | ||
| // for (int32_t j = 0; j < MAX_CORES_PER_THREAD; j++) { | ||
| // executing_reg_task_ids_[i][j] = AICPU_TASK_INVALID; | ||
| // } |
There was a problem hiding this comment.
这段注释掉的旧代码应该直接删除而不是留着。如果确认"状态没发生变化"这个判断是正确的,那就不需要保留;如果还不确定,应该加个 TODO 或者先验证再合入。注释掉的代码会增加维护负担,后续开发者不清楚是"暂时禁用"还是"已废弃待删"。
另外 executing_reg_task_ids_ 已经从二维降为一维了,这里注释中的 executing_reg_task_ids_[i][j] 已经无法编译,如果取消注释会直接报错,进一步说明应该删除。
executing_reg_task_ids refactor:
executing_reg_task_ids_ was redundant and complicated reassignment
check_running_cores_for_completion and dispatch_subtask_to_core
lookup instead of collecting separate running/idle lists
orch_to_sched control (PTO2_ORCH_TO_SCHED env var):
without converting to scheduler threads or reassigning cores
where orchestrators become schedulers after orchestration completes