Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move completed-epoch handling out of task_manager #299

Merged
merged 2 commits into from
Nov 14, 2024
Merged

Conversation

fknorr
Copy link
Contributor

@fknorr fknorr commented Nov 4, 2024

Replaces epoch_monitor with an atomic counter in runtime for the purpose of TDAG pruning, and re-uses the fence promise mechanism for suspending the main thread when waiting on an epoch task.

This consolidates the "previous horizon is applied as an epoch" logic, which was previously duplicated, in a single location in runtime.

Furthermore, task_graph ownership is moved out of task_manager, making it a single-threaded graph generator like the others. A rename to task_graph_generator should follow eventually but was omitted here to keep diff noise low.

@fknorr fknorr added this to the 0.7.0 milestone Nov 4, 2024
@fknorr fknorr requested review from psalz and PeterTh November 4, 2024 11:20
@fknorr fknorr self-assigned this Nov 4, 2024
Copy link

github-actions bot commented Nov 4, 2024

Check-perf-impact results: (a2d9681a083c32e2b46fe0fbb689f304)

⚠️ Significant slowdown (>1.25x) in some microbenchmark results: generating large task graphs / jacobi topology

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

  • command-graph : 1.01x
  • graph-nodes : 1.03x
  • grid : 1.00x
  • instruction-graph : 0.98x
  • scheduler : 1.01x
  • system : 1.03x
  • task-graph : 1.11x ⚠️

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/5)

include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
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/5)

include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/runtime.h Outdated Show resolved Hide resolved
include/runtime.h Show resolved Hide resolved
include/runtime.h Show resolved Hide resolved
include/runtime.h Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
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/5)

include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
test/task_graph_tests.cc Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
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/5)

src/runtime.cc Show resolved Hide resolved
src/runtime.cc Show resolved Hide resolved
src/runtime.cc Show resolved Hide resolved
src/runtime.cc Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
test/test_utils.h Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
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/5)

include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
src/task_manager.cc Show resolved Hide resolved
src/task_manager.cc Show resolved Hide resolved
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/1)

src/runtime.cc Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Nov 4, 2024

Pull Request Test Coverage Report for Build 11839608873

Details

  • 62 of 63 (98.41%) changed or added relevant lines in 12 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 95.164%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/runtime.cc 26 27 96.3%
Totals Coverage Status
Change from base Build 11836687260: -0.01%
Covered Lines: 6682
Relevant Lines: 6780

💛 - Coveralls

Copy link
Member

@psalz psalz left a comment

Choose a reason for hiding this comment

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

I like it, LGTM!

src/runtime.cc Outdated Show resolved Hide resolved
include/task_manager.h Outdated Show resolved Hide resolved
test/dag_benchmarks.cc Show resolved Hide resolved
Copy link

Check-perf-impact results: (a42f16ae449d806ee4af2ef57be62d77)

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

Replaces `epoch_monitor` with an atomic counter in `runtime` for the
purpose of TDAG pruning, and re-uses the fence promise mechanism for
suspending the main thread when waiting on an epoch task.

This consolidates the "previous horizon is applied as an epoch" logic,
which was previously duplicated, in a single location in `runtime`.

Furthermore, `task_graph` ownership is moved out of `task_manager`,
making it a single-threaded graph generator like the others. A rename to
`task_graph_generator` should follow eventually but was omitted here to
keep diff noise low.
Copy link

Check-perf-impact results: (bb30e23d8de4cfa5e0bbdb911e0279cc)

⚠️ Significant slowdown (>1.25x) in some microbenchmark results: generating large task graphs / soup topology

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

  • command-graph : 1.02x
  • graph-nodes : 0.99x
  • grid : 1.02x
  • instruction-graph : 1.00x
  • scheduler : 1.01x
  • system : 1.02x
  • task-graph : 1.02x

@fknorr fknorr merged commit 46f6194 into master Nov 14, 2024
17 checks passed
@fknorr fknorr deleted the epoch-promise branch November 14, 2024 16:55
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