Implement GET protocol for dependencies by devreal · Pull Request #420 · ICLDisco/parsec

devreal · 2022-08-16T21:23:39Z

Avoids a round-trip by directly fetching data when a dependency release arrives.
Adds runtime_comm_get MCA parameter to enable use of the GET protocol.

Currently enabled to have it worked by CI. I'm not sure I am using the right datatypes since the reshape and redistribute tests are failing...

Signed-off-by: Joseph Schuchart schuchart@icl.utk.edu

omor1 · 2022-08-17T00:16:35Z

Have you looked at performance at all? I think I've discussed with @bosilca ad nauseam regarding the implications regarding communication prioritization. I might pull a version of this into my branch at some point so I can test.

I'm not sure I am using the right datatypes since the reshape and redistribute tests are failing...

There could very well be a bug in the GET implementation for the MPI comm engine that has gone undiagnosed since it's not been well-tested.

devreal · 2022-08-17T00:26:20Z

I implemented this to have a better baseline in the comparison with TTG (which does GET instead of PUT). As far as I remember, there was little to no benefit in terms of performance (didn't get worse though).

omor1 · 2022-08-17T00:32:29Z

didn't get worse though

That's relevant! How much did you scale and were you using THREAD_MULTIPLE?

George's hypotheses regarding this are, if I recall correctly, along the lines that a sender can more easily regulate how much data it pushes onto the network than the receiver—with a GET protocol the sender doesn't have as much ability to prioritize communications, so multiple receivers can end up competing for a sender's bandwidth. On the other hand, a PUT protocol can overwhelm a receiver with many incoming messages, but that shouldn't be the case for most PaRSEC applications since the receiver also regulates which data it requests to be sent.

devreal · 2022-09-23T16:28:47Z

Sweet, it seems to have been a problem with the termination detection @therault :) All checks pass now

Adds runtime_comm_get MCA parameter to enable use of the GET protocol. Signed-off-by: Joseph Schuchart <schuchart@icl.utk.edu>

devreal · 2022-09-23T17:01:12Z

Argh of course not, CI doesn't test the GET protocol. Test fail if run with PARSEC_MCA_runtime_comm_get=1. I still get MPI_ERR_TRUNCATE errors, so there is a problem with GET.

omor1 · 2022-09-23T20:00:27Z

Argh of course not, CI doesn't test the GET protocol. Test fail if run with PARSEC_MCA_runtime_comm_get=1. I still get MPI_ERR_TRUNCATE errors, so there is a problem with GET.

MPI_ERR_TRUNCATE should only occur if you send more than the matching receive expects right?

I think I might have found the bug. When a process gets too many internal GET AMs (or is asked to do too many PUTs), then it defers starting the MPI_Isend until later by pushing to mpi_funnelled_dynamic_req_fifo. When starting the send later, it expects the tag to be used to be in cb.cb_type.onesided.size—why there, I don't know. mpi_no_thread_put does this correctly, but mpi_funnelled_internal_get_am_callback seems to do things terribly wrong—I have no idea if it even sends to the correct destination in this case, or what data it's sending, since it's using entirely different fields in the cb_type union. Wow this seems broken.

Like I said, no one has tested GET really.

devreal · 2022-09-23T20:05:10Z

Sigh, thanks for checking @omor1. I'm fairly sure I had it working at some point before the big merge. The MPI backend is still student research quality, at best. I hate the fact that we're putting data into random fields, makes the code unmaintainable. I guess It's good to have pointer though, will have to take a closer look at it again...

omor1 · 2022-09-23T20:12:18Z

The MPI backend is still student research quality, at best.

"research quality"

The LCI backend is better, in my humble opinion, and is certainly better-documented. It still has a decent amount of jankiness from various things I tried and haven't fully ripped out, but is more maintainable.

bosilca · 2022-10-12T17:23:41Z

+                parsec_type_size(dtt, &dtt_size);
+                parsec_ce.mem_register(dataptr, PARSEC_MEM_TYPE_CONTIGUOUS,
+                                      -1, NULL,
+                                      dtt_size,


aren't we missing the count here ? parsec_type_size returns the size of the dtt type but it does not account for the nbdtt, so we need to scale it up for the mem_register in the contiguous case.

Yes—this has long been fixed on my branch, see 948aa58.

@omor1 if you have a fix, would you mind upstreaming them to this branch?

@omor1 any fixes that you have are more than welcomed.

bosilca · 2022-10-12T17:24:31Z

+    /* Retreive deps from callback_data */
+    remote_dep_cb_data_t *cb_data = (remote_dep_cb_data_t *)msg;
+    parsec_remote_deps_t* deps = cb_data->deps;
+	parsec_execution_stream_t* es = &parsec_comm_es;


indentation.

bosilca · 2022-10-12T17:25:15Z

+    ce->mem_unregister(cb_data->memory_handle);
+    parsec_thread_mempool_free(parsec_remote_dep_cb_data_mempool->thread_mempools, cb_data);
+
+    parsec_comm_puts--;


puts or gets ?

I don't think that any of parsec_comm_gets_max, parsec_comm_gets, parsec_comm_puts_max, and parsec_comm_puts are actually being used in any way, so the point is moot—the number of concurrent communications is being managed by each communication engine, not at the upper layer.

bosilca · 2022-10-12T17:26:16Z

+                parsec_type_size(dtt, &dtt_size);
+                parsec_ce.mem_register(PARSEC_DATA_COPY_GET_PTR(deps->output[k].data.data), PARSEC_MEM_TYPE_CONTIGUOUS,
+                                      -1, NULL,
+                                      dtt_size,


same comment as above, we need to account for nbddt in the contiguous case.

bosilca · 2022-10-12T17:27:38Z

+                    receiver_memory_handle,
+                    receiver_memory_handle_size );
+
+            // TODO: fix the profiling!


still TODO's left in the code.

devreal requested a review from a team as a code owner August 16, 2022 21:23

devreal force-pushed the get-protocol branch 3 times, most recently from 83623c1 to 1b856ca Compare September 23, 2022 16:14

Implement GET protocol for dependencies

edbd8fe

Adds runtime_comm_get MCA parameter to enable use of the GET protocol. Signed-off-by: Joseph Schuchart <schuchart@icl.utk.edu>

devreal force-pushed the get-protocol branch from 1b856ca to edbd8fe Compare September 23, 2022 16:31

bosilca reviewed Oct 12, 2022

View reviewed changes

devreal marked this pull request as draft January 25, 2024 18:09

Conversation

devreal commented Aug 16, 2022

Uh oh!

omor1 commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devreal commented Aug 17, 2022

Uh oh!

omor1 commented Aug 17, 2022

Uh oh!

devreal commented Sep 23, 2022

Uh oh!

devreal commented Sep 23, 2022

Uh oh!

omor1 commented Sep 23, 2022

Uh oh!

devreal commented Sep 23, 2022

Uh oh!

omor1 commented Sep 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

omor1 commented Aug 17, 2022 •

edited

Loading