Proposal for sessions/bundles with a start/stop API and simple example by davidozog · Pull Request #481 · openshmem-org/specification

davidozog · 2021-10-15T16:25:18Z

This PR addresses issue #189.

As always I appreciate feedback, thanks!

naveen-rn · 2021-10-15T17:36:08Z

@@ -0,0 +1,23 @@
+\newtext{The performance of \openshmem programs that issue many consecutive and


Irrespective of small/large-sized messages - bundles/sessions provides hints to the implementation to optimize. We need a better definition of what a bundle or session refers to.

I see what you mean, but I don't want to be prescriptive with defining the optimization... Do you have any suggestions on wording?

naveen-rn · 2021-10-15T17:36:51Z

+\begin{apidefinition}
+
+\begin{Csynopsis}
+void @\FuncDecl{shmem\_bundle\_start}@(void);


why do we need a separate API - I don't see why someone can't use the super-set API with ctx.

I don't think we need it, but I included for consistency with fence, quiet, PE-specific quiet, and the RMAs/AMOs. It's just a convenience routine - I'm happy to remove if a majority find it unnecessary.

naveen-rn · 2021-10-15T17:39:45Z

+\end{apiarguments}
+
+\apidescription{
+    The \FUNC{shmem\_bundle\_stop} routine provides a hint to the \openshmem


We are not defining the exit condition for this routine correctly - I'm not sure whether the intention is to have an implicit quiet as part of this routine.
Based on the example - it looks like there is an implicit quiet. Not sure - why this is defined in such a way.

As discussed in the WG's, there is no implicit quiet at the end of a bundle (but there is within the shmem_free, right? I don't think the example needs a quiet, but maybe the example could be improved in this regard).

My intent was for the statement:

[bundling routines] do not affect the completion or ordering semantics of any OpenSHMEM
routines in the program

to cover this. But perhaps it would help to include something like this in the API description or a note?

naveen-rn · 2021-10-15T17:46:08Z

+
+\begin{Csynopsis}
+void @\FuncDecl{shmem\_bundle\_start}@(void);
+void @\FuncDecl{shmem\_ctx\_bundle\_start}@(shmem_ctx_t ctx);


I think - we are missing two things:

There should a hint parameter for this routine - apart from the context. Hints should help implementations to support different options.

Should there be shmem_team_t as part of the argument? I'm not entirely sure about this requirement. My initial thinking is we should support - I'm open to suggestions. Adding teams argument - doesn't make it a collective call. Though, we can also retrieve this information from the ctx argument - it is useful to have teams argument along with or instead of teams. Proposed API:

vous shmem_bundle_start(shmem_team_t team, shmem_ctx_t ctx, int hints)

Hint can be used to determine whether team or ctx is passed. If multiple CTX or all CTX from a team is expected to create separate bundles - it would be optimal to use a single internal bundle.

I'm confused on this one - doesn't the context directly imply a team? I don't see why we would allow bundling on a team other than the context it's associated with...?

Assume the users want to create bundles for separate shared contexts from the same team. With this API, they would have calling bundle_start separately on each context. Internally, we couldn't perform chaining correctly - with just this information. As the network resource might be shared by multiple other shared contexts in that team.

My attempt to solve this - just pass team and say all contexts in this team is expected to be bundled. With this information and internal network resource information - implementations can create separate or one single chain.

Nevertheless - we need hints argument irrespective of whether we plan to add team argument or not.

hint would say - SHMEM_BUNDLE_TEAM or SHMEM_BUNDLE_CTX

+1 to the hints suggestion. It's long options on context creation.

I think we should have hints inside this routine and not in the context - I don't think we want to restrict the users to use a separate context for these sort of optimizations. They should be able to use the same context within the session and outside the session

If they misuse them - say use a shared context within session and simulateously outside on another thread - it is their perf issue to handle.

I agree. Sorry, my comment was a bit confusing. I was suggesting that the parameter to the proposed function could be called long options if we want to follow the convention set by the contexts API.

Hah! I like the long options suggestion as well, but I'm still confused about the team...

If multiple contexts share an underlying HW resource (STX or whatever) then yes, the runtime may choose to bundle across those contexts, but how does passing the team help? The runtime could check if the context is shared, and it should know what team it's on, and what other contexts are shared on that team. Am I missing something?

naveen-rn · 2021-10-15T17:55:45Z

+\apidescription{
+    The \FUNC{shmem\_bundle\_stop} routine provides a hint to the \openshmem
+    library to stop applying bundling-related optimizations.
+}


We are not specifically making it mandatory for users to call a stop after a start is called. I think, we should make it mandatory to call a stop after a start.

Agreed - thanks.

And some advice that it's a bad idea to go into non-shmem code (like a long computation loop) without a stop?

naveen-rn · 2021-10-15T17:57:24Z

+\apidescription{
+    The \FUNC{shmem\_bundle\_start} routine provides a hint to the \openshmem
+    library to being applying bundling-related optimizations.
+}


Do we need to handle a scenario - when multiple starts are called recursively without breaking the bundle?

shmem_start_bundle(ctx1) shmem_start_bundle(ctx1) shmem_start_bundle(ctx1) shmem_stop_bundle(ctx1)

Good catch - my hunch is to disallow this. Do you think so?

It might make sense to allow nesting across different contexts, do you think that should be allowed?

I'm not fond of nesting or disallowing. Just a note that stop affects them all in that example?

Something like "If bundling is already started, a call to shmem_start_bundle has no effect and if it is stopped, a call to shmem_stop_bundle has no effect."?

naveen-rn · 2021-10-15T18:00:48Z

One-more item from #189 are not yet addressed:

Defining the relationship between memory ordering routines and bundle? What happens when a memory ordering (explicit or implicit from collectives) is called within a bundle

davidozog · 2021-10-15T20:50:33Z

Thanks very much for the review @naveen-rn.

Defining the relationship between memory ordering routines and bundle? What happens when a memory ordering (explicit or implicit from collectives) is called within a bundle

I tried to capture that with:

[bundling routines] do not affect the completion or ordering semantics of any OpenSHMEM routines in the program.

For example, if a quiet occurs within a bundle, then the runtime must ensure remote completions on the calling PE as normal. Also:

routines, such as ... the memory ordering routines might require the library to enforce remote completion, reducing the potential benefit of bundling optimizations.

So while it's not strictly prohibited in this draft, it is discouraged to include memory ordering within a chain, because it may require the implementation to "break" the chaining optimization.

Does it makes sense? Do you think memory ordering be prohibited within a chain?

nspark · 2021-10-19T13:43:50Z

@@ -0,0 +1,23 @@
+\newtext{The performance of \openshmem programs that issue many consecutive and
+small-sized communication routines might be improved by combining these
+operations into fewer messages.


We don't use the term "message" elsewhere in the specification. Is there a different term we could use?

~~Maybe "operation"?~~ Oops, too many "operations" 😉

Yes, this might need some wordsmithing... Maybe "... by combining these routines into fewer [underlying] operations in the [OpenSHMEM] runtime [/library]"?

nspark · 2021-10-19T13:54:17Z

+The \emph{bundling routines} provide a convenient interface for indicating to
+the \openshmem library that a series of operations on a communication context
+are eligible for bundling-related optimization.


Who determines eligibility? Is it the application or the library? To nit-pick, I think it's the application that indicates suitability (e.g., now I will be issuing lots of nonblocking RMA operations), while it's the library and specification that determine eligibility (e.g, we cannot bundle blocking gets). Elsewhere, there's a comment from @bcernohous "that it's a bad idea to go into non-shmem code (like a long computation loop) without a stop." To me, this speaks to the suitability of the

I'll suggest something like:

The bundling routines provide a mechanism for an application to indicate to the OpenSHMEM library that it intends to issue a series of operations on a communication context that are suitable for bundling optimization.

nspark · 2021-10-19T15:52:05Z

+\newtext{The bundling routines are \textit{hints} to the \openshmem library,
+and they do not affect the completion or ordering semantics of any \openshmem
+routines in the program.


Suggested change

\newtext{The bundling routines are \textit{hints} to the \openshmem library,

and they do not affect the completion or ordering semantics of any \openshmem

routines in the program.

\newtext{The bundling routines are \textit{hints} to the \openshmem library;

they do not affect the completion or ordering semantics of any \openshmem

routines in the program.

nspark · 2021-10-19T15:59:39Z

+Because bundling is performed with respect to an \openshmem communication
+context, routines not performed on a communication context are ineligible for
+bundling optimization.}


This seems (initially, at least) to be contradictory to the shmem_bundle_{start,stop} API, which indicates bundling on the default context. Do we mean routines like collectives?

Yes, exactly - like collectives. I would argue that the default context is indeed a communication context, but I see how this could be confusing on first glance. Maybe, "routines not performed on a valid communication context..."? I believe we refer to "valid" contexts (including the default) elsewhere in the spec.

nspark · 2021-10-19T16:12:19Z

+    The \FUNC{shmem\_bundle\_start} routine provides a hint to the \openshmem
+    library to being applying bundling-related optimizations.


Suggested change

The \FUNC{shmem\_bundle\_start} routine provides a hint to the \openshmem

library to being applying bundling-related optimizations.

The \FUNC{shmem\_bundle\_start} routine provides a hint to the \openshmem

library to begin applying bundling-related optimizations.

I also feel iffy about the expression "bundling-related optimizations." In the intro, we have "bundling phase," so maybe...

Suggested change

The \FUNC{shmem\_bundle\_start} routine provides a hint to the \openshmem

library to being applying bundling-related optimizations.

The \FUNC{shmem\_bundle\_start} routine provides a hint to the \openshmem

library to begin a bundling phase.

Agreed, thanks - and we should also do this in shmem_bundle_stop.

nspark · 2021-10-29T17:51:06Z

I agree with @jdinan's comment that this API may be a better fit for the "Communication Management Routines."

In that case, maybe the naming should be slightly revised; e.g., shmem_ctx_bundle_start → shmem_ctx_start_bundle. But, this leaves shmem_bundle_start as something of an oddball.

nspark · 2021-10-29T18:15:24Z

+#include <shmem.h>
+#include <stdlib.h>


Suggested change

#include <shmem.h>

#include <stdlib.h>

#include <shmem.h>

#include <stdint.h>

#include <stdlib.h>

nspark · 2021-10-29T18:17:22Z

+#define N_UPDATES (1 << 20)
+#define N_INDICES (1 << 10)
+#define N_VALUES  (1 << 31)


Suggested change

#define N_UPDATES (1 << 20)

#define N_INDICES (1 << 10)

#define N_VALUES (1 << 31)

#define N_UPDATES (1lu << 20)

#define N_INDICES (1lu << 10)

#define N_VALUES (1lu << 31)

nspark · 2021-10-29T18:18:33Z

+
+  shmem_init();
+
+  uint64_t *table = shmem_malloc(N_INDICES * sizeof(uint64_t));


Suggested change

uint64_t *table = shmem_malloc(N_INDICES * sizeof(uint64_t));

uint64_t *table = shmem_calloc(N_INDICES, sizeof(uint64_t));

wrrobin · 2021-10-29T18:35:59Z

+      size_t random_idx = rand() % N_INDICES;
+      uint64_t random_val = rand() % N_VALUES;
+      shmem_uint64_atomic_xor(&table[random_idx], random_val, random_pe);
+  }


Minor: the indentations can be matched.

jdinan · 2021-10-29T18:39:11Z

+
+  int mype = shmem_my_pe();
+  int npes = shmem_n_pes();
+


Suggest adding srand(mype);

nspark · 2021-10-29T18:53:31Z

@naveen-rn suggested making bundling a property that could be enabled/disabled dynamically (i.e., not only at context creation), which would generalize the API for potential future dynamic properties on contexts.

nspark · 2022-03-14T18:40:26Z

@davidozog One case of interest to me is combining bundling with put-with-signal. I would like to think there's a good bit of room for optimization internal to an OpenSHMEM library.

For example, we often think of put-with-signal as roughly equivalent to:

shmem_putmem(..., pe);
shmem_fence();
shmem_set(&sigword, signal, pe);

(Note, we lack, and need, shmem_signal_set; see #382.)

In my mind, bundling + put-with-signal could effectively transform:

for (int i = 0; i < bound; i++)
  shmem_put_signal_nbi(..., &sigword, SHMEM_SIGNAL_SET, pe_targets[i]);

Into:

for (int i = 0; i < bound; i++)
  shmem_put_nbi(..., pe_targets[i]);
shmem_fence();
for (int i = 0; i < bound; i++)
  shmem_signal_set(&sigword, signal, pe_targets[i]);

Not every library would necessarily want to make such a transformation. But, I imagine some might find it advantageous.

David Ozog added 2 commits October 15, 2021 12:13

Add 1st draft of bundle start/stop API & example

2387433

Added change-log entry for the bundle routines

352f20f

davidozog added the Feedback Requested label Oct 15, 2021

naveen-rn requested changes Oct 15, 2021

View reviewed changes

naveen-rn reviewed Oct 15, 2021

View reviewed changes

nspark reviewed Oct 19, 2021

View reviewed changes

nspark reviewed Oct 29, 2021

View reviewed changes

wrrobin reviewed Oct 29, 2021

View reviewed changes

jdinan reviewed Oct 29, 2021

View reviewed changes

davidozog mentioned this pull request Nov 29, 2021

Operation Sessions Extension #189

Closed

davidozog mentioned this pull request Sep 28, 2022

OpenSHMEM Sessions (ctx hints) API #493

Merged

davidozog closed this Sep 28, 2022

		@@ -0,0 +1,23 @@
		\newtext{The performance of \openshmem programs that issue many consecutive and

		The \FUNC{shmem\_bundle\_start} routine provides a hint to the \openshmem
		library to being applying bundling-related optimizations.


		shmem_init();

		uint64_t table = shmem_malloc(N_INDICES sizeof(uint64_t));

	uint64_t table = shmem_malloc(N_INDICES sizeof(uint64_t));
	uint64_t *table = shmem_calloc(N_INDICES, sizeof(uint64_t));

Uh oh!

Conversation

davidozog commented Oct 15, 2021

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!

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!

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!

naveen-rn commented Oct 15, 2021

Uh oh!

davidozog commented Oct 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidozog Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

davidozog Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nspark commented Oct 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidozog Oct 29, 2021 •

edited

Loading

davidozog Oct 29, 2021 •

edited

Loading