nlesc / constellation Goto Github PK

ConstellationIdentifier is now part of the implementation. It makes sense to move this to the top level API where ActivityIdentifier is also located.

See subject

In the current implementation, the OrActivityContext behaves either like a bag or a list, depending on the value of the ordered parameter of the constructor. If ordered is true the order matters and the context is a list, if it is false it does not matter and the order is a bag.

The implementation of equals however seems to test for set equality in the worst case (slow path) implementation, but behaves differently in the fast path implementation. This is hard to trigger in a test, as several fast paths guarantee that this slow path is almost never reached.

One issue is the comparison in the occurrence of doubles values. In other words:

 (A, B) == (A, B, B) ?

The current implementation says no since their length is different. Are these the semantics we want ?

Similar problems occur with comparing ordered and not ordered, or does:

(B, A)[with order] == (A, B)[without order]

Again, the fast path in equals says no (by comparing their hash codes), the slow path says yes by testing set equality. Again, are these the semantics we want ?

In the api-change branch, should we merge Context, AbstractContext, OrContext into a single Context object? Compare with StealPool, which has a similar structure.

The OrActivityContext gets an array of UnitActivityContext as first parameter, but does not check in the constructor whether or not one of the array values is null. For example:

   new OrActivityContext(new UnitActivityContext[2], true)

is accepted without complaints. However, not all method can handle these null values. For example, hashcode can but equals cannot, and neither can the constructor with false as the last parameter instead of true.

This suggest the the null values should not be accepted in the first place. Alternatively, the array should be collapsed to not contain any null values anymore (as it is copied anyway).

Will fix this in the unittest branch.

The Event.toString() contains dead code:

    String s = "source: ";
    if (getSource() != null) {
        s += getSource().toString();
    } else {
        s += "none";
    }
    s += "; target: ";
    if (getTarget() != null) {
        s += getTarget().toString();
    } else {
        s += "none";
    }

The getSource() and getTarget() calls will never return null since the source and target fields are used to invoke a method in the constructor of Event. As a result both else clauses are unreachable (also for tests).

... but we should be able to remove them as well.

As it is, Activity.identifier() may return null, which may not be what you expect. Maybe it should be initialized to some predefined constant if it is not scheduled yet?

If there are now only two steal strategies left, why do we still need a public constructor? We could just do with the two public possible values. Other values are rejected now anyway.

When calling FlexibleEventCollector.toString an expection may be thrown if the FlexibleEventCollector has not been submitted yet. This is unexpected behaviour for a toString method

Constellation.identifier() now returns a String. It would be more logical to return a ConstellationIdentifier instead after issue #3 has been resolved.

Should we be able to do this?

The API seperation of Constellation seems confused. For example, the top level API contains an ibis.constellation.ActivityIdentifier class that extends an ibis.constellation.impl.ActivityIdentifierImpl class in the implementation subpackage. This does not make sense?

Either the top level should truely be a separate API (and therefore not import things from the implementation), or the whole thing should be flattened ?

We currently have two types of contexts, ActivityContext and ExecutorContext. Both have the UnitContext and OrContext subtypes.

I'm wondering if we could replace this with a single Context implementation (keeping the Unit and Or subtypes).

I remember there was a reason for this split, but unfortunately don't remember the reason itself. It probably has to do with the ranges and matching, but I have the feeling there are more elegant solutions for this, like completely hiding the matching in the impl package.