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

Performance: dropped frames on reactive graph updates #14721

Closed
gyzerok opened this issue Dec 16, 2024 · 12 comments
Closed

Performance: dropped frames on reactive graph updates #14721

gyzerok opened this issue Dec 16, 2024 · 12 comments
Labels

Comments

@gyzerok
Copy link

gyzerok commented Dec 16, 2024

Describe the bug

We are developing a highly interactive application that displays a lot of interconnected data on the screen simultaneously. That's why our reactive graph is somewhat deep. Mostly Svelte is working amazingly for us and performance is great! However we've found one case where updating the graph taking so long, that our app start dropping frames.

All the following flamecharts are taken on a 4x CPU slowdown. That's to emulate more realistic user device as well as to make problems more visible. However without throttling there are occasional frame drops as well.

Here is a pick on how our flamechart does look like. The whole task here takes almost 78ms.
image

If we zoom in a bit, we can see lots of check_dirtiness calls.
image

This first 2 screens also got me wondering if it'd be possible to split process_deferred calls into multiple frames inside Svelte? That'd make for better scalability even on the big graphs.

And here is part of another task also taking 70ms+.
image

When zoomed it also should a lot of check_dirtiness calls.
image

Reproduction

Unfortunately making a reproduction seems to be really tricky and I am still working on it. Meanwhile I am looking for any help or suggestions on how to arrive at it. I know a high quality report would be much better and I'm looking to improve this one hopefully with your help.

Simplified our code looks somewhat like the following. We have a sorted list of items which we derive based on several fields values. Of course it's not the most efficient way to allocate an array and sort it on every change here, but it keeps a lot of things very simple (basically for the same reasons deriveds are advised over effects in docs). And I'd expect it to be performance bottleneck with large lists of items - like thousands or even tens of thousands (maybe I am wrong here). In our case frames seems to be dropped even when size is less than a hundred.

import { SvelteMap } from 'svelte/reactivity';

export class Store {
	cache: SvelteMap<number, Model> = new SvelteMap();

	constructor() {
		for (let i = 1; i <= 500; i++) {
			const model = new Model(i);
			this.cache.set(i, model);
		}
	}

	readonly all: Model[] = $derived.by(() => {
		return Array.from(this.cache.values()).sort((a, b) => b.maxTime - a.maxTime);
	});

	readonly even: Model[] = $derived.by(() => {
		return this.all.filter((model) => model.id % 2 === 0);
	});
}

export class Model {
	time: number = $state(0);

	updateTime: number | null = $state(null);

	constructor(readonly id: number) {
		this.time = Date.now();
	}

	moveUp() {
		this.updateTime = Date.now();
	}

	maxTime = $derived.by(() => {
		return Math.max(this.time, this.updateTime || 0);
	});
}
<script lang="ts">
	import type { Store } from '$lib/Store.svelte';
	import { getContext } from 'svelte';
	import { flip } from 'svelte/animate';

	const store: Store = getContext('store');
</script>

<div>
	<div>Total: {store.all.length}, Even: {store.even.length}</div>
	{#each store.all as model, i (model.id)}
		{@const next = store.all[i + 1]}
		<div animate:flip>
			{model.id.toString().padStart(4, '0')}{' '}<button onclick={() => model.moveUp()}
				>click</button
			>

			{#if !next}
				<div>that's it</div>
			{/if}
		</div>
	{/each}
</div>

Logs

No response

System Info

MacBook Pro M1 2020, 16GB RAM
Chrome 131 with 4x CPU throttling enabled

Severity

annoyance

@peterkogo
Copy link

@gyzerok I was facing very similar issues with a somewhat similar use case.

Here is a playground reproduction of the code you provided. When should the performance issues exactly appear?

@gyzerok
Copy link
Author

gyzerok commented Dec 16, 2024

@peterkogo unfortunately my code is not enough to show a sensible reproduction. Apparently it really depends on the size of the reactive graph which in our real app is much bigger. The code I've posted for now is more to give a rough idea of what our app is doing.

However in the REPL with 4x slowdown the task still gets quite a lot of time to complete, though there are no missing frames there. In our case animations start stuttering and loosing frames.
image

@gyzerok
Copy link
Author

gyzerok commented Dec 16, 2024

@peterkogo can you also tell me more about your case? Did you solve those issues?

To me it feels like reactive graph should scale somehow in svelte, so making it larger does not result in stuttering when growing. It'd be great if svelte would propagate signals in multiple tasks. But I am not sure if that's reasonable or even doable.

@peterkogo
Copy link

In our case, the state consists of an array of objects and making that state deeply reactive is unfortunately too slow. We had to opt for $state.raw and make the objects immutable.

We had to pay the performance cost in 2 ways:

  1. adding far more dependencies in your state graph, as every single property access results in a runtime state subscription (I don't know if I'm using the right terms here, anyone correct me please)

  2. Accessing deeply reactive state in general is also considerably slower. We have to iterate over the data in a non-reactive manner (some javascript functions that calculate stuff) and just getting some value from an object was slowing things down. Proxies don't come for free.

@trueadm
Copy link
Contributor

trueadm commented Dec 16, 2024

@peterkogo unfortunately my code is not enough to show a sensible reproduction. Apparently it really depends on the size of the reactive graph which in our real app is much bigger. The code I've posted for now is more to give a rough idea of what our app is doing.

However in the REPL with 4x slowdown the task still gets quite a lot of time to complete, though there are no missing frames there. In our case animations start stuttering and loosing frames. image

Most of the time is because of DEV. In prod this REPL example with 6x slowdown is still less than 1ms for me.

I think in your original use-case, you want to avoid deep reactivity in performance sensitive areas. As mentioned above, using $state.raw will be significantly faster because proxies aren't free and they add about 20% overhead compared to not using them.

@Leonidaz
Copy link

Leonidaz commented Dec 16, 2024

@gyzerok just an experiment, I didn't measure any of this.

try this version, modified from what @peterkogo created:

Playground

  • replaced SvelteMap with Map
  • created a method on the store, sortChanged() to set only one state to watch for the deriveds vs having to watch every single item when they're looped over (uses untrack())
  • pass a reference sortChanged() from the store to the model
  • trigger derived reactivity by explicitly calling sortChanged() from moveUp().
  • UPDATED: removed all state / reactivity from the Model (and untrack on deriveds) as it's no longer necessary.

i'm also curious in your perf measurements, given the example above, if before triggering sortChanged() can be after a tick, e.g. tick().then(() => this.#sortChanged());.

@gyzerok
Copy link
Author

gyzerok commented Dec 16, 2024

We've made a call with Dominic and were able to figure out what should be changed in my reproduction to produce the problem.

The issue is in how the Store is created. If we change my code to create store outside of the UI graph then all the derived created within it become unowned which in turn disables optimization inside svelte which skips redundant check_dirtiness for signals. That basically results in the flamegraphs shown in the issue.

The fix is in the linked PR and it wastly improves performance in my original codebase. So hopefully everyone can benefit from it soon.

Huge thanks to Dominic!

@Leonidaz
Copy link

Leonidaz commented Dec 16, 2024

@gyzerok @trueadm that's awesome! 🚀

@gyzerok I updated the example that I provided above to also remove the reactivity from the Model class and untrack from deriveds (as they were no longer needed).

Just out of curiosity, with the @trueadm's fix, based on my example, if reducing the number of signals and subscribers makes any difference for the performance? (But, yeah, not sure of your exact use case as you may need the reactivity for other stuff).

Playground

@gyzerok
Copy link
Author

gyzerok commented Dec 17, 2024

I've updated our project to 5.14.1 and here is how the same flamegraph looks now with 4x slowdown. It still takes some time but is 2x faster with animations not lagging noticeably.

Without 4x slowdown everything is silky smooth and everything takes less than a frame 10-12ms (previously around 30ms).

image

@gyzerok
Copy link
Author

gyzerok commented Dec 17, 2024

@Leonidaz thank you for your suggestion! If I understand in your REPL everything correctly, I think Dominic was proposing a similar thing with switching back to a regular Map and adding a version field to it for reactivity. I think I'll try that as well and see how it goes.

@Leonidaz
Copy link

@Leonidaz thank you for your suggestion! If I understand in your REPL everything correctly, I think Dominic was proposing a similar thing with switching back to a regular Map and adding a version field to it for reactivity. I think I'll try that as well and see how it goes.

cool, yeah would be really interesting to see if there is any difference for the sake of all of us. 🙏

btw, I also updated the example to just use a simple array and not have to convert from Map to array. Optimized derived on the first run as the array is already sorted.

@gyzerok
Copy link
Author

gyzerok commented Dec 18, 2024

Closing the issue since performance is much better after the fix and there no other concrete findings here. Will open a new one if more info comes out.

@Leonidaz using plain Map does not yield any noticeable result in our code unfortunately.

@gyzerok gyzerok closed this as completed Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants