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

Creating child documents using repeat groups causing android-wraper to crash #6136

Closed
derickl opened this issue Nov 25, 2019 · 7 comments
Closed
Assignees
Labels
Priority: 2 - Medium Normal priority Type: Bug Fix something that isn't working as intended Won't fix: Deprioritized Too low priority

Comments

@derickl
Copy link
Member

derickl commented Nov 25, 2019

Describe the bug
Creating linked documents as per the Creating Additional Docs sections of this doc is not working as expected. If we created documents as outlined in Repeated Docs Example and the child document is explicitly bound to the repeat, there's an overallocation of memory (300MB) compared to 50 MB when the child document is bound to a group directly wrapped under the repeat.

On typical CHW phones (1GB) , we consistently run out of memory and crash when using the original form https://medic.slack.com/files/U1ZF01J12/FQS6DJ2BF/pregnancy_outcomes.xlsx

I'm attaching two forms to illustrate when the feature is working and when it's breaking.

Test files
pregnancy_outcomes_buggy.xlsx
pregnancy_outcomes_fixed.xlsx

To Reproduce
Steps to reproduce the behavior:

  1. Upload one of the attached forms to an instance
  2. Launch the form Pregnancy Outcome
  3. Enter the 10 as the response to the question How many children
  4. Take a memory snapshot and observe the memory used.

Expected behavior
No crashing when running on mobile, especially memory constrained models that we recommend to partners.

Environment

  • Instance: moh-zanzibar.dev
  • Browser: Firefox (v70), Chrome on latest medic-android
  • Client platform: MacOS 10.15
  • App: (webapp, medic-android)
  • Version: (Probably 3.x but has been tested on 3.4-3.7)

Additional context
A workaround to what's explicit documented in https://github.com/medic/medic-docs/blob/master/configuration/forms.md is to bind the child document to a group immediately under the repeat.

Using the original partner form, memory allocation jumps from around 50MB to 3GB.

This has been reported by D-Tree and PIH

@derickl derickl added the Type: Bug Fix something that isn't working as intended label Nov 25, 2019
@derickl derickl changed the title Repeat groups causing app to crash Creating child documents using repeat groups causing android-wraper to crash Nov 25, 2019
@garethbowen garethbowen added the Priority: 2 - Medium Normal priority label Nov 25, 2019
@dianabarsan dianabarsan self-assigned this Apr 9, 2020
@dianabarsan
Copy link
Member

Looking at this, it seems like the cause of the overallocation of memory is one of the calculations in the form:

image

Because this refers to a repeat, every time a new repeat entity is added, every other existent repeat entity is recalculated in order to provide this new value, not just the one scoping the calculate (the "parent").
The used heap is successfully garbage collected after execution, so we're not talking about a memory leak.
I'm not sure why that calculation is set to equal the repeat itself, because we're also using db-doc-ref for the same field, the calculated value will be overwritten with the generated _id of the "child" document.
I know pyxform requires a calculate field, but if we use something else that does not refer to the "repeat", the memory overallocation doesn't occur.

Is it feasible to change the "calculate" field to something else and update the documentation to reflect this?

@kennsippell
Copy link
Member

kennsippell commented Apr 11, 2020

Can we use the test harness to automatically fail tests when the form allocates too much to the heap? Can the harness somehow flag scenarios similar to this? If so, we could enforce best practices like this across projects?

@dianabarsan
Copy link
Member

@derickl Can I, please, get some feedback about my comment above?

@garethbowen , you were curious if this happens on latest enketo-core, it does.

@derickl
Copy link
Member Author

derickl commented Apr 17, 2020

@dianabarsan
RE: #6136 (comment)

We could probably change how we arrive child_doc. (and update the documentation) medic/medic-docs#208

@garethbowen
Copy link
Member

Does anyone have any objections to closing this and going with the workaround Diana found plus the documentation issue Derick raised?

@dianabarsan
Copy link
Member

Do 8 objection-less days count as no objections?

@garethbowen
Copy link
Member

It seems like there are no objections so I'm closing this as Won't Fix so we can move forward with 3.9.0. Let me know if there's more we can do here.

@garethbowen garethbowen added the Won't fix: Deprioritized Too low priority label May 4, 2020
derickl pushed a commit to medic/medic-docs that referenced this issue May 11, 2020
derickl added a commit to medic/medic-docs that referenced this issue May 12, 2020
Related to medic/cht-core#6136

Resolves #208

Co-authored-by: Derick Lung'aho <derick.lungaho@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 2 - Medium Normal priority Type: Bug Fix something that isn't working as intended Won't fix: Deprioritized Too low priority
Projects
None yet
Development

No branches or pull requests

4 participants