Skip to content

Commit

Permalink
Avoid hitting the main instance to get the dependant triggerables
Browse files Browse the repository at this point in the history 
Key change in this commit is to add as targets all descendants of a group node with a relevance condition
  • Loading branch information
@ggalmazor
ggalmazor committed Jan 9, 2020
1 parent 303e968 commit 2bd11f6
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 81 deletions.
2 changes: 1 addition & 1 deletion src/main/java/org/javarosa/core/model/FormDef.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ public void finalizeTriggerables() throws IllegalStateException {
// DAGify the triggerables based on dependencies and sort them so that
// triggerables come only after the triggerables they depend on
//
dagImpl.finalizeTriggerables(getMainInstance(), getEvaluationContext());
dagImpl.finalizeTriggerables(getMainInstance());
}

/**
Expand Down
51 changes: 13 additions & 38 deletions src/main/java/org/javarosa/core/model/TriggerableDag.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,8 @@ Triggerable addTriggerable(Triggerable triggerable) {
* will create the appropriate ordering and dependencies to ensure the
* conditions will be evaluated in the appropriate orders.
*/
public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ec) throws IllegalStateException {
List<QuickTriggerable[]> edges = getDagEdges(mainInstance, ec, allTriggerables, triggerablesPerTrigger);
public void finalizeTriggerables(FormInstance mainInstance) throws IllegalStateException {
List<QuickTriggerable[]> edges = getDagEdges(allTriggerables, triggerablesPerTrigger);
triggerablesDAG = buildDag(allTriggerables, edges);
repeatConditionsPerTargets = getRepeatConditionsPerTargets(mainInstance, triggerablesDAG);
}
Expand All @@ -325,10 +325,10 @@ private static Map<TreeReference, QuickTriggerable> getRepeatConditionsPerTarget
* <li>Builds a cache of immediate cascades of each vertex, meaning that we will remember the dependant vertices without having to traverse the DAG again</li>
* </ul>
*/
private static List<QuickTriggerable[]> getDagEdges(FormInstance mainInstance, EvaluationContext evalContext, Set<QuickTriggerable> vertices, Map<TreeReference, Set<QuickTriggerable>> triggerIndex) {
private static List<QuickTriggerable[]> getDagEdges(Set<QuickTriggerable> vertices, Map<TreeReference, Set<QuickTriggerable>> triggerIndex) {
List<QuickTriggerable[]> edges = new ArrayList<>();
for (QuickTriggerable vertex : vertices) {
Set<QuickTriggerable> dependants = getDependantTriggerables(mainInstance, evalContext, vertex, triggerIndex);
Set<QuickTriggerable> dependants = getDependantTriggerables(vertex, triggerIndex);

if (dependants.contains(vertex))
throwCyclesInDagException(dependants);
Expand Down Expand Up @@ -392,41 +392,16 @@ private static void throwCyclesInDagException(Collection<QuickTriggerable> trigg
* Get all of the elements which will need to be evaluated (in order) when
* the triggerable is fired.
*/
private static Set<QuickTriggerable> getDependantTriggerables(FormInstance mainInstance, EvaluationContext ec, QuickTriggerable triggerable, Map<TreeReference, Set<QuickTriggerable>> triggerIndex) {
Set<QuickTriggerable> allDependantTriggerables = new LinkedHashSet<>();
Set<TreeReference> targets = new HashSet<>();
for (TreeReference target : triggerable.getTargets()) {

targets.add(target);

// For certain types of triggerables, the update will affect
// not only the target, but also the children of the target.
// In that case, we want to add all of those nodes
// to the list of updated elements as well.
if (triggerable.isCascadingToChildren()) {
targets.addAll(getChildrenOfReference(mainInstance, ec, target));
}
}

// Now go through each of these updated nodes (generally
// just 1 for a normal calculation,
// multiple nodes if there's a relevance cascade.
for (TreeReference target : targets) {

This comment has been minimized.

Sign in to view

Copy link
@lognaturel

lognaturel Jan 10, 2020

Member

I'm having trouble seeing where this work is accounted for. I think that's what the loop that doesn't do anything is trying to do?

This comment has been minimized.

Sign in to view

Copy link
@ggalmazor

ggalmazor Jan 10, 2020

Author Contributor

This would be the outer loop in the original borked implementation
// Check our index to see if that target is a Trigger
// for other conditions
// IE: if they are an element of a different calculation
// or relevancy calc

// We can't make this reference generic before now or
// we'll lose the target information,
// so we'll be more inclusive than needed and see if any
// of our triggers are keyed on the predicate-less path
// of this ref
Set<QuickTriggerable> dependantTriggerables = triggerIndex.get(target.hasPredicates() ? target.removePredicates() : target);
if (dependantTriggerables != null)
allDependantTriggerables.addAll(dependantTriggerables);
private static Set<QuickTriggerable> getDependantTriggerables(QuickTriggerable triggerable, Map<TreeReference, Set<QuickTriggerable>> triggerIndex) {
Set<QuickTriggerable> result = new HashSet<>();
for (TreeReference targetRef : triggerable.getTargets()) {
Set<QuickTriggerable> children = triggerIndex.getOrDefault(targetRef, emptySet());
result.addAll(children);
for (QuickTriggerable child : children)

This comment has been minimized.

Sign in to view

Copy link
@lognaturel

lognaturel Jan 10, 2020

Member

This whole loop doesn't make sense. All of the children have already been added in the line above so !result.contains(child) can never be true. I've removed all of it and confirmed that all tests still pass.

This comment has been minimized.

Sign in to view

Copy link
@lognaturel

lognaturel Jan 10, 2020

Member

I would expect this loop to be on the same level as the loop over targets rather than nested in it. And I would expect it to match 2bd11f6#diff-3b8933c71e91d3732caa7bbe1013f1c9L425. That said, I don't understand why that original code is not recursive. It seems like all levels of triggerables should be accounted for.

It makes me very nervous that no tests have broken. Do we need a longer dependency chain?

This comment has been minimized.

Sign in to view

Copy link
@ggalmazor

ggalmazor Jan 10, 2020 •
edited
Loading

Author Contributor

Ugh, I required some time to realize this too.

It looks like we need a test that breaks the implementation. The nested loop doesn't make sense at all.

It's fine, and we don't need it to be recursive too. We need to remove the nested loop, though.

I'm not sure what was I thinking while writing that code...

The naming I choose wasn't helping at all to understand what this method did and how it was used. I thought I had my head wrapped around it, but it turns out I didn't fully understand what was going on... my bad!

  • This method is used while computing the DAG edges, so no recursive/all descendants set of triggerables is required. We need just pairs of parent/child vertices, so the algorithm in getDependantTriggerables doesn't need to be recursive.
  • Self-references can be part of the resulting set. They will be detected in getDagEdges, and an exception will be thrown.
  • Longer chains of cycles requiring more than one node will be detected in buildDag
  • Super abstract and only Guillermo understand explanation is: There's no a > b > c situation where we would need to resolve c starting from a, because we study all the triggerables (a, b, and c) and that will get us to c from b

So, summing up, this is what I'll do:

  • Remove the nested loop
  • Improve naming to make it explicit what's the show about.
if (child != triggerable && !result.contains(child))

This comment has been minimized.

Sign in to view

Copy link
@lognaturel

lognaturel Jan 10, 2020

Member

Why is the first condition needed? Wouldn't there be a cycle if a triggerable were in its own child list? I removed it and all the tests still pass so either a test with that case needs to be introduced or it's unnecessary.
result.addAll(getDependantTriggerables(child, triggerIndex));
}
return allDependantTriggerables;
return result;
}

/**
Expand Down
Loading

0 comments on commit 2bd11f6

Please sign in to comment.