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

Fix for broken deadline propagation mechanism #1451

Merged
merged 26 commits into from
Dec 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
dd9eda4
Set the deadline member of ReactionInstance when declaredDeadline is set
Nov 2, 2022
7444959
Add getInheritedDeadline
Nov 2, 2022
f919955
Add tests for checking priority of deadline reactions
erlingrj Nov 2, 2022
56dbd5b
Use inherited deadline to assign priority to reaction
Nov 3, 2022
ea95ede
Fix deadline tests. They need to be single-threaded.
erlingrj Nov 3, 2022
397b62e
We need to make string of the UNSIGNED value of the index/deadline
erlingrj Nov 3, 2022
5f61ddf
Format Deadline tests
erlingrj Nov 3, 2022
af4d53b
Rename to inferredDeadline and break cycles when setting it.
erlingrj Nov 5, 2022
3a68ba3
FIx reference to reactionInstance.deadline, should be declaredDeadlin…
erlingrj Nov 5, 2022
36c7b58
Move inferred deadline out of reactionInstance and into Runtime
erlingrj Nov 6, 2022
e05ef70
Implement assignInferredDeadlines in ReactionInstanceGraph. Call it i…
erlingrj Nov 6, 2022
fad06e9
WIP: Use Runtime.deadline in code-generation
erlingrj Nov 6, 2022
0d089e6
Add functions for getting Set and List of deadlines from ReactionInst…
erlingrj Nov 6, 2022
5733906
Fix mistake in assignInferredDeadlines
erlingrj Nov 6, 2022
71b0591
Fix code generation. Get deadlines in same way as levels are got.
erlingrj Nov 6, 2022
e6378e3
Add new test program for Banked reactors
erlingrj Nov 6, 2022
ca43fe1
Increase deadline to 100sec to fix macOS CI
erlingrj Nov 7, 2022
e7ad63c
Detect if there are deadlines globally.
Nov 29, 2022
794de2c
Fix typo
erlingrj Nov 29, 2022
02301f1
Fix some comments
erlingrj Nov 29, 2022
53fd90e
Address Edwards comments
erlingrj Nov 30, 2022
3c95857
Apply suggestions from code review
erlingrj Nov 30, 2022
26b861b
Address Martens comments
erlingrj Nov 30, 2022
1c95d0e
Remove redundant(?) calls to assignLevels
erlingrj Dec 7, 2022
a9c7a7e
Undo mistake
erlingrj Dec 7, 2022
7228484
Merge branch 'master' into fix-index-deadline3
erlingrj Dec 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ public KPolygon addReactionFigure(KNode node, ReactionInstance reaction) {
if (hasDeadlineCode) {
KText contentContainerText = _kContainerRenderingExtensions.addText(contentContainer,
_utilityExtensions.trimCode(reaction.getDefinition().getDeadline().getCode()));
associateWith(contentContainerText, reaction.deadline);
associateWith(contentContainerText, reaction.declaredDeadline);
_kRenderingExtensions.setForeground(contentContainerText, Colors.BROWN);
_kRenderingExtensions.setFontSize(contentContainerText, 6);
_kRenderingExtensions.setFontName(contentContainerText, KlighdConstants.DEFAULT_MONOSPACE_FONT_NAME);
Expand Down
6 changes: 6 additions & 0 deletions org.lflang/src/org/lflang/generator/GeneratorBase.java
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,12 @@ public abstract class GeneratorBase extends AbstractLFValidator {
*/
public boolean hasModalReactors = false;

/**
* Indicates whether the program has any deadlines and thus
* needs to propagate deadlines through the reaction instance graph
*/
public boolean hasDeadlines = false;

// //////////////////////////////////////////
// // Target properties, if they are included.
/**
Expand Down
1 change: 1 addition & 0 deletions org.lflang/src/org/lflang/generator/GeneratorUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.lflang.lf.KeyValuePair;
import org.lflang.lf.KeyValuePairs;
import org.lflang.lf.Model;
import org.lflang.lf.Reaction;
import org.lflang.lf.Reactor;
import org.lflang.lf.TargetDecl;
import org.lflang.util.FileUtil;
Expand Down
59 changes: 48 additions & 11 deletions org.lflang/src/org/lflang/generator/ReactionInstance.java
Original file line number Diff line number Diff line change
Expand Up @@ -223,11 +223,6 @@ public ReactionInstance(
*/
public DeadlineInstance declaredDeadline;

/**
* Inferred deadline. Defaults to the maximum long value.
*/
public TimeValue deadline = new TimeValue(TimeValue.MAX_LONG_DEADLINE, TimeUnit.NANO);

/**
* Sadly, we have no way to mark reaction "unordered" in the AST,
* so instead, we use a magic comment at the start of the reaction body.
Expand Down Expand Up @@ -356,12 +351,27 @@ public Set<ReactionInstance> dependsOnReactions() {
public Set<Integer> getLevels() {
Copy link
Member

Choose a reason for hiding this comment

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

Unless assignLevels is invoked in the constructor, there is no assurance that this method will always return a sensible result. Where is assignLevels called?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

assignLevels is invoked once in createMainReactorInstance() in CGenerator.c. So it is called once on the top level reactor, which, if my understanding is correct should do the calculation for the whole reaction graph exactly once

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the problem that was addressed by calling assignLevels, "just in case" is that the level of a reaction is not known until you have constructed the whole reaction graph. So you cannot just do it in the constructor. A simple fix would be to add a simple function to reactorInstance which checks if levels has been assigned yet. (hasLevelsBeenAssigned()) this would walk up the containment hierarchy to the main reactor and check if it has created a reactionInstanceGraph yet (and thus assigned levels). If this for some reason is not true, then I propose to throw an error because the developer might be requesting the levels at a point in time where they are not available (e.g. before the reaction graph has been fully constructed).

How does this sound?

Copy link
Member

Choose a reason for hiding this comment

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

OK, that sounds like a good stop-gap measure to me. Still not fond of the original design, but the error will help prevent confusion.

Set<Integer> result = new LinkedHashSet<>();
// Force calculation of levels if it has not been done.
parent.assignLevels();
// FIXME: Comment out this as I think it is redundant.
// If it is NOT redundant then deadline propagation is not correct
// parent.assignLevels();
for (Runtime runtime : runtimeInstances) {
result.add(runtime.level);
}
return result;
}

/**
* Return a set of deadlines that runtime instances of this reaction have.
* A ReactionInstance may have more than one deadline if it lies within.
*/
public Set<TimeValue> getInferredDeadlines() {
Set<TimeValue> result = new LinkedHashSet<>();
for (Runtime runtime : runtimeInstances) {
result.add(runtime.deadline);
}
return result;
}


/**
* Return a list of levels that runtime instances of this reaction have.
Expand All @@ -372,12 +382,28 @@ public Set<Integer> getLevels() {
public List<Integer> getLevelsList() {
List<Integer> result = new LinkedList<>();
// Force calculation of levels if it has not been done.
parent.assignLevels();
// FIXME: Comment out this as I think it is redundant.
// If it is NOT redundant then deadline propagation is not correct
// parent.assignLevels();
for (Runtime runtime : runtimeInstances) {
result.add(runtime.level);
}
return result;
}

/**
* Return a list of the deadlines that runtime instances of this reaction have.
* The size of this list is the total number of runtime instances.
* A ReactionInstance may have more than one deadline if it lies within
*/
public List<TimeValue> getInferredDeadlinesList() {
List<TimeValue> result = new LinkedList<>();
for (Runtime runtime : runtimeInstances) {
result.add(runtime.deadline);
}
return result;
}


/**
* Return the name of this reaction, which is 'reaction_n',
Expand Down Expand Up @@ -530,11 +556,11 @@ public TimeValue assignLogicalExecutionTime() {

/** Inner class representing a runtime instance. */
public class Runtime {
public TimeValue deadline = TimeValue.MAX_VALUE;
public Runtime dominating = null;
public TimeValue deadline;
public Runtime dominating;
/** ID ranging from 0 to parent.getTotalWidth() - 1. */
public int id = 0;
public int level = 0;
public int id;
public int level;

public ReactionInstance getReaction() {
return ReactionInstance.this;
Expand All @@ -551,5 +577,16 @@ public String toString() {
result += ")";
return result;
}

public Runtime() {
this.dominating = null;
this.id = 0;
this.level = 0;
if (ReactionInstance.this.declaredDeadline != null) {
this.deadline = ReactionInstance.this.declaredDeadline.maxDelay;
} else {
this.deadline = TimeValue.MAX_VALUE;
}
}
}
}
51 changes: 50 additions & 1 deletion org.lflang/src/org/lflang/generator/ReactionInstanceGraph.java
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,16 @@ public void rebuild() {
// throw new InvalidSourceException("Reactions form a cycle!");
}
}
/**
* This function rebuilds the graph and propagates and assigns deadlines
* to all reactions.
*/
public void rebuildAndAssignDeadlines() {
this.clear();
addNodesAndEdges(main);
assignInferredDeadlines();
this.clear();
}

/*
* Get an array of non-negative integers representing the number of reactions
Expand Down Expand Up @@ -298,7 +308,46 @@ private void assignLevels() {
adjustNumReactionsPerLevel(origin.level, 1);
}
}


/**
* This function assigns inferred deadlines to all the reactions in the graph.
* It is modeled after `assignLevels` but it starts at the leaf nodes and uses
* Kahns algorithm to build a reverse topologically sorted graph
*
*/
private void assignInferredDeadlines() {
List<ReactionInstance.Runtime> start = new ArrayList<>(leafNodes());

// All leaf nodes have deadline initialized to their declared deadline or MAX_VALUE
while (!start.isEmpty()) {
Runtime origin = start.remove(0);
Set<Runtime> toRemove = new LinkedHashSet<>();
Set<Runtime> upstreamAdjacentNodes = getUpstreamAdjacentNodes(origin);

// Visit effect nodes.
for (Runtime upstream : upstreamAdjacentNodes) {
// Stage edge between origin and upstream for removal.
toRemove.add(upstream);

// Update deadline of upstream node if origins deadline is earlier.
if (origin.deadline.isEarlierThan(upstream.deadline)) {
upstream.deadline = origin.deadline;
}
}
// Remove visited edges.
for (Runtime upstream : toRemove) {
removeEdge(origin, upstream);
// If the upstream node has no more outgoing edges,
// then move it in the start set.
if (getDownstreamAdjacentNodes(upstream).size() == 0) {
start.add(upstream);
}
}

// Remove visited origin.
removeNode(origin);
}
}

/**
* Adjust {@link #numReactionsPerLevel} at index <code>level<code> by
Expand Down
16 changes: 16 additions & 0 deletions org.lflang/src/org/lflang/generator/ReactorInstance.java
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,22 @@ public ReactionInstanceGraph assignLevels() {
}
return cachedReactionLoopGraph;
}

/**
* This function assigns/propagates deadlines through the Reaction Instance Graph.
* It performs Kahn`s algorithm in reverse, starting from the leaf nodes and
* propagates deadlines upstream. To reduce cost, it should only be invoked when
erlingrj marked this conversation as resolved.
Show resolved Hide resolved
* there are user-specified deadlines in the program.
* @return
*/
public ReactionInstanceGraph assignDeadlines() {
if (depth != 0) return root().assignDeadlines();
if (cachedReactionLoopGraph == null) {
cachedReactionLoopGraph = new ReactionInstanceGraph(this);
}
cachedReactionLoopGraph.rebuildAndAssignDeadlines();
return cachedReactionLoopGraph;
}

/**
* Return the instance of a child rector created by the specified
Expand Down
3 changes: 3 additions & 0 deletions org.lflang/src/org/lflang/generator/c/CGenerator.java
Original file line number Diff line number Diff line change
Expand Up @@ -2571,6 +2571,9 @@ private void createMainReactorInstance() {
errorReporter.reportError("Main reactor has causality cycles. Skipping code generation.");
return;
}
if (hasDeadlines) {
this.main.assignDeadlines();
}
erlingrj marked this conversation as resolved.
Show resolved Hide resolved
// Inform the run-time of the breadth/parallelism of the reaction graph
var breadth = reactionInstanceGraph.getBreadth();
if (breadth == 0) {
Expand Down
90 changes: 68 additions & 22 deletions org.lflang/src/org/lflang/generator/c/CTriggerObjectsGenerator.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.lflang.ASTUtils;
import org.lflang.AttributeUtils;
import org.lflang.TargetConfig;
import org.lflang.TimeValue;
import org.lflang.TargetProperty.CoordinationType;
import org.lflang.TargetProperty.LogLevel;
import org.lflang.federated.CGeneratorExtension;
Expand Down Expand Up @@ -342,20 +343,41 @@ private static boolean setReactionPriorities(
) {
var foundOne = false;
// Force calculation of levels if it has not been done.
reactor.assignLevels();
// FIXME: Comment out this as I think it is redundant.
// If it is NOT redundant then deadline propagation is not correct
// reactor.assignLevels();

// We handle four different scenarios
// 1) A reactionInstance has 1 level and 1 deadline
// 2) A reactionInstance has 1 level but multiple deadlines
// 3) A reaction instance has multiple levels but all have the same deadline
// 4) Multiple deadlines and levels

// If any reaction has multiple levels, then we need to create
// an array with the levels here, before entering the iteration over banks.
var prolog = new CodeBuilder();
var epilog = new CodeBuilder();
for (ReactionInstance r : reactor.reactions) {
if (currentFederate.contains(r.getDefinition())) {
var levels = r.getLevels();
if (levels.size() != 1) {
var levelSet = r.getLevels();
var deadlineSet = r.getInferredDeadlines();

if (levelSet.size() > 1 || deadlineSet.size() > 1) {
// Scenario 2-4
if (prolog.length() == 0) {
prolog.startScopedBlock();
epilog.endScopedBlock();
}
}
if (deadlineSet.size() > 1) {
// Scenario (2) or (4)
var deadlines = r.getInferredDeadlinesList().stream()
.map(elt -> ("0x" + Long.toString(elt.toNanoSeconds(), 16) + "LL"))
.collect(Collectors.toList());

prolog.pr("interval_t "+r.uniqueID()+"_inferred_deadlines[] = { "+joinObjects(deadlines, ", ")+" };");
}

if (levelSet.size() > 1) {
// Scenario (3) or (4)
// Cannot use the above set of levels because it is a set, not a list.
prolog.pr("int "+r.uniqueID()+"_levels[] = { "+joinObjects(r.getLevelsList(), ", ")+" };");
}
Expand All @@ -368,33 +390,57 @@ private static boolean setReactionPriorities(
for (ReactionInstance r : reactor.reactions) {
if (currentFederate.contains(r.getDefinition())) {
foundOne = true;
// The most common case is that all runtime instances of the
// reaction have the same level, so deal with that case
// specially.
var levels = r.getLevels();
if (levels.size() == 1) {
var level = -1;
for (Integer l : levels) {
level = l;
}
var levelSet = r.getLevels();
var deadlineSet = r.getInferredDeadlines();

// Get the head of the associated lists. To avoid duplication in
// several of the following cases
var level = r.getLevelsList().get(0);
var inferredDeadline = r.getInferredDeadlinesList().get(0);
var runtimeIdx =CUtil.runtimeIndex(r.getParent());

if (levelSet.size() == 1 && deadlineSet.size() == 1) {
// Scenario (1)

// xtend doesn't support bitwise operators...
var indexValue = r.deadline.toNanoSeconds() << 16 | level;
var reactionIndex = "0x" + Long.toString(indexValue, 16) + "LL";
var indexValue = inferredDeadline.toNanoSeconds() << 16 | level;

var reactionIndex = "0x" + Long.toUnsignedString(indexValue, 16) + "LL";

temp.pr(String.join("\n",
CUtil.reactionRef(r)+".chain_id = "+r.chainID+";",
"// index is the OR of level "+level+" and ",
"// deadline "+r.deadline.toNanoSeconds()+" shifted left 16 bits.",
"// deadline "+ inferredDeadline.toNanoSeconds()+" shifted left 16 bits.",
CUtil.reactionRef(r)+".index = "+reactionIndex+";"
));
} else {
var reactionDeadline = "0x" + Long.toString(r.deadline.toNanoSeconds(), 16) + "LL";
} else if (levelSet.size() == 1 && deadlineSet.size() > 1) {
// Scenario 2
temp.pr(String.join("\n",
CUtil.reactionRef(r)+".chain_id = "+r.chainID+";",
"// index is the OR of levels["+runtimeIdx+"] and ",
"// deadlines["+runtimeIdx+"] shifted left 16 bits.",
CUtil.reactionRef(r)+".index = ("+r.uniqueID()+"_inferred_deadlines["+runtimeIdx+"] << 16) | " +
level+";"
));

} else if (levelSet.size() > 1 && deadlineSet.size() == 1) {
// Scenarion (3)
temp.pr(String.join("\n",
CUtil.reactionRef(r)+".chain_id = "+r.chainID+";",
"// index is the OR of levels["+runtimeIdx+"] and ",
"// deadlines["+runtimeIdx+"] shifted left 16 bits.",
CUtil.reactionRef(r)+".index = ("+inferredDeadline.toNanoSeconds()+" << 16) | " +
r.uniqueID()+"_levels["+runtimeIdx+"];"
));

} else if (levelSet.size() > 1 && deadlineSet.size() > 1) {
// Scenario (4)
temp.pr(String.join("\n",
CUtil.reactionRef(r)+".chain_id = "+r.chainID+";",
"// index is the OR of levels["+CUtil.runtimeIndex(r.getParent())+"] and ",
"// deadline "+r.deadline.toNanoSeconds()+" shifted left 16 bits.",
CUtil.reactionRef(r)+".index = ("+reactionDeadline+" << 16) | "+r.uniqueID()+"_levels["+CUtil.runtimeIndex(r.getParent())+"];"
"// index is the OR of levels["+runtimeIdx+"] and ",
"// deadlines["+runtimeIdx+"] shifted left 16 bits.",
CUtil.reactionRef(r)+".index = ("+r.uniqueID()+"_inferred_deadlines["+runtimeIdx+"] << 16) | " +
r.uniqueID()+"_levels["+runtimeIdx+"];"
));
}
}
Expand Down
31 changes: 31 additions & 0 deletions test/C/src/DeadlineInherited.lf
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Test to verify that deadline priority are inherited
target C {
timeout: 1 sec,
threading: false
}

preamble {= int global_cnt = 0; =}

reactor NoDeadline {
timer t(0 msec, 100 msec)

reaction(t) {= global_cnt++; =}
}

reactor WithDeadline {
timer t(0 msec, 100 msec)

reaction(t) {= =}

reaction(t) {=
if (global_cnt != 0) {
lf_print_error_and_exit("Deadline reaction was not executed first. cnt=%i", global_cnt);
}
global_cnt--;
=} deadline(100 sec) {= =}
}

main reactor {
a = new NoDeadline()
b = new WithDeadline()
}
Loading