Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Faabric distributed coordination and merge operations #534

Merged
merged 32 commits into from
Nov 12, 2021
Merged

Conversation

Shillaker
Copy link
Collaborator

@Shillaker Shillaker commented Nov 1, 2021

Update Faasm multi-threaded code to use latest Faabric features, including distributed coordination (faasm/faabric#161), snapshot merge operations (faasm/faabric#142), and scheduler hints (faasm/faabric#169).

Changes:

  • Use snapshot merge operations in OpenMP, both for general shared variables, reduce results.
  • Use Faabric PointToPointGroups for OpenMP locking and barriers.
  • Cache the SchedulingDecision for each level of an OpenMP application, then pass this as a hint when scheduling the same number of threads at the same depth.
  • Remove coordination logic from OpenMP Level class (now handled by PointToPointGroup).
  • Replace MutexManager with Faabric PointToPointGroup in pthread code.

This is an update of #480 which had gone stale.

I also tidied up a couple of things:

  • Remove faabric container from Faasm docker-compose.yml. We should use the faabric CLI container in the Faabric project itself.
  • Remove unhelpful/ stale OpenMP doc.

@Shillaker Shillaker self-assigned this Nov 1, 2021
// One page is 64kB
#define DYNAMIC_MODULE_STACK_SIZE (2 * ONE_MB_BYTES)
#define DYNAMIC_MODULE_MEMORY_SIZE (66 * WASM_BYTES_PER_PAGE)
#define GUARD_REGION_SIZE (10 * WASM_BYTES_PER_PAGE)
Copy link
Collaborator Author

@Shillaker Shillaker Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just moved this stuff out of the wasm module header as it's shared configuration and useful elsewhere.

@Shillaker Shillaker marked this pull request as ready for review November 10, 2021 15:05
@Shillaker Shillaker marked this pull request as draft November 11, 2021 09:04
@Shillaker Shillaker marked this pull request as ready for review November 11, 2021 14:49
@@ -337,106 +338,4 @@ TEST_CASE_METHOD(WasmSnapTestFixture,

::munmap(snapMemory, snapSize);
}

TEST_CASE_METHOD(WasmSnapTestFixture,
"Test ignoring stacks",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This no longer happens.

Copy link
Collaborator

@csegarragonz csegarragonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Just two minor things.

@@ -73,7 +73,7 @@ More detail on some key features and implementations can be found below:
- [Kubernetes and Knative integration](docs/kubernetes.md)- deploying Faasm as part of a full serverless platform.
- [Bare metal/ VM deployment](docs/bare_metal.md) - deploying Faasm on bare metal or VMs as a stand-alone system.
- [API](docs/api.md) - invoking and managing functions and state through Faasm's HTTP API.
- [MPI](docs/mpi.md) and [OpenMP](docs/openmp.md) - executing existing MPI and OpenMP applications in Faasm.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there value in linking the OpenMp docs from the cpp repo here? Maybe worth for the future to have accessible documentation about MPI and OpenMP in Faasm.

Copy link
Collaborator Author

@Shillaker Shillaker Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the OpenMP docs said anything about the Faasm implementation of OpenMP then there would be, however, those docs are purely about debugging issues with compilation, which is only relevant to the cpp repo.

The MPI docs are also not really relevant to this repo and focus on compiling and uploading functions, so could also be moved to cpp.

@@ -5,6 +5,7 @@
#include <faabric/scheduler/Scheduler.h>

namespace tests {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually don't add this lines myself, does clang-format say something about it?

Copy link
Collaborator Author

@Shillaker Shillaker Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see what harm this newline does, in fact, I think it makes it easier to read as it clearly separates the namespace declaration from whatever comes next. The Google style guide seems to agree as all their examples have a newline: https://google.github.io/styleguide/cppguide.html#Namespaces. What's the argument for not adding it?

Doing a brief scan I can see that the repo is pretty mixed at the moment and can't find a rule in clang-tidy to enforce anything so we can leave it for now.

@Shillaker Shillaker merged commit 2367e1e into master Nov 12, 2021
@Shillaker Shillaker deleted the omp-coord branch November 12, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants