Indirect reference to repeat count doesn't update as repeat instances are added

[lognaturel]

On v2.17.0, if there's a calculate with a call on count(repeat) outside of that repeat and then a reference to that calculate inside the repeat, the reference inside the repeat is updated as the repeat count changes when repeat instances are added or removed. See master...lognaturel:v2.17.0-with-testing#diff-4c309f85e6172ad03a5c4590307ea6b4R105 for a branch that runs that test on v2.17.0. The same test fails on master.

CC @ggalmazor would be great to get your take on this when you have some cycles.

[lognaturel]

I did quick debugging sessions on v2.17.0 vs master and got to the call on expandReference in evaluateTriggerable. On master, that expansion never results in more than one node. On v2.17.0, it expands to all repeat instances which is what we want. d7593de is the first bad commit according to bisect.

[ggalmazor]

Hi @lognaturel! The master...lognaturel:v2.17.0-with-testingdiff-4c309f85e6172ad03a5c4590307ea6b4R105 link doesn't show anything... Did you rebase?

It would be great if you can provide or point me in the right direction to a form that reproduces the issue. I could take a look at this during the weekend.

[lognaturel]

Thanks and sorry! master...lognaturel:v2.17.0-with-testing. The test is in Safe2014DagImplTest.java. I'll be looking at it this weekend too so maybe we can touch base some time. It looks like that firedAnchors map was doing something useful after all.

I did a little more exploration yesterday here are some things I've found interesting:

• If I replace the inner-count calculate with count(/data/repeat), everything works as expected. So there's something about the indirection introduced that makes things different.
• In doEvaluateTriggerables, if I change what's passed to evaluateTriggerable to anchorRef.genericize() from anchorRef, naturally, the tests that register DAG events fail. All tests get expected values except that relative_paths_in_nested_repeats and validation_only_triggers_calculations_involving_user_visible_fields end up with better behavior. I need to check what those look like on v2.17.0.

[ggalmazor]

Gosh, this is hard! Starting to look at this now. The good news (I guess) is that we can always bring back the old firedAnchors map, right?

[ggalmazor]

OK, in master I'm getting this in the first set of assertions:

The assertion is:
        range(0, 5).forEach(n -> assertThat(scenario.answerOf("/data/repeat[" + n + "]/inner-count"), is(intAnswer(5))));

java.lang.AssertionError: 
Expected: is answer with value 5
     but: was answer 1(<1>)

Are we on the same page?

[ggalmazor]

Changing the assertion to:

range(0, 5).forEach(n -> assertThat(scenario.answerOf("/data/repeat[" + n + "]/inner-count"), is(intAnswer(n+1))));

(Notice the n+1)

Makes it pass, which means that the inner-count calculate is being executed once each time a new repeat is created. The problem is that previous repeats aren't recalculated.

I'd swear we explored this issue before...

[ggalmazor]

How would this be different than the relative_paths_in_nested_repeats test? or validation_only_triggers_calculations_involving_user_visible_fields

[ggalmazor]

Well, it turns out that reverting the firedAnchor changes does solve the test but relative_paths_in_nested_repeats, and validation_only_triggers_calculations_involving_user_visible_fields still present the "buggy" behavior.

See: https://github.com/ggalmazor/javarosa/commit/eb8ade55ea635a31f5045f8b5fe17e8a974d3a6c

[lognaturel]

I mentioned above that on master, changing the inner calculate to count(/data/repeat) produced the expected results. It turns out that's only true if there's also a count calculation outside the repeat. That is, in my original test, using the following binds works:

bind("/data/count").type("int").calculate("count(/data/repeat)"),
bind("/data/repeat/inner-count").type("int").calculate("count(/data/repeat)"))

However, this does not:

bind("/data/count").type("int").calculate("4*4"),
bind("/data/repeat/inner-count").type("int").calculate("count(/data/repeat)"))

It looks like in the first case the reference to /data/repeat inside the repeat is used as a generic reference but in the second case it's contextualized based on the repeat instance. This is pretty terrible stuff, I think. I believe what's going on is that in the first case the result for the expression count(/data/repeat) is cached. I vaguely remember something about this kind of caching happening but I don't remember where.

If I use ../../repeat I get yet different behavior. In that case the count does update as repeats get added but only for the current repeat instances, not for all of them.

I'm keeping master...lognaturel:issue-584 up to date with interesting tests.

[lognaturel]

Edit: always considering the generic ref results in a huge performance regression:

before	ChildVaccinationBenchmark.run_1_times  thrpt   25  1.927 ± 0.068  ops/s
after	ChildVaccinationBenchmark.run_1_times  thrpt   25  0.042 ±  0.003  ops/s

When @ggalmazor and I touched base earlier, we agreed I'd pursue always considering the generic reference and he'd pursue bringing back firedAnchors and trying to refactor it so it makes more sense. I think the generic ref approach is guaranteed to give correct results but at a significant performance cost. So over to you, @ggalmazor, and I'll take a look at what you come up with during my day tomorrow.

===

As I noted above, replacing anchorRef with anchorRef.genericize() leads to most of the desired behavior. I touched base with @ggalmazor about this approach and our concern is performance since that means every repeat instance would always be considered. Here's a "test" (no assertion) that gets at what effect this has. In this case, the only calculation in the repeat is totally independent of the repeat reference. What we don't want happening is that calculation being re-triggered for every single instance because it could be expensive. My understanding from the DAG event output is that they're not recomputed because there's no reference to the repeat.

    @Test
    public void onRepeatAdd_calculationsInRepeat_withoutReferenceToRepeat_areOnlyCalculatedForNewInstance() throws IOException {
        Scenario scenario = Scenario.init("Count outside repeat used inside", html(
            head(
                title("Count outside repeat used inside"),
                model(
                    mainInstance(t("data id=\"outside-used-inside\"",
                        t("repeat jr:template=\"\"",
                            t("inner-calc")
                    ))),
                    bind("/data/repeat/inner-calc").type("int").calculate("4*4")),
                body(
                    repeat("/data/repeat",
                        input("/data/repeat/inner-calc")
                    )
                )))).onDagEvent(dagEvents::add);

        dagEvents.clear();

        range(0, 5).forEach(n -> {
            scenario.next();
            scenario.createNewRepeat();
            assertThat(scenario.answerOf("/data/repeat[" + n + "]/inner-calc"), CoreMatchers.is(intAnswer(16)));
            scenario.next();
        });

        range(0, 5).forEach(n -> assertThat(scenario.answerOf("/data/repeat[" + n + "]/inner-calc"), CoreMatchers.is(intAnswer(16))));

        dagEvents.forEach(event -> System.out.println(event.getDisplayMessage()));
    }

I also replaced all the logic in deleteRepeatGroup with:

        Set<QuickTriggerable> alreadyEvaluated = triggerTriggerables(mainInstance, evalContext, deleteRef, new HashSet<>());
        evaluateChildrenTriggerables(mainInstance, evalContext, deletedElement, false, alreadyEvaluated);

and confirmed that all the delete-based tests get the expected results (with different DAG events). deleteRepeatGroupWithCalculationsTimingTest gives me speeds in the same order of magnitude (twice as fast reproducibly but who knows how meaningful that is).

[lognaturel]

I've spent some more time with the original firedAnchors code and it still doesn't make sense to me. I see that anchorRefs for evaluateTriggerable will either be the original changed ref (anchorRef) or the set of repeat instances. I think this is what causes every repeat to be updated, not the call on expandRef which never expands anything. I think basically what this code ends up doing is identifying triggerable cascades with some triggerables inside of a repeat that refer to nodes outside the repeat. There are lots of things about the design of the solution that I can’t explain and I’m starting to wonder whether it might almost have accidentally worked for some cases.

The reason always genericizing the anchorRef is a bad idea is that many (most?) repeats only have references to nodes within that same repeat and in that case, adding a new repeat instance doesn’t need to touch other repeat instances. As far as I’ve figured, here are the cases of triggerables that would need to be evaluated for every repeat instance when an instance is added or deleted:

• One of the triggerables earlier in the cascade that led to this triggerable is outside the repeat. This is illustrated by the test that I first referred to: a count is computed outside the repeat and then referenced inside the repeat. When the count outside the repeat updates, every repeat instance needs to also get that update. This is also related to the deletion performance improvement we made in Reduce access to DAG methods and speed up repeat deletion when there are dependent expressions outside the repeat  #583.
• This triggerable is inside the repeat and is immediately triggered by instance addition or deletion. This would be something like a call on position or count inside the repeat.
• This triggerable’s expression references some node in the repeat with a predicate. For example, it could access another repeat instance by using a position() expression or filter all nodes with name fields that start with ‘a’.

I believe the firedAnchors approach only handled the first case. I wrote up a quick solution to address the first two: master...lognaturel:issue-584. The third case is trickier because it requires looking at the triggerable expression. Since it has never worked and no one seems to have missed it, I don’t plan to explore it further for now.

I ran the ChildVaccination benchmark and got similar results on master vs on that branch so I think it’s ok performance-wise. It also lets us remove the special implementation for repeat deletion.

I have some more tests I want to write but I wanted to give a quick update.