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

Instance with many repeating group wont open #822

Closed
FTLems opened this issue Jan 10, 2023 · 5 comments · Fixed by #861
Closed

Instance with many repeating group wont open #822

FTLems opened this issue Jan 10, 2023 · 5 comments · Fixed by #861
Assignees
Labels
kind/bug Something isn't working org/krt quality/debt

Comments

@FTLems
Copy link
Contributor

FTLems commented Jan 10, 2023

Description of the bug

We have app: KRT-1012a-1 which is abo-schema (We instanciate to users mailbox).
This app contains a reapeating group:
image

If there are many repeating group (maybe 120 stk), it takes12 sek to open the group after clicking Rediger knapp. In general the entire app is very slow.

Also, it cant handle that many components in summary-page, and the entire summary-page appears white after it should iterate the repeating group here.
image

The instance is:
/instance/50164456/78c06a1a-9684-4af5-bc39-30b243733bb3

Note. The repeating groups are empty and only contain one field of data (component 5.1). User has to fill in the rest.

Steps To Reproduce

  1. Find the instance and try to run it ?

Additional Information

No response

@FTLems FTLems added the kind/bug Something isn't working label Jan 10, 2023
@ivarne
Copy link
Member

ivarne commented Jan 10, 2023

I tried to look at the js profile, and it seems like most of the time is spent recalculating useLayoutsAsNodes again and again. There is a useMemo there, but it obviously caches per component, which is not enough when there is hundreds of rows. Seems like another approach is required. Would it be possible to call resolvedNodesInLayout and nodesInLayout when a relevant action dispatches in redux and cache the result in redux.
image

@olemartinorg olemartinorg transferred this issue from Altinn/altinn-studio Jan 10, 2023
@olemartinorg
Copy link
Contributor

Very related to #345, and in some ways related to #377.

Nice to know about this limitation, and there are been thoughts and plans to run expressions a little less often (i.e. by doing it once and passing around the resolved nodes using a top-level react context).

Is there an easy way for me to reproduce this issue? I don't have access to your data model, so I'd love to get my hands on something I can use to replicate this (without creating 120+ rows myself).

@olemartinorg olemartinorg moved this to ⚠️ Blocked in Team Apps Jan 11, 2023
@olemartinorg
Copy link
Contributor

We've discussed this a bit further on Slack, and I now have some test instance data for me to reproduce the issue. This issue is currently blocking the release of this app, which should be in production on 01.feb, so we should have a fix (or workaround) in time before that.

@olemartinorg olemartinorg self-assigned this Jan 12, 2023
@olemartinorg olemartinorg moved this from ⚠️ Blocked to 👷 In Progress in Team Apps Jan 20, 2023
@olemartinorg
Copy link
Contributor

@ivarne @FTLems
Good evening, and good news! I've made some good progress on this, but some things still remain for this to be possible to release. You can have a look at my working branch bug/expr-per-group, and test the changes from there.

When testing out your sample with 124 rows in a repeating group, on my computer it takes ~456ms to open the repeating group. Of this, resolving expressions now take ~66ms.

20230123_20h27m23s_grim

From what I can tell from the flame graph above (spanning ~456ms), the big blocks taking time now are:

  • Dispatching the event to open the repeating group row for editing, updating the redux state
  • Expression resolving (~66ms). This can be further optimized if we want to, as not much is changing when you open the repeating group for editing - in fact, nothing should really change, but for some reason the useMemo() refreshes here.
  • Our createRepeatingGroupComponents function (~57ms). I believe this is due for a replacement using our node hierarchy, as this hierarchy is already constructed when evaluating the expressions above. I haven't looked into this one in particular, but I believe it could be refactored away entirely in the future.
  • React virtual DOM diffing. This takes up pretty much the rest of the time. I also haven't looked into this in particular, but our entire repeating group table (with children) re-renders, which might(!) be unnecessary. If I have time, I will look into optimizing this some more.

I think this is at a workable state right now, but the large refactor needed to optimize this caused a few breakages I need to address and fix before releasing it. As mentioned, please test out the branch as it is right now and let me know if you find this usable or not! I'll keep working on fixing the breakages so that we'll be able to release this very soon.

@FTLems
Copy link
Contributor Author

FTLems commented Jan 25, 2023

Thanks for prioritizing this. Just tried this branch yesterday, and it was significant improvent. I agree that this is a workable state and we can use this for our app. 👍

@olemartinorg olemartinorg moved this from 👷 In Progress to 🔎 Review in Team Apps Jan 26, 2023
@olemartinorg olemartinorg moved this from 🔎 Review to 🧪 Test in Team Apps Jan 27, 2023
@olemartinorg olemartinorg moved this from 🧪 Test to ✅ Done in Team Apps Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working org/krt quality/debt
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants