Explicitly manage buffer / host object lifetimes in graph generation #246
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Check-perf-impact results: (4c65f1399a47e0eb1340f63004745b17) ❓ No new benchmark data submitted. ❓ |
fknorr
force-pushed
the
explicit-lifetime-mgmt
branch
2 times, most recently
from
7f025ed to
c23899d
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
fknorr
force-pushed
the
explicit-lifetime-mgmt
branch
from
c23899d to
33d4e65
Compare
Pull Request Test Coverage Report for Build 7976008559Details
💛 - Coveralls |
psalz
approved these changes
Feb 12, 2024
|
Check-perf-impact results: (f7ef1830fe7f830eee417b0b5a37c2ef) ❓ No new benchmark data submitted. ❓ |
|
I've gone ahead and renamed every |
PeterTh
reviewed
Feb 19, 2024
task_manager and distributed_graph_generator already maintain state for each buffer and host object, but keep it around indefinitely even after the buffer or host object in question is destroyed. Also, neither have access to buffer debug names and thus can't include that information in error reports (such as uninitialized-read detection). This commit adds explicit methods for tracking the creation and destruction of objects to task_manager, distributed_graph_generator, scheduler (and now by necessity, runtime, which receives these requests directly instead of via the buffer_lifetime_callback). This 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 with the merge of instruction graph scheduling.
fknorr
force-pushed
the
explicit-lifetime-mgmt
branch
from
2b675b1 to
b4ef1c0
Compare
fknorr
force-pushed
the
explicit-lifetime-mgmt
branch
from
b4ef1c0 to
b0717e7
Compare
|
Check-perf-impact results: (19e9c0e63e2eff8d602cc7a81b622c19) ✔️ No significant performance change in the microbenchmark set. You are good to go! Relative execution time per category: (mean of relative medians)
|
fknorr
force-pushed
the
explicit-lifetime-mgmt
branch
from
6098277 to
234e017
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a back-port from IDAG development and a second attempt at #216.
task_manageranddistributed_graph_generatoralready 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) totask_manager,distributed_graph_generator,scheduler; and now by necessity,runtime, which receives these requests directly instead of indirectly throughbuffer_manager::buffer_lifetime_callback.It also removes the
recorder->buffer_managerdependency by replicating the buffer name (like all other metadata) in both graph generators. This foreshadows the eventual removal ofbuffer_managerpost-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_managerandbuffer_manageranymore, 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).