Skip to content

Performance issue with contexts using $derived in pair with array operations (dev mode only) #14491

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

Closed
plgingras88 opened this issue Dec 1, 2024 · 5 comments · Fixed by #14533
Labels

Comments

@plgingras88
Copy link
Contributor

plgingras88 commented Dec 1, 2024

Describe the bug

In development mode only, Svelte contexts can experience significant performance slowdowns when used in pair with any array operations (map, filter, etc.) in a $derived, no matter if the derived dependencies have changed or not.

Steps to reproduce:

1- Create a "runes class" with 3 runes:

  • A $state rune (A) that is an array.
  • A $derived rune (B) that simply references the $state and performs a trivial operation (e.g., = array.map(item => item))).
  • A $state rune (C) that has no relationship with the previous $state (A) and its $derived (B).

2- Instantiate the class and pass the instance to a context using setContext.

3- Have multiple child components call getContext to retrieve the context.

Observed behavior:
If $state (C) changes, despite it has no impact on $state (A) neither its $derived (B), each additional child component calling getContext introduces incremental delays, which accumulate and result in a noticeable slowdown.

Reproduction

https://svelte.dev/playground/25a02d8edb9f4a4b9e5de54188beb94e?version=5.2.9

Simply hover any row. It will change the hovered $state (C). You'll notice it is laggy. But if you remove the array map in the $derived, it is fast again.

In my real-world codebase, running a devtool perf graph exposes hundreds of add_owner_to_object and add_owner instances. These seem to be only existing in dev mode.

Logs

No response

System Info

System:
    OS: Windows 11 10.0.22631
    pnpm: 9.12.1 - ~\AppData\Roaming\npm\pnpm.CMD
    bun: 1.1.29 - ~\AppData\Roaming\npm\bun.CMD
  Browsers:
    Edge: Chromium (131.0.2903.63)
    Internet Explorer: 11.0.22621.3527
  npmPackages:
    svelte: ^5.2.6 => 5.2.10

Severity

annoyance

@Ocean-OS
Copy link
Contributor

Ocean-OS commented Dec 2, 2024

There appear to be a few issues/potential solutions for this:

  1. In the documentation, it does mention that Context is not inherently reactive.
  2. The array is very large, and so Array.map calls will take more time than if the array had a shorter length.
  3. One thing I have found that appears to mitigate most of the performance issues is to put the data in component props rather than context.
  4. Another thing I'm not entirely sure of is that add_owner/add_owner_to_object might be iterating through myData as well, slowing down performance even more.
  5. Additionally, Svelte does not proxy classes, so to improve reactivity and performance, use a POJO when possible.

@henrykrinkle01
Copy link
Contributor

So I had a lengthy discussion with OP in the discord thread that lead to this issue

  1. I don't see why it's relevant here. The class instance itself is not reactive, but its properties are.
  2. The Array.map call only happens once.
  3. We prefer using context to avoid prop drilling.
  4. This only happens in dev time. In production where there is no add_owner / add_owner_to_object, it's fast. But let's compare to this repl with class extension where the class instance is exactly the same, but it's fast. There is something weird going on.
  5. This is only a minimal reproduction. Using classes is valid and we should not be punished for using them.

@dummdidumm
Copy link
Member

This is a dev mode only problem because add_owner / add_owner_to_object are validation operations to be able to do mutation validation checks. You will not have these problems in prod.

We can look into why exactly it's slow in this case.

@dummdidumm dummdidumm added the perf label Dec 2, 2024
@plgingras88
Copy link
Contributor Author

plgingras88 commented Dec 2, 2024

This is a dev mode only problem because add_owner / add_owner_to_object are validation operations to be able to do mutation validation checks. You will not have these problems in prod.

We can look into why exactly it's slow in this case.

Definitely a dev thing, but it took us several hours to realize that it occurs only in development mode. I really appreciate HenryKrinkle's assistance in identifying this untrivial situation.

In our real-world application, we invoke the contexts thousands of times, which causes the app to freeze for many seconds whenever any of the context's reactive properties is triggered. This makes the app unusable during development, hindering our ability to progress and be confident with the prod output too.

Moreover, although this is a development-only issue, it can be a significant deterrent for new Svelte developers trying to learn and understand the framework. I believe the development mode should not exhibit such significant performance degradation compared to production mode, as it can be misleading and counterproductive.

@dummdidumm
Copy link
Member

The reason why this is so slow is:

  • ownership addition recurses into an object until it finds a state symbol or class boundary
  • we're calling a hidden add_owner method on the class, that adds the given owner to all of the class' reactive objects
  • we're wrapping this call in a render_effect so that in case something changes we can rerun ownership addition
  • now if you hover over something, hover is changed, which means for each getContext, i.e. each row we do one render_effect, and for each of them we iterate over the derived with thousands of elements, finding the state symbol for each element, and call ownership addition on it. So that's ~ 2000 render effects times 2000 array entries

The task is to find a way to do less work here.

dummdidumm added a commit that referenced this issue Dec 3, 2024
`$state.raw` and `$derived(.by)` will not have a state symbol on them, potentially causing a disastrous amount of traversal to potentially not find any state symbol. So it's better to not traverse them.

Potentially someone could create a `$state` while creating `$state.raw` or inside a `$derived.by`, but that feels so much of an edge case that it doesn't warrant a perf hit for the common case.

Fixes #14491
Rich-Harris pushed a commit that referenced this issue Dec 3, 2024
* fix: don't try to add owners to non-`$state` class fields

`$state.raw` and `$derived(.by)` will not have a state symbol on them, potentially causing a disastrous amount of traversal to potentially not find any state symbol. So it's better to not traverse them.

Potentially someone could create a `$state` while creating `$state.raw` or inside a `$derived.by`, but that feels so much of an edge case that it doesn't warrant a perf hit for the common case.

Fixes #14491

* for bind:, too
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants