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

Make useExpressionDataSources and useValidationDataSources cheaper #2720

Merged
merged 31 commits into from
Nov 18, 2024

Conversation

bjosttveit
Copy link
Member

@bjosttveit bjosttveit commented Nov 14, 2024

Description

When profiling loading a form with many repeating group rows, as well as when filling out data in a large repeating group, one of the largest contributors to timing and memory allocation is useExpressionDataSources and useValidationDataSources. Both of these hooks use a lot of simple hooks within them as well as a lot of delayed selectors. When having a thousands of nodes these hooks are called A LOT, and so optimizing these makes a big difference even though you may not think these are expensive in regular use. There are primarily two optimizations introduced here, one for simple hooks, and one for delayed selectors.

  1. For regular hooks where everyone just needs the exact same value, I created a createHookContext method that allows you to run the hook once for the entire node generator and then reuse the value with a useContext instead. It turns out that even a direct subscription to a zustand store via a useSelector is much more expensive than using a simple context value. Some other hooks like useExternalAPI (useQuery) is also expensive when used at this scale.
  2. Delayed selectors previously used 3x useRef, 1x useState, 1x useEffect, and 2x useCallback. For expression data sources we probably had something like 10 delayed selectors (there were some overlap as some other hooks used them internally), we use something like up to 4 expressiondatasources for each node (markhidden, runexpressions, options, sourceoptions). With 10 000 nodes, this becomes a cool couple million hooks 🤯 (of just delayed selectors). Now its more like 80 000 hooks for 10 000 nodes of useExpressionDataSources (of just delayed selectors). Delayed selectors are now implemented using only 1x useRef, and 1x useSyncExternalStore. Additionally, you can create an arbitrary number of delayed selectors with useMultipleDelayedSelectors that also just has a fixed 1x useRef and 1x useSyncExternalStore.

Related Issue(s)

  • closes #{issue number}

Verification/QA

  • Manual functionality testing
    • I have tested these changes manually
    • Creator of the original issue (or service owner) has been contacted for manual testing (or will be contacted when released in alpha)
    • No testing done/necessary
  • Automated tests
    • Unit test(s) have been added/updated
    • Cypress E2E test(s) have been added/updated
    • No automatic tests are needed here (no functional changes/additions)
    • I want someone to help me make some tests
  • UU/WCAG (follow these guidelines until we have our own)
    • I have tested with a screen reader/keyboard navigation/automated wcag validator
    • No testing done/necessary (no DOM/visual changes)
    • I want someone to help me perform accessibility testing
  • User documentation @ altinn-studio-docs
    • Has been added/updated
    • No functionality has been changed/added, so no documentation is needed
    • I will do that later/have created an issue
  • Support in Altinn Studio
    • Issue(s) created for support in Studio
    • This change/feature does not require any changes to Altinn Studio
  • Sprint board
    • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board
    • I don't have permissions to do that, please help me out
  • Labels
    • I have added a kind/* label to this PR for proper release notes grouping
    • I don't have permissions to add labels, please help me out

@bjosttveit bjosttveit added the kind/other Pull requests containing chores/repo structure/other changes label Nov 15, 2024
@bjosttveit bjosttveit marked this pull request as ready for review November 15, 2024 14:11
Copy link
Contributor

@olemartinorg olemartinorg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phenomenal!! :godmode: I think I understood at least 80% of this, and reading this has finally forced me to learn more about useSyncExternalStore() (which I've wanted for some time now). Thanks for that! 😅

src/utils/layout/generator/GeneratorDataSources.tsx Outdated Show resolved Hide resolved
bjosttveit and others added 6 commits November 15, 2024 16:35
Co-authored-by: Ole Martin Handeland <github@olemartin.org>
…nd store, making it far more accessible (also removes the need for selecting it in a hookContext)
# Conflicts:
#	src/utils/layout/NodesContext.tsx
#	src/utils/layout/generator/LayoutSetGenerator.tsx
Ole Martin Handeland and others added 5 commits November 18, 2024 14:08
…red when the route changes. This caused a node item to remain 'undefined', blocking the state from becoming ready. Also adding a tripwire for this.
…e-renders of FormProvider (which in turn causes all-node removal and re-adds). Requiring conditions on all set-hooks in the future.
@bjosttveit bjosttveit merged commit 8489406 into main Nov 18, 2024
13 checks passed
@bjosttveit bjosttveit deleted the fix/perf#6 branch November 18, 2024 15:35
@bjosttveit bjosttveit self-assigned this Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/other Pull requests containing chores/repo structure/other changes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants