search

celerity / celerity-runtime

High-level C++ for Accelerator Clusters
https://celerity.github.io
MIT License
139 stars 18 forks source link

Explicitly manage buffer / host object lifetimes in graph generation #246

Closed fknorr closed 6 months ago

fknorr commented 7 months ago

This is a back-port from IDAG development and a second attempt at #216.

task_manager and distributed_graph_generator already maintain state for each buffer (add_buffer()) and host object (implicit on creation) , but keep that state around indefinitely even after the buffer or host object in question is destroyed. Also, neither graph generator has access to buffer debug names and thus can't include that information in error reports (such as uninitialized-read detection, there is a TODO in the code about this).

This PR adds explicit member functions for tracking the creation and destruction of buffers (create_buffer, destroy_buffer) and host objects (create_host_object, destroy_host_object) to task_manager, distributed_graph_generator, scheduler; and now by necessity, runtime, which receives these requests directly instead of indirectly through buffer_manager::buffer_lifetime_callback.

It also removes the recorder -> buffer_manager dependency by replicating the buffer name (like all other metadata) in both graph generators. This foreshadows the eventual removal of buffer_manager post-IDAG.

I purposefully changed the recording API such that its user is responsible for constructing records and the recorder simply aggregates them. This is now possible because the recorders do not need to know about task_manager and buffer_manager anymore, and will allow us, in the future, to add debug info to records that is not present in the "real" DAG (see the upcoming instruction graph generator for how this might look).

github-actions[bot] commented 7 months ago

Check-perf-impact results: (4c65f1399a47e0eb1340f63004745b17)

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

coveralls commented 7 months ago

Pull Request Test Coverage Report for Build 7976008559

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/recorders.cc 24 25 96.0%
<!-- Total: 205 206 99.51% -->
Totals Coverage Status
Change from base Build 7970610878: 0.1%
Covered Lines: 5046
Relevant Lines: 5239
💛 - Coveralls
github-actions[bot] commented 6 months ago

Check-perf-impact results: (f7ef1830fe7f830eee417b0b5a37c2ef)

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

fknorr commented 6 months ago

I've gone ahead and renamed every buffer_state variable to buffer in the DGGen for consistency.

github-actions[bot] commented 6 months ago

Check-perf-impact results: (19e9c0e63e2eff8d602cc7a81b622c19)

:heavy_check_mark: No significant performance change in the microbenchmark set. You are good to go!

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