Skip to content

Conversation

@eddyashton
Copy link
Member

This PR contains some follow-up cleanups from the discussion in #7269:

  • Returning const std::string& from the get_name calls to avoid any additional copies. The name is generally per-type, can opt in to per-instance where appropriate, in either case it should be formatted/constructed eagerly.
  • Removing the .empty() method. Replaced by a get_summary() returning some basic stats about the current queue state.
  • API simplification, removing unnecessary IJobBoard and clarifying which tests (most) are testing a local JobBoard vs the global static ccf::tasks.

Also a perf improvement - rather than gluing together discrete condition_variable and locked queue (taking 2 separate mutexes in sequence) we use a condition_variable wait for workers to pause directly on the work queue.

@eddyashton eddyashton requested a review from a team as a code owner September 26, 2025 12:38
Copilot AI review requested due to automatic review settings October 16, 2025 13:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Introduces a redesigned task execution system to route tasks directly to waiting worker threads, removes the IJobBoard interface, and centralizes delayed/periodic scheduling within JobBoard. Key changes:

  • Replaces IJobBoard with concrete JobBoard offering wait-based task dispatch, delayed/periodic scheduling, and summary reporting.
  • Adds ThreadManager for dynamic worker management and updates tests to use new APIs (set_worker_count, JobBoard::tick, get_summary).
  • Standardizes task/action naming to return const std::string&, eliminating transient string_view returns.

Reviewed Changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/tasks/worker.h Adds worker loop using timed waits on JobBoard.
src/tasks/thread_manager.{h,cpp} Introduces ThreadManager for managing pooled worker threads.
src/tasks/job_board.{h,cpp} Reimplements JobBoard with direct worker notification, delayed/periodic task handling, and summary API.
src/tasks/task_system.{h,cpp} Simplifies global API; delegates scheduling to JobBoard and worker mgmt to ThreadManager.
src/tasks/task.h Changes get_name() to pure virtual returning const std::string&.
src/tasks/basic_task.h Adjusts BasicTask naming and get_name signature.
src/tasks/ordered_tasks.{h,cpp} Updates constructor/API to use concrete JobBoard and rvalue name handling.
src/tasks/fan_in_tasks.{h,cpp} Removes custom naming; uses concrete JobBoard.
Tests (multiple files) Adapted to new APIs (summary checks, periodic/delayed handling moved to JobBoard instance).
Bench files Updated task name accessors to return const std::string&.
CMakeLists.txt Adds new source (thread_manager.cpp) and new test file (tasks_api.cpp).
.clang-tidy Disables additional CERT concurrency checks.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

- Overzealous type replacement
- Implicit include
- Redefined default name
@achamayou achamayou enabled auto-merge (squash) October 29, 2025 08:17
@achamayou achamayou merged commit 1b6d6d7 into microsoft:main Oct 29, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants