Full recording of tasks and commands on request (e.g. for graph printing) #197

PeterTh · 2023-07-27T14:55:31Z

This was supposed to be a small thing I'll quickly finish before doing what I actually want to do. Oh well.

Results of some in-person discussions during development:

We want the recorded information to be standalone, simple data, without any dependencies at the point in time when it is printed.
Specifically, everything required for the output needs to be resolved when a task or command is recorded.
This is sufficiently important for us to accept that we will have some data structure code duplication in the storage data structures (which are completely independent from the data structures used during the actual execution).
Recording can be turned on with an environment variable. We do not believe that there will be any measurable performance impact while it is turned off (basically, one additional correctly predicted branch per command/task).

I still want to extend test coverage a bit before making the full PR, I noticed that a few things aren't covered at all during development.

Edit:
Added the additional tests and squashed into meaningful commit chunks. Ready for review.

github-actions · 2023-07-27T14:56:14Z

Check-perf-impact results: (9e900a306dc9f17e4a27439205a7680c)

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

fknorr

I like it!

One design decision that strikes me as odd is having the task manager / graph generator own the recorder. IMO we would have better separation of concerns if these classes received a pointer to an existing recorder, which would also allow us to get rid of both print_graph member functions.

include/print_graph.h

src/print_graph.cc

src/runtime.cc

PeterTh · 2023-08-09T15:31:34Z

Review comments addressed.

This includes a pretty large (in terms of LOC) change to all unit tests working directly with tasks, due to the introduction of a new helper context, which allows us to throw away a lot of boilerplate.

github-actions

clang-tidy made some suggestions

src/runtime.cc

test/runtime_tests.cc

test/task_graph_tests.cc

test/test_utils.h

psalz

Good stuff!

include/distributed_graph_generator.h

include/print_graph.h

include/task_manager.h

README.md

test/runtime_tests.cc

test/system/distr_tests.cc

test/buffer_manager_tests.cc

psalz

LGTM

src/recorders.cc

…e.g. for graph printing)

fknorr

Something I missed in the initial review:

test/distributed_graph_generator_test_utils.h

fknorr

LGTM!

psalz

Very nice that the task naming thing worked out so well in the end!

src/utils.cc

test/utils_tests.cc

Add testing context utility to reduce boilerplate and obviate the need for explicit calls to maybe_print.

- also includes some extended tests

* Recorders no longer owned by task manager / graph gen * Also fix member var naming scheme and namespaces according to review suggestions * Several minor improvements

PeterTh force-pushed the full-graph-recording branch 2 times, most recently from 63659b6 to df1d1e0 Compare August 1, 2023 09:18

PeterTh marked this pull request as ready for review August 1, 2023 09:18

PeterTh force-pushed the full-graph-recording branch from 7d2bb8c to c60e073 Compare August 1, 2023 14:39

PeterTh mentioned this pull request Aug 3, 2023

Implement breadth-triggered Horizons #199

Merged

fknorr reviewed Aug 9, 2023

View reviewed changes

include/print_graph.h Outdated Show resolved Hide resolved

include/print_graph.h Outdated Show resolved Hide resolved

src/print_graph.cc Outdated Show resolved Hide resolved

src/runtime.cc Outdated Show resolved Hide resolved

src/runtime.cc Outdated Show resolved Hide resolved

github-actions bot reviewed Aug 9, 2023

View reviewed changes

psalz force-pushed the full-graph-recording branch from f846555 to e199133 Compare August 10, 2023 11:03

PeterTh force-pushed the full-graph-recording branch from e199133 to ef27e17 Compare August 10, 2023 12:08

psalz requested changes Aug 10, 2023

View reviewed changes

psalz reviewed Aug 10, 2023

View reviewed changes

test/buffer_manager_tests.cc Outdated Show resolved Hide resolved

PeterTh force-pushed the full-graph-recording branch from ef27e17 to 65fdd46 Compare August 10, 2023 13:34

psalz approved these changes Aug 16, 2023

View reviewed changes

src/recorders.cc Outdated Show resolved Hide resolved

PeterTh force-pushed the full-graph-recording branch 2 times, most recently from ab0a662 to 8f791c7 Compare August 22, 2023 14:53

fknorr approved these changes Aug 23, 2023

View reviewed changes

fknorr pushed a commit to fknorr/celerity-runtime that referenced this pull request Aug 23, 2023

[BASE] celerity#197 Full recording of tasks and commands on request (…

0716c23

…e.g. for graph printing)

PeterTh force-pushed the full-graph-recording branch from 8f791c7 to a83b6f3 Compare August 24, 2023 10:33

fknorr requested changes Aug 24, 2023

View reviewed changes

test/distributed_graph_generator_test_utils.h Outdated Show resolved Hide resolved

PeterTh force-pushed the full-graph-recording branch from a83b6f3 to 926cf24 Compare August 28, 2023 14:11

PeterTh requested a review from fknorr August 28, 2023 15:06

fknorr approved these changes Aug 28, 2023

View reviewed changes

PeterTh requested a review from psalz September 7, 2023 10:36

psalz approved these changes Sep 8, 2023

View reviewed changes

src/utils.cc Outdated Show resolved Hide resolved

test/utils_tests.cc Outdated Show resolved Hide resolved

fknorr reviewed Sep 8, 2023

View reviewed changes

test/utils_tests.cc Show resolved Hide resolved

fknorr added this to the 0.5.0 milestone Sep 8, 2023

PeterTh force-pushed the full-graph-recording branch from 727de29 to 64fb5f0 Compare September 8, 2023 10:48

Add task test context

beb69fe

Add testing context utility to reduce boilerplate and obviate the need for explicit calls to maybe_print.

PeterTh added 6 commits September 8, 2023 13:02

Enable complete task graph recording and printing

6a006aa

Command graph recording and printing

3aeca7d

- also includes some extended tests

Test gathering of command graph from distributed nodes

7520645

Escape <> in task name strings when generating graph

8b9ec1c

Address review comments

9606205

* Recorders no longer owned by task manager / graph gen * Also fix member var naming scheme and namespaces according to review suggestions * Several minor improvements

Spend more effort to make type-derived task labels readable

ad4c05b

PeterTh force-pushed the full-graph-recording branch from 64fb5f0 to ad4c05b Compare September 8, 2023 11:02

PeterTh merged commit 660fdf9 into master Sep 11, 2023

PeterTh deleted the full-graph-recording branch September 11, 2023 09:36

fknorr mentioned this pull request Sep 21, 2023

Remove task_manager / buffer_manager dependency from recorders #216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full recording of tasks and commands on request (e.g. for graph printing) #197

Full recording of tasks and commands on request (e.g. for graph printing) #197

PeterTh commented Jul 27, 2023 •

edited

Loading

github-actions bot commented Jul 27, 2023

fknorr left a comment

PeterTh commented Aug 9, 2023

github-actions bot left a comment

psalz left a comment

psalz left a comment

fknorr left a comment

fknorr left a comment

psalz left a comment

Full recording of tasks and commands on request (e.g. for graph printing) #197

Full recording of tasks and commands on request (e.g. for graph printing) #197

Conversation

PeterTh commented Jul 27, 2023 • edited Loading

github-actions bot commented Jul 27, 2023

fknorr left a comment

Choose a reason for hiding this comment

PeterTh commented Aug 9, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

psalz left a comment

Choose a reason for hiding this comment

psalz left a comment

Choose a reason for hiding this comment

fknorr left a comment

Choose a reason for hiding this comment

fknorr left a comment

Choose a reason for hiding this comment

psalz left a comment

Choose a reason for hiding this comment

PeterTh commented Jul 27, 2023 •

edited

Loading