Cache and reuse dynamic choice lists #619
Conversation
lognaturel
force-pushed
the
cache-choices
branch
4 times, most recently
from
c544a2b to
2016964
Compare
Codecov Report
@@ Coverage Diff @@
## master #619 +/- ##
=========================================
Coverage 55.19% 55.19%
- Complexity 3261 3269 +8
=========================================
Files 246 246
Lines 13396 13388 -8
Branches 2573 2572 -1
=========================================
- Hits 7394 7390 -4
- Misses 5198 5201 +3
+ Partials 804 797 -7
Continue to review full report at Codecov.
|
FormEntryPrompts are used ephemerally so in practice this generally has no effect. It's a weird division of responsibility, anyway.
This gives ItemsetBinding the responsibility of figuring out whether its cached list is valid or needs to be recomputed so client code doesn't need to know about that process.
The contextualized trigger references will be different for different repeat instances so the current question is accounted for in that way.
lognaturel
force-pushed
the
cache-choices
branch
from
2016964 to
3f23844
Compare
src/test/java/org/javarosa/core/model/DynamicSelectUpdateTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/javarosa/core/model/DynamicSelectUpdateTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/javarosa/core/model/DynamicSelectUpdateTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/javarosa/core/model/DynamicSelectUpdateTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/javarosa/core/model/DynamicSelectUpdateTest.java
Outdated
Show resolved
Hide resolved
lognaturel
force-pushed
the
cache-choices
branch
from
f361264 to
5a644aa
Compare
|
I ran
I think the last thing pending is for @dcbriccetti to flag areas he's not confident about so we can compare notes and see whether there are additional areas of risk I might have missed. |
src/test/java/org/javarosa/core/model/DynamicSelectUpdateTest.java
Outdated
Show resolved
Hide resolved
So that forms make sense when rendered by a client and filled by hand. This makes no difference for the tests.
|
Thanks so much for all the great feedback and checks! |
| @@ -318,7 +292,8 @@ public String getHelpText() { | |||
|
|
|||
|
|
|||
| /** | |||
| * Attempts to return the specified Item (from a select or 1select) text. | |||
| * Attempts to return the specified item text from a STATIC select or select1. Does NOT | |||
| * support dynamic selects ({@see ItemsetBinding}). | |||
There was a problem hiding this comment.
This comment suggests I thought this was not applicable to dynamic selects. However, it's now crashing for dynamic selects so something is not right: #625 (comment)
There was a problem hiding this comment.
Ah. There's Collect code that directly calls getSelectItemText regardless of whether the select is static or dynamic. It seems that used to work and doesn't anymore.
| //will no longer be an option this go around | ||
| if(choice != null) | ||
| { | ||
| selection.add(choice.selection()); |
There was a problem hiding this comment.
I think this used to ensure there was always a SelectionChoice for the Selection.
Speeds up displaying previously-filtered choice lists.
Closes getodk/collect#4363
What has been done to verify that this works as intended?
Added and ran tests, tried it in Collect.
Why is this the best possible solution? Were any other approaches considered?
We've talked about many possible performance improvements in this area. Most would only work in some cases. Adding caching works for any choice filter expression and gives us O(1) performance in many cases. Initially computing a choice list is O(N) and can still be slow but once it's computed, we don't have to do it again. I think this is a relatively low-risk, high impact change that we want to do no matter what other improvements we make later.
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?
The intent is just to speed up displaying filtered choice lists that were previously filtered and didn't change. There's risk around all things dynamic choice lists.
I initially thought that this would increase memory usage but I believe that it makes no difference because a choice list was always saved in
ItemsetBindingpreviously, it just was immediately discarded.Do we need any specific form for testing your changes? If so, please attach one.
See tests.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.