Skip to content

Conversation

fknorr
Copy link
Contributor

@fknorr fknorr commented Oct 27, 2024

This PR is in preparation of the command lookahead feature, which will transparently avoid buffer resizes, but requires an arbitrary large task graph to be kept in memory, which is not possible with the bounded-capacity task_ring_buffer.

The old master-worker model required looking up tasks by task_id, so task_manager and task_ring_buffer provided this lookup functionality. This is no longer necessary, and the scheduler thread can be decoupled from task_manager by handling stable task pointers directly. This change simplifies the interactions between application thread and scheduler thread considerably.

  • Task instances are now managed in the same epoch-based structure employed for instructions, and the ring buffer size limit is gone.
  • Task-commands keep a const task* pointer directly, which is guaranteed to be live by the horizon mechanism. In the future task pointers can be introduced to instructions as well, avoiding kernel lambda copying.
  • The initial epoch is now generated explicitly instead of implicitly in the task_manager constructor, so it can be treated like any other instruction, avoiding many special-casings.
  • task_callback is replaced with a delegate interface, and [tci]dag_test_contexts can avoid manually constructing horizons.

@fknorr fknorr added this to the 0.7.0 milestone Oct 27, 2024
@fknorr fknorr requested review from psalz and PeterTh October 27, 2024 07:11
@fknorr fknorr self-assigned this Oct 27, 2024
Copy link

Check-perf-impact results: (e884a5b266877a2f8b1f68ea7d0b8bef)

❓ No new benchmark data submitted. ❓
Please re-run the microbenchmarks and include the results if your commit could potentially affect performance.

@fknorr fknorr force-pushed the remove-task-lookup branch from a0a351b to 784bdd9 Compare October 27, 2024 07:12
@coveralls
Copy link

coveralls commented Oct 27, 2024

Pull Request Test Coverage Report for Build 11609757631

Details

  • 152 of 152 (100.0%) changed or added relevant lines in 15 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.05%) to 95.249%

Files with Coverage Reduction New Missed Lines %
src/print_graph.cc 1 94.51%
Totals Coverage Status
Change from base Build 11603776400: 0.05%
Covered Lines: 6730
Relevant Lines: 6831

💛 - Coveralls

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@fknorr fknorr mentioned this pull request Oct 28, 2024
Copy link

Check-perf-impact results: (da9a532564f4bc5510893652bf98be8b)

⚠️ Significant slowdown (>1.25x) in some microbenchmark results: benchmark intrusive graph dependency handling with N nodes - 100 / checking for dependencies, normalizing a fully mergeable tiling of boxes - 1 / large, embedded in 3d
Added microbenchmark(s): task-graph, benchmark task handling / generating and deleting tasks
Removed microbenchmark(s): task-graph, benchmark task handling > without access thread / generating and deleting tasks, task-graph, benchmark task handling > with access thread / generating and deleting tasks with access thread

Relative execution time per category: (mean of relative medians)

  • command-graph : 1.02x
  • graph-nodes : 1.05x
  • grid : 1.04x
  • instruction-graph : 1.01x
  • scheduler : 1.04x
  • system : 0.99x
  • task-graph : 0.98x

Copy link

Check-perf-impact results: (2a3c70512edf780ef21985d30e324aa9)

🚀 Significant speedup (<0.80x) in some microbenchmark results: normalizing a fully mergeable tiling of boxes - 1 / large, native
Added microbenchmark(s): task-graph, benchmark task handling / generating and deleting tasks
Removed microbenchmark(s): task-graph, benchmark task handling > without access thread / generating and deleting tasks, task-graph, benchmark task handling > with access thread / generating and deleting tasks with access thread

Relative execution time per category: (mean of relative medians)

  • command-graph : 0.96x
  • graph-nodes : 1.00x
  • grid : 1.02x
  • instruction-graph : 0.98x
  • scheduler : 1.01x
  • system : 0.98x
  • task-graph : 0.87x 🚀

@fknorr fknorr force-pushed the remove-task-lookup branch from 72577dd to 1f38098 Compare October 31, 2024 07:59
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/6)

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (2/6)

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (3/6)

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (4/6)

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (5/6)

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (6/6)

@fknorr fknorr force-pushed the remove-task-lookup branch from 1f38098 to b1b034e Compare October 31, 2024 09:32
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/2)

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (2/2)

The old master-worker model required looking up tasks by task_id, so
task_manager and task_ring_buffer provided this lookup functionality.
This is no longer necessary, and the scheduler thread can be decoupled
from task_manager by handling stable task pointers directly.

Task instances are now managed in the same epoch-based structure
employed for instructions, and the ring buffer size limit is gone.

Additionally, the initial epoch is now generated explicitly instead of
implicitly in the task_manager constructor, so it can be treated like
any other instruction, avoiding many special-casings.
@fknorr fknorr force-pushed the remove-task-lookup branch from b1b034e to c85888f Compare October 31, 2024 09:56
@fknorr fknorr merged commit b6222f7 into master Oct 31, 2024
27 of 29 checks passed
@fknorr fknorr deleted the remove-task-lookup branch October 31, 2024 10:08
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.

4 participants