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

Workaround needed for stale jr:choice-name() values #3870

Closed
abbyad opened this issue Sep 5, 2017 · 10 comments
Closed

Workaround needed for stale jr:choice-name() values #3870

abbyad opened this issue Sep 5, 2017 · 10 comments
Labels
Priority: 1 - High Blocks the next release. Type: Performance Make something faster

Comments

@abbyad
Copy link
Contributor

abbyad commented Sep 5, 2017

The fix for #3281 has been revoked due to the performance issues seen in #3837. This is a blocker for 2.13 since existing projects are relying on this feature. For instance, app and contact forms in Standard config use of this feature to construct a place name or SMS message.

A theoretical workaround described in #3837 (comment) is to use jr:itext() instead, but using that function in a calculate gives this error:
Error loading form Error: ["FormLogicError: Could not evaluate: jr:itext( '/delivery/group_note/default_chw_sms/anc_only_home_sba_birth:label' ), message: Failed to execute 'evaluate' on 'Document': The string 'jr:itext(\"/delivery/group_note/default_chw_sms/anc_only_home_sba_birth:label\")' is not a valid XPath expression."]

We should figure out if it is possible to fix to use jr:itext(), or find another method.

@abbyad
Copy link
Contributor Author

abbyad commented Sep 7, 2017

Adding to this issue another form. It is a stripped down version of the delivery form that uses jr:itext() in a calculate attribute... and it works in ODK Collect. It seems therefore that this usage should be supported in Enketo as well. Doing so would allow us to continue using XLSForms to create and use these generated choice labels while managing translations like we do for all other fields.

In the test form, you can see that the following are equivalent and work in Collect:
jr:choice-name(${default_chw_sms},'${default_chw_sms}')
jr:itext(concat('/issue-3870/group_note/default_chw_sms/', concat(${default_chw_sms}, ':label')))

@alxndrsn
Copy link
Contributor

alxndrsn commented Sep 8, 2017

The fix for #3281 has been revoked due to the performance issues seen in #3837.

This has not happened.

@abbyad
Copy link
Contributor Author

abbyad commented Sep 8, 2017

Ah, ok... I understood that the patch was removed. Either way, we can't count on it due to performance issue, right?

@alxndrsn
Copy link
Contributor

alxndrsn commented Sep 8, 2017

We should figure out if it is possible to fix to use jr:itext(), or find another method.

Enketo doesn't treat itext() as an XPath expression
getodk/xforms-spec#125 (comment)

My understanding is that currently enketo handles jr:itext() when the form is first built, which means that it's not supported as a runtime xpath expression. Ref enketo/enketo#727

We could add support for this in our enketo/javarosa xpath support, but I'd like to understand the impact that would have on performance, and if possible get the change in the upstream enketo-core instead of us implementing different behaviour.


As I demoed yesterday, we can avoid jr:choice-name() already using an external instance. If that's acceptable then we don't need to change the behaviour of jr:itext().

@alxndrsn
Copy link
Contributor

alxndrsn commented Sep 8, 2017

I understood that the patch was removed.

I understood that, and tried to be more explicit on the other ticket: #3837 (comment)

Either way, we can't count on it due to performance issue, right?

Agreed!

@abbyad
Copy link
Contributor Author

abbyad commented Sep 8, 2017

I'd like to understand the impact that would have on performance, and if possible get the change in the upstream enketo-core instead of us implementing different behaviour.

That makes sense!

As I demoed yesterday, we can avoid jr:choice-name() already using an external instance. If that's acceptable then we don't need to change the behaviour of jr:itext().

Unfortunately, I can't see a good way to create a secondary instance from a XLSForm, nor how to manage the translations like that.

alxndrsn added a commit that referenced this issue Sep 15, 2017
Attempt to work around jr:choice-name() performance issues by reclalculating
fields one more time on form load, and on every page change.  If choice-name()s
only reference fields on different pages, or vice versa, then maybe they will
become usable.

Issue: #3870
alxndrsn added a commit that referenced this issue Sep 15, 2017
Attempt to work around jr:choice-name() performance issues by reclalculating
fields one more time on form load, and on every page change.  If choice-name()s
only reference fields on different pages, or vice versa, then maybe they will
become usable.

Issue: #3870
@garethbowen garethbowen self-assigned this Sep 18, 2017
@garethbowen garethbowen added the Type: Performance Make something faster label Sep 18, 2017
garethbowen pushed a commit that referenced this issue Sep 19, 2017
Attempt to work around jr:choice-name() performance issues by reclalculating
fields one more time on form load, and on every page change.  If choice-name()s
only reference fields on different pages, or vice versa, then maybe they will
become usable.

Issue: #3870
@abbyad
Copy link
Contributor Author

abbyad commented Sep 20, 2017

From tests on master it looks like the approach to do the recalculations on page turns (as is done in Collect) resolves both our problems.

Forms seems to load quickly, in a 1-3s range instead of 7-12+ seconds. We should test this further once released to beta.

Also, there are no unexpected stale values using jr:choice-name. Calcs can be stale within a single page, but wont be when moving to the next page, which is acceptable for our use. This test form is uploaded to alpha.dev shows the jr:choice-name recalculations on page turns:

<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:orx="http://openrosa.org/xforms" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
  <h:head>
    <h:title>Test Issue 3870</h:title>
    <model>
      <instance>
        <issue-3870-recalc-on-page-load delimiter="#" id="issue:3870:recalc:on:page:load" prefix="J1!issue:3870:recalc:on:page:load!" version="2017-09-19">
          <page1>
            <input>Default Value</input>
            <translator>clinic</translator><output/><debug/>
            <note/>
          </page1>
          <page2>
            <debug/>
            <note/>
          </page2>
          <meta>
            <instanceID/>
          </meta>
        </issue-3870-recalc-on-page-load>
      </instance>
      <instance id="contact-summary"/>
      <bind nodeset="/issue-3870-recalc-on-page-load/page1/input" type="string"/>
      <bind nodeset="/issue-3870-recalc-on-page-load/page1/translator" type="select1"/>
      <bind calculate="jr:choice-name( /issue-3870-recalc-on-page-load/page1/translator ,' /issue-3870-recalc-on-page-load/page1/translator ')" nodeset="/issue-3870-recalc-on-page-load/page1/output" type="string"/>
      <bind nodeset="/issue-3870-recalc-on-page-load/page1/debug" readonly="true()" type="string"/>
      <bind nodeset="/issue-3870-recalc-on-page-load/page1/note" readonly="true()" type="string"/>
      <bind nodeset="/issue-3870-recalc-on-page-load/page2/debug" readonly="true()" type="string"/>
      <bind nodeset="/issue-3870-recalc-on-page-load/page2/note" readonly="true()" type="string"/>
      <bind calculate="concat('uuid:', uuid())" nodeset="/issue-3870-recalc-on-page-load/meta/instanceID" readonly="true()" type="string"/>
    </model>
  </h:head>
  <h:body class="pages">
    <group appearance="field-list" ref="/issue-3870-recalc-on-page-load/page1">
      <label>Page 1</label>
      <input ref="/issue-3870-recalc-on-page-load/page1/input">
        <label>Input</label>
      </input>
      <select1 ref="/issue-3870-recalc-on-page-load/page1/translator">
        <label>Translator</label>
        <item>
          <label>[<output value=" /issue-3870-recalc-on-page-load/page1/input "/>] District</label>
          <value>district_hospital</value>
        </item>
        <item>
          <label>[<output value=" /issue-3870-recalc-on-page-load/page1/input "/>] Health Center</label>
          <value>health_center</value>
        </item>
        <item>
          <label>[<output value=" /issue-3870-recalc-on-page-load/page1/input "/>] Area</label>
          <value>clinic</value>
        </item>
      </select1>
      <input ref="/issue-3870-recalc-on-page-load/page1/debug">
        <label>jr:choice-name($ {translator},'$ {translator}') = **<output value=" /issue-3870-recalc-on-page-load/page1/output "/>**</label>
      </input>
      <input ref="/issue-3870-recalc-on-page-load/page1/note">
        <label>The jr:choice-name value shown above is stale after losing focus on a modified _Input_ field. The _Translator_ options are updated, but trying to use jr:choice-name to get those labels does not work.

The new strategy is to make jr:choice-name valid on initial load and page turns. See what happens when you go to the next page.</label>
      </input>
    </group>
    <group appearance="field-list" ref="/issue-3870-recalc-on-page-load/page2">
      <label>Page 2</label>
      <input ref="/issue-3870-recalc-on-page-load/page2/debug">
        <label>jr:choice-name($ {translator},'$ {translator}') = **<output value=" /issue-3870-recalc-on-page-load/page1/output "/>**</label>
      </input>
      <input ref="/issue-3870-recalc-on-page-load/page2/note">
        <label>The value above should now be correct because it was calculated when changing pages. Going back to the previous page will also show the correct value.</label>
      </input>
    </group>
  </h:body>
</h:html>

garethbowen pushed a commit that referenced this issue Sep 20, 2017
Attempt to work around jr:choice-name() performance issues by reclalculating
fields one more time on form load, and on every page change.  If choice-name()s
only reference fields on different pages, or vice versa, then maybe they will
become usable.

Issue: #3870
@garethbowen
Copy link
Contributor

Merged and backported to 2.13.x. This backport includes upgrading enketo to the latest so it'll need extensive regression testing around forms.

@abbyad
Copy link
Contributor Author

abbyad commented Sep 20, 2017

This is now incredibly fast in 2.13.0-beta.17. The Delivery form that was taking 7+ seconds is now loaded in <0.5s as an offline restricted user. Moving to Ready, and we can open up new issues for any Enketo regressions.

Also, it is worth restating that the workaround for stale jr:choice-name() values is to only recalculate values when showing a new page.

Forms must be designed accordingly because values with nested calculations on a single page could be stale. cc @ngamita @derickl

@alxndrsn
Copy link
Contributor

The Delivery form that was taking 7+ seconds is now loaded in <0.5s as an offline restricted user.

🎉 !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1 - High Blocks the next release. Type: Performance Make something faster
Projects
None yet
Development

No branches or pull requests

4 participants