Missing `const` qualifiers on team and other API objects

[Dan Bonachea]

From current spec:

team team::split(intrank_t color , intrank_t key);

However this method does not semantically mutate the target object, so I think it should be declared const

[Dan Bonachea]

Looking further, we are probably also missing const qualification for the team argument to basically every function that takes one, eg:

void rpc_ff(team &team, intrank_t recipient , Func &&func, Args &&...args);
... rpc(team &team, intrank_t recipient, Func &&func, Args &&...args);
void barrier(team &team = world());
RType barrier_async(team &team = world());
RType reduce_{one,all}(T &&value , BinaryOp &&op, intrank_t root, team &team = world());
RType broadcast(T &&value , intrank_t root, team &team = world());

None of these calls mutate the provided team object.

I believe the only semantic mutators on team are destroy(), the move constructor and the destructor. teams are otherwise immutable.

The implementation is also lacking the const qualifiers, so it's definitely too late to tackle for this release.

[Dan Bonachea]

A quick skim turned up a few other methods on other API objects that are probably missing const qualifiers (on the target object):

atomic_domain<T>::{load,store,compare_exchange,(fetch_)op}();
future <T...>::then(); 
future <T...>::wait(_tuple)(); 

[john bachan]

The team API was intentionally crafted as non-const. I agree that team's feel overwhelmingly const in their semantics, but I would still push back on slapping this qualifier on the code:

1. Team's contain a piece of state, the collective id generator, which is mutated to generate "names" (128-bit hashes) for collective calls. By making team operations const, we would have to exempt this piece of state with the mutable keyword. That isn't so terrible, but it forces us to never allow the collective id to be visible to users otherwise they could observe the non-constness. I would prefer to not constrain ourselves in this way.

2. The same argument applies to any other kind of bookkeeping we may want to do under teams in the future.

3. Const is not a beloved feature of c++, so I prefer to only use it where it has definite strengths. Like in deciding which copy or move constructor to use in some context. Const has little apparent value to me on non-value-like objects such as our team facade wrapping very mutable communication engine state (especially down in gasnet).

My general stance is that anything which communicates ought to be non-const.

The lack of const on the future API is an oversight I would agree to.

[Dan Bonachea]

Sounds like we at least agree to fix the following to be const methods:

future <T...>::then(); 
future <T...>::wait(_tuple)(); 

@john bachan said:

My general stance is that anything which communicates ought to be non-const.

I'm not sure what this means. Clearly all communication operations mutate something in the system, but not every object passed to a communication routine is mutated - source memory for bulk RMA being one obvious example. One could argue that the permissions on source memory are mutated because we prohibit freeing that region before source completion, but we've drawn the line at saying the semantically visible contents of source memory are const across the behavior of the operation, so the corresponding source pointer to RMA accepts a const-qualified target type.

I believe that currently team and atomic_domain are both semantically const with respect to all communication operations (excluding destroy). Semantically they are only used to "name" the process participants, which is a non-mutating query. In reality I believe atomic_domain really is immutable all the way down to the GASNet level throughout its lifetime - it really is nothing more than a non-changing set of metadata used to name an AMO process target and select the correct transport algorithm.

Semantically team is just an unchanging mapping between two spaces of rank_id, and a set of rules about required ordering of collective calls using them. IMO the fact the implementation may contain some mutable buffers or metadata uniquely associated with each team doesn't make those part of the client-level team object, or change its const-ness wrt to the client. GASNet can and does maintain mutable registration state associated with read-only inputs to RMA, but that doesn't change the client-visible constness of RMA source memory. Even if we later chose to expose things like statistical counters, these could be queries on a system object where a const team & argument just names which part of the communication engine we are asking about.

All that being said, I agree with John's orthogonal point which is that we don't provide copy operations for team and atomic_domain, so the lack of const-ness may have no relevant value/impact in practice. The main value I can think of is documentation and abstraction enforcement - if we had the proper const-ness suggested above, then a const qualifier on a team argument would prevent moves and destroys in a callee, but still allow their use for communication. Similarly, world() and local_team() could return const team & and we would not need verbiage like "The result is undefined if a move is performed on the returned team." that cannot be statically enforced. Of course it would also force users to use const on variables that might contain those teams, which might be too bothersome to be worthwhile.

[Dan Bonachea]

Add missing consts. Fixes #148.

→ <<cset 74c316b6a146>>

[Dan Bonachea]

Merge pull request #27

• const-fixes: Address comments on PR #27. Make an equivalence more explicit. Add newly introduced missing consts. Add future::result_reference() and future::wait_reference(). Fixes #150. Add missing consts. Fixes #148.

→ <<cset 43696a1f4dd2>>