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.

Sign up for GitHub

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 cycle checking when calculating min delay from nearest physical action #2459

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix cycle checking when calculating min delay from nearest physical action #2459

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8635b98
Fix cycle checking when calculating min delay from nearest physical a…
lsk567 Jan 21, 2025
5f8973a
Keep tracing backward from input ports. Add test case.
lsk567 Jan 24, 2025
1ec934c
Add a timeout to TriggerLoop.lf
lsk567 Jan 25, 2025
21d9a98
Add a future todo comment
lsk567 Feb 4, 2025
e1c2744
Add multiport to test case
lsk567 Feb 7, 2025
21f4cca
Make sure there are no unconnected inputs
lsk567 Feb 7, 2025
4fc4e4f
Apply spotless
lsk567 Feb 7, 2025
08fb52e
Merge branch 'master' into fix-fed-cycle-check
edwardalee Mar 5, 2025
0402868
Merged master, affecting submodules only
edwardalee Mar 5, 2025
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
95 changes: 78 additions & 17 deletions core/src/main/java/org/lflang/federated/generator/FederateInstance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@
*/
package org.lflang.federated.generator;

import com.google.common.base.Objects;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand All @@ -44,6 +44,7 @@
import org.lflang.generator.PortInstance;
import org.lflang.generator.ReactionInstance;
import org.lflang.generator.ReactorInstance;
import org.lflang.generator.RuntimeRange;
import org.lflang.generator.TriggerInstance;
import org.lflang.lf.Action;
import org.lflang.lf.ActionOrigin;
Expand All @@ -57,6 +58,7 @@
import org.lflang.lf.Output;
import org.lflang.lf.Parameter;
import org.lflang.lf.ParameterReference;
import org.lflang.lf.Port;
import org.lflang.lf.Reaction;
import org.lflang.lf.Reactor;
import org.lflang.lf.ReactorDecl;
Expand Down Expand Up @@ -419,20 +421,20 @@ public boolean includes(Action action) {
// Look in triggers
for (TriggerRef trigger : convertToEmptyListIfNull(react.getTriggers())) {
if (trigger instanceof VarRef triggerAsVarRef) {
if (Objects.equal(triggerAsVarRef.getVariable(), action)) {
if (Objects.equals(triggerAsVarRef.getVariable(), action)) {
return true;
}
}
}
// Look in sources
for (VarRef source : convertToEmptyListIfNull(react.getSources())) {
if (Objects.equal(source.getVariable(), action)) {
if (Objects.equals(source.getVariable(), action)) {
return true;
}
}
// Look in effects
for (VarRef effect : convertToEmptyListIfNull(react.getEffects())) {
if (Objects.equal(effect.getVariable(), action)) {
if (Objects.equals(effect.getVariable(), action)) {
return true;
}
}
Expand Down Expand Up @@ -494,7 +496,7 @@ public boolean includes(Timer timer) {
// Look in triggers
for (TriggerRef trigger : convertToEmptyListIfNull(r.getTriggers())) {
if (trigger instanceof VarRef triggerAsVarRef) {
if (Objects.equal(triggerAsVarRef.getVariable(), timer)) {
if (Objects.equals(triggerAsVarRef.getVariable(), timer)) {
return true;
}
}
Expand Down Expand Up @@ -661,7 +663,7 @@ public LinkedHashMap<Output, TimeValue> findOutputsConnectedToPhysicalActions(
for (PortInstance output : instance.outputs) {
for (ReactionInstance reaction : output.getDependsOnReactions()) {
TimeValue minDelay = findNearestPhysicalActionTrigger(reaction);
if (!Objects.equal(minDelay, TimeValue.MAX_VALUE)) {
if (!Objects.equals(minDelay, TimeValue.MAX_VALUE)) {
physicalActionToOutputMinDelay.put((Output) output.getDefinition(), minDelay);
}
}
Expand All @@ -686,6 +688,25 @@ public String toString() {
* otherwise
*/
public TimeValue findNearestPhysicalActionTrigger(ReactionInstance reaction) {
Set<ReactionInstance> visited = new HashSet<>();
return findNearestPhysicalActionTriggerRecursive(visited, reaction);
}

/**
* Find the nearest (shortest) path to a physical action　trigger from this 'reaction' in terms of
* minimum delay by recursively visiting upstream triggers and reactions until a physical action
* is reached.
*
* @param visited A set of reactions that have been visited used to avoid deep loops
* @param reaction The reaction to start with
* @param pathMinDelay The amount of mininum delays accumulated from the output port to the
* reaction (previous argument)
* @return The minimum delay found to the nearest physical action and TimeValue.MAX_VALUE
* otherwise
*/
private TimeValue findNearestPhysicalActionTriggerRecursive(
Set<ReactionInstance> visited, ReactionInstance reaction) {
visited.add(reaction); // Mark the reaction as visited.
TimeValue minDelay = TimeValue.MAX_VALUE;
for (TriggerInstance<? extends Variable> trigger : reaction.triggers) {
if (trigger.getDefinition() instanceof Action action) {
Expand All @@ -698,24 +719,64 @@ public TimeValue findNearestPhysicalActionTrigger(ReactionInstance reaction) {
// Logical action
// Follow it upstream inside the reactor
for (ReactionInstance uReaction : actionInstance.getDependsOnReactions()) {
// Avoid a loop
if (!Objects.equal(uReaction, reaction)) {
// Avoid a potentially deep loop by checking the visited set.
if (!visited.contains(uReaction)) {
TimeValue uMinDelay =
actionInstance.getMinDelay().add(findNearestPhysicalActionTrigger(uReaction));
actionInstance
.getMinDelay()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't look quite right because this minDelay of this action will get ignored if the uReaction has been previously visited via some other path.

.add(findNearestPhysicalActionTriggerRecursive(visited, uReaction));
if (uMinDelay.isEarlierThan(minDelay)) {
minDelay = uMinDelay;
}
}
}
}

} else if (trigger.getDefinition() instanceof Output) {
// Outputs of contained reactions
PortInstance outputInstance = (PortInstance) trigger;
for (ReactionInstance uReaction : outputInstance.getDependsOnReactions()) {
TimeValue uMinDelay = findNearestPhysicalActionTrigger(uReaction);
if (uMinDelay.isEarlierThan(minDelay)) {
minDelay = uMinDelay;
} else if (trigger.getDefinition() instanceof Port) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
} else if (trigger.getDefinition() instanceof Port) {
} else if (trigger instanceof PortInstance) {

PortInstance port = (PortInstance) trigger;
// Regardless of whether the port is an output or input port of
// a contained reactor, recurse on reactions that write to it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// a contained reactor, recurse on reactions that write to it.
// a containing reactor, recurse on reactions that write to it.

for (ReactionInstance uReaction : port.getDependsOnReactions()) {
// Ports can form a loop also.
if (!visited.contains(uReaction)) {
TimeValue uMinDelay = findNearestPhysicalActionTriggerRecursive(visited, uReaction);
if (uMinDelay.isEarlierThan(minDelay)) {
minDelay = uMinDelay;
}
}
}
// The trigger is an input of a contained reactor. If it
// another upstream contained reactor connects to it, trace to
// the upstream contained reactor's output ports.
if (trigger.getDefinition() instanceof Input) {
var upstreamPorts = port.eventualSources();
for (RuntimeRange<PortInstance> range : upstreamPorts) {
PortInstance upstreamPort = range.instance;

// Get a connection delay value between upstream port and
// the current port.
TimeValue connectionDelay =
upstreamPort.getDependentPorts().stream()
.filter(
sRange ->
sRange.destinations.stream()
.map(it -> it.instance)
.anyMatch(port::equals))
.map(sRange -> ASTUtils.getDelay(sRange.connection.getDelay()))
.filter(Objects::nonNull)
.map(TimeValue::fromNanoSeconds)
.findFirst()
.orElse(TimeValue.ZERO);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a cleaner way to look at an input port (PortInstance) and get a connection delay from an upstream connection into the input port? @edwardalee @erlingrj

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow that code is ugly. It's also not quite correct. E.g., suppose you have hierarchy:

   p1 ----> p2 ---/D/---> p3

this will return TimeValue.ZERO, I think, ignoring D.
Are you trying to get the minimum tag offset along the path from the eventual source to the port?
This is pretty tricky. Something like this was done by @byeonggiljun in CExtensionUtils.java, but there is a telling comment there:

        // The minimum delay calculation needs to be made in the C code because it
        // may depend on parameter values.

Also, if the port is a multiport, there could be multiple delays to it.

Copy link
Collaborator Author

@lsk567 lsk567 Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Are you trying to get the minimum tag offset along the path from the eventual source to the port?

Yes, that is precisely what findNearestPhysicalActionTrigger was designed to do, but it wasn't doing it thoroughly.

Here is the test case I designed. The current master branch would falsely report 10 sec when tracing back from out2.

TriggerLoop

The fix in this PR now correctly identifies that the minimum path delay from out2 to the physical action is 2 sec, not 10 sec:

 --> test/C/src/federated/TriggerLoop.lf:23:3
   |
22 | reactor Hard {
23 |   output out2: int
   |   ^^^^^^^^^^^^^^^^ Found a path from a physical action to output for reactor "Hard".
The amount of delay is 2 sec.
With centralized coordination, this can result in a large number of messages to the RTI.
Consider refactoring the code so that the output does not depend on the physical action,
or consider using decentralized coordination. To silence this warning, set the target
parameter coordination-options with a value like {advance-message-interval: 10 msec}

But it has to do some gymnastics since it seems hard to get the connection delay value from an input port (the in port of the left Delay, of type RuntimeRange.Port). So far I have only found a way to access the delay from SendRange, which requires tracing from the input port to the upstream output port, hence the ugliness.

Perhaps a long-term solution is to refactor RuntimeRange to make accessing connection delays easier.

Something like this was done by @byeonggiljun in CExtensionUtils.java, but there is a telling comment there

If the parameters are not passed in on the command line, which should not be possible now, I think it should be okay?

Also, if the port is a multiport, there could be multiple delays to it.

That's true. I will augment the test case to check for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like this was done by @byeonggiljun in CExtensionUtils.java, but there is a telling comment there

If the parameters are not passed in on the command line, which should not be possible now, I think it should be okay?

It doesn't work with the usual LF parameters, try making the connection delay a parameter, this algorithm will use the initializer of that parameter as the delay always.

This should be solvable in LFC because it doesn't depend on any target code.


for (ReactionInstance uReaction : upstreamPort.getDependsOnReactions()) {
if (!visited.contains(uReaction)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, I'm not sure this should be skipped just because the upstream has been visited. There may be multiple paths to the upstream. I suggest replacing visited with a set that contains only the "in progress" reactions (those for which we are inside a recursive call to find their delay). Then also cache the values computed for each reaction (just for efficiency, not needed for correctness).

TimeValue uMinDelay =
connectionDelay.add(
findNearestPhysicalActionTriggerRecursive(visited, uReaction));
if (uMinDelay.isEarlierThan(minDelay)) {
minDelay = uMinDelay;
}
}
}
}
}
}
Expand Down
48 changes: 48 additions & 0 deletions test/C/src/federated/TriggerLoop.lf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/**
* When there is an output in the federate reactor, the compiler checks if the output is
* (transitively) connected to a physical action inside the same reactor, as the physical action
* could result in generating a large number of NULL messages. As the compiler looks for the
* physical action recursively, it should be able to handle non-trivial "trigger loop" formed by a
* set of actions. This tests check if the compiler can successfully handle programs with such a
* pattern without getting stack overflow.
*/
target C

reactor Simple {
// This output is required to trigger finding the closest upstream physical action inside the reactor.
output out: int
logical action a
logical action b

reaction(a) -> b {= =}

reaction(b) -> a, out {= =}
}

reactor Hard {
output out2: int
@label(
"We want to find this physical action from out2. The compiler should warn about min path delay = 2 sec.")
physical action d(1 sec)
@label("This physical action has a larger min delay. The compiler should ignore it.")
physical action e(10 sec)
delay = new Delay()
delay2 = new Delay()
delay.out -> delay2.in after 1 sec

reaction(d, delay2.out) -> delay.in {= =}

reaction(e, delay2.out) -> d, out2 {= =}
}

reactor Delay {
input in: int
output out: int

reaction(in) -> out {= =}
}

federated reactor {
simple = new Simple()
hard = new Hard()
}
Loading