Track Message Type in Execution Graph #175
Conversation
| @@ -36,8 +36,6 @@ TEST_CASE_METHOD(MpiTestFixture, | |||
| rankA1, rankA2, BYTES(buffer), MPI_INT, messageData.size(), &status); | |||
| } | |||
|
|
|||
| REQUIRE(msg.intexecgraphdetails_size() == 1); | |||
There was a problem hiding this comment.
I would have had to increase this number every time we tracked a different thing. It did not seem a useful thing to test/easy to forget to change.
There was a problem hiding this comment.
See point about constant above, I think this is worth testing.
include/faabric/util/exec_graph.h
Outdated
| @@ -16,4 +16,6 @@ void incrementCounter(faabric::Message& msg, | |||
| const int valueToIncrement = 1); | |||
|
|
|||
| static inline std::string const mpiMsgCountPrefix = "mpi-msgcount-torank-"; | |||
|
|
|||
| static inline std::string const mpiMsgTypeCountPrefix = "mpi-msgtype-torank"; | |||
There was a problem hiding this comment.
I can't remember what the conclusion of our last discussion on these two points was, but to reiterate (feel free to ignore) I think these constants should (a) be #defines to fit with the style of the repo; (b) live in the MPI headers if possible because (i) they're MPI specific; (ii) this list will continue to grow and potentiall become quite big otherwise.
Either way, re. your point below about tracking the number of fields, I think it's still useful, but to more obviously connect them up, I'd add a constant wherever these labels are defined, e.g. #define N_MPI_EXEC_GRAPH_DETAILS 2, then use that in the test.
| @@ -36,8 +36,6 @@ TEST_CASE_METHOD(MpiTestFixture, | |||
| rankA1, rankA2, BYTES(buffer), MPI_INT, messageData.size(), &status); | |||
| } | |||
|
|
|||
| REQUIRE(msg.intexecgraphdetails_size() == 1); | |||
There was a problem hiding this comment.
See point about constant above, I think this is worth testing.
|
|
||
| // Work out the message type breakdown | ||
| faabric::util::exec_graph::incrementCounter( | ||
| *thisRankMsg, | ||
| fmt::format("{}-{}-{}", | ||
| faabric::util::exec_graph::mpiMsgTypeCountPrefix, | ||
| MPI_MSGTYPE_COUNT_PREFIX, |
There was a problem hiding this comment.
Does this string format then result in a double hyphen at the beginning (as the prefix already has a hyphen)?
There was a problem hiding this comment.
For the MSGTYPE_COUNT we have to provide two values, so the constant actually hasn't got a trailing hyphen. I will change it to make them both consistent.
I am half-hearted on wether we should merge this in or not. On the one hand it is useful to have, and doesn't do any harm. On the other hand, it's just a profiling tool we have used at a point in time.
I am slightly leaning towards merging it in, but also happy to keep this out.