🦋 Changeset detected

Latest commit: 76cd242

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Playground

pnpm add https://pkg.pr.new/svelte@17461

Although my initial approach to the async tracking fix worked, it caused boundaries in a particular component of mine to get stuck on pending. Can't quite reproduce it in a test, but I've refactored to only record promises as settled after decrement which takes care of that bug.

This appears to completely fix the significant performance issues my projects have been having since Svelte >5.43.3 with components with LayerChart; thank you!

Great work @hmnd!

I tested this locally against the new LayerChart docs and see a significant improvement for many examples (especially Choropleth and Bubble map). Thanks!

The spike map example is still very slow compared to pre-async even with this PR, but I'll reevaluate more closely and report back after this PR hits an official release (and see if I find any other examples).

I appreciate all your work, and thanks again!

@techniq Could you give my latest push a try? I think it solves for the spike map now too. Performance doesn't look completely on the same level of next.layerchart.com, but pretty close!

Also, if comparing against the deployed next.layerchart.com, try it with pnpm preview or env NODE_ENV=production pnpm dev --mode production to avoid slow dows from dev-time checks.

That was quick!

I tested the spike map with the latest https://pkg.pr.new/svelte@17461 and dev still takes over 15 seconds to render and production (env NODE_ENV=production pnpm dev --mode production) take a few seconds.

With that said, this is drastically better than using svelte@latest which typically takes 30+ seconds and produces "Page Unresponsive" prompts (sometimes multiple).

Compared to next branch (pre-async), even with these changes they still feel slower, but haven't done any direct measurement (even running locally, spike map renders in a 2-3 seconds compared to 15+)

With that said, this is VERY much trending strongly in the right direction.

I was seeing a huge number of flushes happening, due to decrement being called on each #each item. Best I can come up with at the moment is batching batch.decrement and pending count updates via micro tasks. The latter is needed in order to schedule it in the same batch as decrement, as otherwise I was runinng into async-effect-pending tests failing.

Though the children of the each block being in an async() feels weird... I think that's happening because of the @const deriveds within them, but does that make sense considering the each is already in an async()?

Profiling on layerchart docs, it looks like this finally brings things down to pre-async levels, with rendering the #each items incurring the highest cost (as expected with a high number of items to render).

Or maybe there's some work needed around getting the is_promise_settled fast path to hit more often?

I was playing around with this branch, trying to understand how we end up in situations where we're awaiting a promise that's already resolved, and it seems to be very rare — with this diff...

diff --git a/packages/svelte/src/internal/client/dom/blocks/async.js b/packages/svelte/src/internal/client/dom/blocks/async.js
index fc04894772..d0183ac3ed 100644
--- a/packages/svelte/src/internal/client/dom/blocks/async.js
+++ b/packages/svelte/src/internal/client/dom/blocks/async.js
@@ -20,6 +20,7 @@ import { get_boundary } from './boundary.js';
  */
 export function async(node, blockers = [], expressions = [], fn) {
 	if (expressions.length === 0 && blockers.length > 0 && blockers.every(is_promise_settled)) {
+		throw new Error('here');
 		fn(node);
 		return;
 	}

...no tests fail, though you can reach that case like so:

<script>
	import Component from './Component.svelte';

	await delayed(0, 500);
	let x = 1;
</script>

<Component {x}>
	<Component {x} />
</Component>

With this diff...

diff --git a/packages/svelte/src/internal/client/reactivity/async.js b/packages/svelte/src/internal/client/reactivity/async.js
index 6ee97b86c3..0a13566cac 100644
--- a/packages/svelte/src/internal/client/reactivity/async.js
+++ b/packages/svelte/src/internal/client/reactivity/async.js
@@ -47,6 +47,10 @@ export function flatten(blockers, sync, async, fn) {
 	// Filter out already-settled blockers - no need to wait for them
 	var pending = blockers.filter((b) => !is_promise_settled(b));
 
+	if (pending.length < blockers.length) {
+		throw new Error('here');
+	}
+
 	if (async.length === 0 && pending.length === 0) {
 		fn(sync.map(d));
 		return;

...we get 3 distinct failed tests, such as async-each-derived:

<script>
	let checked = $state(false);

	const foo = $derived(await checked);
</script>

<input type="checkbox" bind:checked />

{#each checked === foo && [1]}
	<p>{checked}</p>
{/each}

The template effect inside the each block is waiting for the const foo ... because Svelte overzealously treats that declaration as having the potential to change the value of checked, because checked is an argument to a CallExpression. If we tell it not to do that in the case of $derived(...) expressions...

diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js
index ef0b35f560..63d63020c6 100644
--- a/packages/svelte/src/compiler/phases/2-analyze/index.js
+++ b/packages/svelte/src/compiler/phases/2-analyze/index.js
@@ -1032,6 +1032,12 @@ function calculate_blockers(instance, scopes, analysis) {
 					const rune = get_rune(node, context.state.scope);
 					if (rune === '$effect') return;
 
+					// deriveds do not mutate their inputs
+					if (rune === '$derived' || rune === '$derived.by') {
+						context.next();
+						return;
+					}
+
 					/** @type {Set<Binding>} */
 					const touched = new Set();
 					touch(node, context.state.scope, touched);

...the test passes. (Might open a branch for that; it applies to other runes too, though when I tried doing it for all runes it caused separate failures which I haven't yet investigated.)

All of which is to say that it seems at least possible that this is enough of an edge case that the additional complexity isn't warranted. (I tend to be wary of using WeakSet in general as I've been given to understand it introduces its own performance downsides. I wondered about replacing the promises with { promise, settled } objects, but that of course has rippling effects...). Though it's always possible that the test suite just isn't representative of pathological cases in real-world apps.

Either way I do think it could be worth us tweaking the compiler to make these cases even rarer. The previous diff is one such example. Another example would be async-style-after-await, where this code gets generated:

$.async(node_1, [$$promises[1]], [() => color], (node_1, $$condition) => {
	var consequent_1 = ($$anchor) => {
		// ...

		$.template_effect(
			() => // ...,
			void 0,
			void 0,
			[$$promises[1], $$promises[2]]
		);

		// ...
	};

	// ...
});

It seems unnecessary to await $$promises[1] inside a block where it has already been awaited, and even if it wasn't, $$promises[1] is guaranteed to have settled before $$promises[2] does, so we can just wait for the second one. I haven't looked into how far this generalises, or whether it would be straightforward to make that work.

For now I'll take a closer look at #17511, since it seems like that's where the bigger wins are. Thank you for digging into this!

Found it! promise is reassigned on each iteration, so essentially only one promise was getting added to settled_promises instead of all of them

D'oh — I somehow contorted myself into thinking that wouldn't be a problem, because the promise wouldn't get reassigned until the previous one had resolved? Which... is not how this works at all 😆

I just did some test runs on just #17511

Out of curiosity what test case are you using for this? The LayerChart REPL in #17176 (comment) doesn't appear to be improved by either this or #17511

Happy to bo of some help :)

Yeah, I alluded to the unnecessary awaiting earlier in #17461 (comment)

Here's an example from the layerchart spike-map page (omitting a bunch of irrelevant bits for readability).

$.async(node, [$$promises[3], $$promises[12]], void 0, ($$anchor) => {
		const children = $.wrap_snippet(Spike_map, function ($$anchor, $$arg0) {
			$.validate_snippet_args(...arguments);

			let context = () => $$arg0?.().context;

			context();

			const strokeWidth = $.tag($.derived(() => 1 / context().transform.scale), 'strokeWidth');

			$.get(strokeWidth);

			var fragment_1 = root_1();
			var node_1 = $.first_child(fragment_1);

			$.add_svelte_meta(() => TransformContextControls(node_1, {}), 'component', Spike_map, 71, 2, { componentTag: 'TransformContextControls' });

			var node_2 = $.sibling(node_1, 2);

			$.add_svelte_meta(
				() => Layer(node_2, {
					children: $.wrap_snippet(Spike_map, ($$anchor, $$slotProps) => {
						var fragment_2 = root_2();
						var node_3 = $.first_child(fragment_2);

						$.async(node_3, [$$promises[12]], void 0, ($$anchor) => {
							$.add_svelte_meta(
								() => GeoPath(node_3, {
									get geojson() {
										return states;
									},

									class: 'fill-surface-content/10 stroke-surface-100',

									get strokeWidth() {
										return $.get(strokeWidth);
									}
								}),
								'component',
								Spike_map,
								74,
								3,
								{ componentTag: 'GeoPath' }
							);
						});

						var node_4 = $.sibling(node_3, 2);

						$.async(node_4, [$$promises[12]], [() => enrichedCountiesFeatures], (node_4, $$collection) => {
							$.add_svelte_meta(
								() => $.each(node_4, 17, () => $.get($$collection), $.index, ($$anchor, feature, $$index, $$array) => {
									var fragment_3 = $.comment();
									var node_5 = $.first_child(fragment_3);

                  ...

									$.append($$anchor, fragment_3);
								}),
								'each',
								Spike_map,
								76,
								3
							);
						});

						var node_7 = $.sibling(node_4, 2);

						$.async(node_7, [$$promises[12]], [() => enrichedCountiesFeatures], (node_7, $$collection) => {
							$.add_svelte_meta(
								() => $.each(node_7, 17, () => $.get($$collection), $.index, ($$anchor, feature, $$index_1, $$array_1) => {
								...
								}),
								'each',
								Spike_map,
								91,
								3
							);
						});

						$.append($$anchor, fragment_2);
					}),

I don't think anything within the top-level $.async should need to await promises[12]. It's awaited in the script, so it's probably safe to assume it's settled by the time the template is rendered?

I'm running the docs-v2 branch of https://github.com/techniq/layerchart

Run the following in the docs folder:

pnpm generate:all # tripped me up at first, only needed once
pnpm dev --force

Though really all you should need is a mix of top-level await returning a large array + each over it, containing an async derived referring back to the await. See: https://github.com/techniq/layerchart/blob/docs-v2/docs/src/examples/components/GeoPath/spike-map.svelte

For now I'll take a closer look at #17511, since it seems like that's where the bigger wins are.

Actually I take it back — with this test I see very significant wins over main (though bizarrely, only when running locally — for some reason it's faster in the playground, and consistent across the two branches? I have no idea why):

<script>
	import { tick } from 'svelte';

	let condition = $state(false);

	await 1;
	let array = Array(20000);
	let message = 'hello';
</script>

<button onclick={() => {
	condition = !condition;

	if (condition) {
		console.time('render');

		tick().then(() => {
			console.timeEnd('render');
		});
	}
}}>
	toggle
</button>

{#if condition}
	{#each array}
		{#if true}
			<p>{message}</p>
		{/if}
	{/each}
{/if}

I'll give the { promise, settled } idea a try to see if there's any observable difference between that and using the WeakSet

Maybe related - I seem to remember there being a hot path in flush_effects due to some extra tracking that only happens in dev

Merged #17519 into this, rather than merging that branch. As mentioned over there I suspect the { promise, settled } approach is microscopically faster, and local testing bears this out.

Will still look out for ways to make the compiler smarter so that we hit the case where we need this logic less often. This is a great improvement, thanks @hmnd!