Queue-less remote send/recv #105

csegarragonz · 2021-06-08T08:16:35Z

In this PR I introduce rank-to-rank messaging bypassing the central message endpoint server.

Sending and receiving messages is in the hot path for most workloads we run, thus we want to introduce the least contention. A low-hanging-fruit is allowing ranks to open messaging endpoints themselves.

As discussed offline, this exposes a bug not covered in the tests, for which I provide a solution in #111.

csegarragonz · 2021-06-09T14:24:31Z

src/transport/MpiMessageEndpoint.cpp

+  const std::shared_ptr<faabric::MPIMessage>& msg)
+{
+    // TODO - is this lazy init very expensive?
+    if (sendMessageEndpoint.socket == nullptr) {


In normal circumstances, I would open both sendMessageEndpoint and recvMessageEndpoint in the constructor. However, this is very problematic for testing. That's because:

mpiMessageEndpoints are static thread local, thus we need to create different worlds in separate threads.

If both send and recv sockets are opened in the constructor, both recv sockets will try to bind to the same adress + port, as they are on the same network namespace, yielding an error (even though we are only using one, in the simplest scenario).

Not that I believe branching could be the bottleneck here, but this is a case where testing influences implementation. Is this what TDD looks like in practice? 🤣

If the implementation relies on TLS then unfortunately the tests should exercise that TLS. As you say, this means we'd need to create worlds in different threads.

Re. the ports and testing, the solution is probably to make the base port configurable, then change the value in tests. In this case, each world could take a port offset as a constructor argument, set to the value we currently use by default, but overridden in tests.

Making this configurable arguably makes these classes more flexible and reusable so I'd say it's a +1 for testing!

As discussed offline, we'll first try with binding to a specific (protocol, address, port) tuple, and will leave port management until the multi-tenancy PR is under-way

src/scheduler/MpiWorld.cpp

Shillaker · 2021-06-14T08:06:55Z

src/scheduler/MpiWorld.cpp

+    // Initialise the endpoint vector if not initialised
+    if (mpiMessageEndpoints.size() == 0) {
+        for (int i = 0; i < size * size; i++) {
+            mpiMessageEndpoints.emplace_back(nullptr);
        }


I can see you've ported these changes into the other PR which I reviewed accidentally (#111), so the comments apply to both.

Relevant comments from the other PR:

a) Rather than spread it around, I think lazy initialisation for the rank should all happen in one place.
b) I think you can do mpiMessageEndpoints = std::vector(size * size, nullptr) and save a few lines.

a) Indeed.
b) mpiMessageEndpoints is a vector of std::unique_ptr so it has a deleted = operator. Thus I need to emplace nullptrs back. Note that this also answers your concern in #111 wrt mpiMessageEndpoint[index being actually initialised to nullptr.

Ah that's annoying, makes sense though.

Shillaker · 2021-06-14T08:07:45Z

src/scheduler/MpiWorld.cpp

+        // Get host for recv rank
+        std::string host = getHostForRank(recvRank);
+        assert(!host.empty());
+        assert(host != thisHost);


Same comment re. assertions and invalid ranks.

I think the first assert can go as we actually check this in getHostForRank but the latter has to do with logic particular to this method. I.e. if we send to a remote rank, the sending rank, must actually be in a remote host.

Shillaker · 2021-06-14T08:14:13Z

src/transport/MpiMessageEndpoint.cpp

+  const std::shared_ptr<faabric::MPIMessage>& msg)
+{
+    // TODO - is this lazy init very expensive?
+    if (sendMessageEndpoint.socket == nullptr) {


If the implementation relies on TLS then unfortunately the tests should exercise that TLS. As you say, this means we'd need to create worlds in different threads.

Re. the ports and testing, the solution is probably to make the base port configurable, then change the value in tests. In this case, each world could take a port offset as a constructor argument, set to the value we currently use by default, but overridden in tests.

Making this configurable arguably makes these classes more flexible and reusable so I'd say it's a +1 for testing!

src/transport/MpiMessageEndpoint.cpp

include/faabric/transport/MpiMessageEndpoint.h

csegarragonz · 2021-06-14T14:46:20Z

tests/test/scheduler/test_remote_mpi_worlds.cpp

 }

 TEST_CASE_METHOD(RemoteMpiTestFixture,
-                 "Test collective messaging across hosts",


These tests weren't using two different threads (not sure why they were passing). Given that I had to change them, I also separated them into smaller tests, easier to identify which fails.

csegarragonz · 2021-06-14T15:20:23Z

tests/utils/fixtures.h

@@ -117,6 +118,19 @@ class ConfTestFixture
    faabric::util::SystemConfig& conf;
 };

+class MessageContextFixture : public SchedulerTestFixture


Had to move this here to use it in the MpiMessageEndpoint tests.

csegarragonz self-assigned this Jun 8, 2021

csegarragonz added enhancement New feature or request mpi Related to the MPI implementation labels Jun 8, 2021

csegarragonz mentioned this pull request Jun 8, 2021

Introduce MPI-specific p2p message endpoint #102

Closed

csegarragonz force-pushed the queueless-v2 branch 2 times, most recently from 6c5b169 to a283e83 Compare June 9, 2021 09:56

csegarragonz marked this pull request as ready for review June 9, 2021 09:57

csegarragonz marked this pull request as draft June 9, 2021 10:07

csegarragonz commented Jun 9, 2021

View reviewed changes

csegarragonz force-pushed the queueless-v2 branch from 149105c to cfaa4bc Compare June 9, 2021 15:48

csegarragonz mentioned this pull request Jun 9, 2021

Latest faabric and cpp faasm/faasm#427

Closed

csegarragonz marked this pull request as ready for review June 11, 2021 10:46

csegarragonz added 5 commits June 11, 2021 10:48

adding p2p send/recv methods

1a34a00

not check queueing when we don't use queues

f4c0b5e

removing sendMPImessage from the function call API

185658d

update tests

3a29169

reusing sockets for rank-to-rank communication

13200da

csegarragonz force-pushed the queueless-v2 branch 2 times, most recently from 63706a4 to cdaf39e Compare June 11, 2021 10:53

adding more tests

421234d

csegarragonz force-pushed the queueless-v2 branch from cdaf39e to 421234d Compare June 11, 2021 11:01

csegarragonz requested a review from Shillaker June 11, 2021 14:09

Shillaker requested changes Jun 14, 2021

View reviewed changes

csegarragonz mentioned this pull request Jun 14, 2021

Different async mechanism: MPI message buffer #111

Merged

csegarragonz commented Jun 14, 2021

View reviewed changes

csegarragonz force-pushed the queueless-v2 branch from 359127a to 527541c Compare June 14, 2021 14:47

pr comments

886d0f5

csegarragonz force-pushed the queueless-v2 branch from 527541c to 886d0f5 Compare June 14, 2021 15:11

csegarragonz requested a review from Shillaker June 14, 2021 15:18

csegarragonz commented Jun 14, 2021

View reviewed changes

Shillaker approved these changes Jun 15, 2021

View reviewed changes

csegarragonz merged commit e3331a1 into master Jun 15, 2021

csegarragonz deleted the queueless-v2 branch June 15, 2021 09:38

csegarragonz mentioned this pull request Jun 15, 2021

Faabric update: MPI queue-less local messages, reworked async faasm/faasm#434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queue-less remote send/recv #105

Queue-less remote send/recv #105

csegarragonz commented Jun 8, 2021 •

edited

Loading

csegarragonz Jun 9, 2021 •

edited

Loading

Shillaker Jun 14, 2021 •

edited

Loading

csegarragonz Jun 14, 2021 •

edited

Loading

Shillaker Jun 14, 2021

csegarragonz Jun 14, 2021 •

edited

Loading

Shillaker Jun 15, 2021

Shillaker Jun 14, 2021

csegarragonz Jun 14, 2021

Shillaker Jun 14, 2021 •

edited

Loading

csegarragonz Jun 14, 2021

csegarragonz Jun 14, 2021

Queue-less remote send/recv #105

Queue-less remote send/recv #105

Conversation

csegarragonz commented Jun 8, 2021 • edited Loading

csegarragonz Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Shillaker Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

csegarragonz Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Shillaker Jun 14, 2021

Choose a reason for hiding this comment

csegarragonz Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Shillaker Jun 15, 2021

Choose a reason for hiding this comment

Shillaker Jun 14, 2021

Choose a reason for hiding this comment

csegarragonz Jun 14, 2021

Choose a reason for hiding this comment

Shillaker Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

csegarragonz Jun 14, 2021

Choose a reason for hiding this comment

csegarragonz Jun 14, 2021

Choose a reason for hiding this comment

csegarragonz commented Jun 8, 2021 •

edited

Loading

csegarragonz Jun 9, 2021 •

edited

Loading

Shillaker Jun 14, 2021 •

edited

Loading

csegarragonz Jun 14, 2021 •

edited

Loading

csegarragonz Jun 14, 2021 •

edited

Loading

Shillaker Jun 14, 2021 •

edited

Loading