Use FilterStrategy optimizations for select choice filters

[seadowg]

Closes getodk/collect#5694

The core change here is to move the state around building the base FilterStrategy chain to FormDef up from TriggerableDag to allow it to be shared by ItemsetBinding (where choice filters are evaluated).

What has been done to verify that this works as intended?

New tests.

Why is this the best possible solution? Were any other approaches considered?

It does feel to me like FormDef is the wrong place to own both the base EvaluationContext and the TriggerableDag instances. In my mind, FormDef should really be a "data" object that's output by the "parsing" part of JavaRosa and that EvaluationContext and TriggerableDag should probably be owned by something in the "runtime" world (FormEntryController for instance). I had a quick think through detaching these, but it would be massive and not something I feel like should distract from the user facing improvements here.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Should just speed things up! Obvious things to think about are how the FilterStrategy instances that are now used during choice filter evaluations could cause problems.

[lognaturel]

This is great! I hadn't thought about the EvaluationContext "owning" the caches extending to this case but that makes a lot of sense. I was initially skeptical of removing the tests you did but I think it's ok. I believe those form shapes are covered elsewhere and the caching no longer applies to those specifically (because they use functions).

This does reduce the number of cases in which caching will come into play. There for sure won't be any with expressions that use functions like the test forms with starts-with. I think it also may affect the level of caching with selects in repeats but I haven't had a chance to think through that fully yet.

As I was understanding and verifying this, I came up with a handful of small commits you may want to take: seadowg/javarosa@select-caching...lognaturel:javarosa:select-caching-play

I'll think through repeat cases a bit more but I think this is likely ready to merge and exercise in Collect.

[lognaturel]

This removes caching for cases where the choice filter compares instance values against:

• relative reference in repeat (second removed test)
• result of a function call (both removed tests)
• expressions other than equality and comparison

Hopefully that's exhaustive.

I think the first two can be handled by relaxing the constraints on the index-based cache. I don't think there needs to be any restriction at all. We can file an issue for that and handle it separately but I do think they should be addressed before release.

The third I think is acceptable.

Having spent a little more time with this, I believe repeats aren't affected beyond the issue with relative references above. These caching strategies don't use the reference for the side of the comparison that is in the main instance. Instead, those references will be evaluated according to the current context and it's the resulting value that's used to look things up in the appropriate cache.

Even though it's not really relevant to the current implementation, I think it would be helpful to add something like

   @Test
    public void eqChoiceFilter_inRepeat_onlyEvaluatedOnce() throws Exception {
        Scenario scenario = Scenario.init("Select in repeat", html(
            head(
                title("Select in repeat"),
                model(
                    mainInstance(
                        t("data id='repeat-select'",
                            t("filter"),
                            t("repeat",
                                t("select")))),

                    instance("choices",
                        item("a", "A"),
                        item("aa", "AA"),
                        item("b", "B"),
                        item("bb", "BB")))),
            body(
                input("filter"),
                repeat("/data/repeat",
                    select1Dynamic("/data/repeat/select", "instance('choices')/root/item[value=/data/filter]"))
            )));

        int evaluations = Measure.withMeasure(asList("PredicateEvaluation", "IndexEvaluation"), () -> {
            scenario.answer("/data/filter", "a");

            scenario.choicesOf("/data/repeat[0]/select");

            scenario.createNewRepeat("/data/repeat");
            scenario.choicesOf("/data/repeat[1]/select");
        });

        // Check that we do less than (size of secondary instance) * (number of choice lookups)
        assertThat(evaluations, lessThan(8));
    }

[seadowg]

@lognaturel there's a lot in flight at the moment, so I think it's going to be easier to merge this as-is and submit your changes as a PR so I can review when I have time.