fix: more efficient template effect grouping (#15050)

* WIP * drive-by * group attribute updates * fix * more * unused * more * WIP * unused * simplify * simplify * simplify * more * unused * unused * more * tweak * update how class/style directives are handled * more * more * simplify * changeset
sveltejs · Jan 18, 2025 · 700029b · 700029b
1 parent a9d1f46
commit 700029b
Show file tree

Hide file tree

Showing 24 changed files with 227 additions and 199 deletions.
diff --git a/.changeset/cyan-games-cheat.md b/.changeset/cyan-games-cheat.md
@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: more efficient template effect grouping
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js
@@ -61,9 +61,6 @@ export function Attribute(node, context) {
 			) {
 				continue;
 			}
-
-			node.metadata.expression.has_state ||= chunk.metadata.expression.has_state;
-			node.metadata.expression.has_call ||= chunk.metadata.expression.has_call;
 		}
 
 		if (is_event_attribute(node)) {

diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/shared/function.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/shared/function.js
@@ -15,6 +15,7 @@ export function visit_function(node, context) {
 
 	context.next({
 		...context.state,
-		function_depth: context.state.function_depth + 1
+		function_depth: context.state.function_depth + 1,
+		expression: null
 	});
 }
diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js
@@ -169,9 +169,9 @@ export function client_component(analysis, options) {
 		module_level_snippets: [],
 
 		// these are set inside the `Fragment` visitor, and cannot be used until then
-		before_init: /** @type {any} */ (null),
 		init: /** @type {any} */ (null),
 		update: /** @type {any} */ (null),
+		expressions: /** @type {any} */ (null),
 		after_update: /** @type {any} */ (null),
 		template: /** @type {any} */ (null),
 		locations: /** @type {any} */ (null)

diff --git a/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts b/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts
@@ -46,14 +46,14 @@ export interface ComponentClientTransformState extends ClientTransformState {
 	readonly events: Set<string>;
 	readonly is_instance: boolean;
 
-	/** Stuff that happens before the render effect(s) */
-	readonly before_init: Statement[];
 	/** Stuff that happens before the render effect(s) */
 	readonly init: Statement[];
 	/** Stuff that happens inside the render effect */
 	readonly update: Statement[];
 	/** Stuff that happens after the render effect (control blocks, dynamic elements, bindings, actions, etc) */
 	readonly after_update: Statement[];
+	/** Expressions used inside the render effect */
+	readonly expressions: Expression[];
 	/** The HTML template string */
 	readonly template: Array<string | Expression>;
 	readonly locations: SourceLocation[];

diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/Fragment.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/Fragment.js
@@ -61,9 +61,9 @@ export function Fragment(node, context) {
 	/** @type {ComponentClientTransformState} */
 	const state = {
 		...context.state,
-		before_init: [],
 		init: [],
 		update: [],
+		expressions: [],
 		after_update: [],
 		template: [],
 		locations: [],
@@ -124,18 +124,13 @@ export function Fragment(node, context) {
 
 		add_template(template_name, args);
 
-		body.push(b.var(id, b.call(template_name)), ...state.before_init, ...state.init);
+		body.push(b.var(id, b.call(template_name)));
 		close = b.stmt(b.call('$.append', b.id('$$anchor'), id));
 	} else if (is_single_child_not_needing_template) {
 		context.visit(trimmed[0], state);
-		body.push(...state.before_init, ...state.init);
 	} else if (trimmed.length === 1 && trimmed[0].type === 'Text') {
 		const id = b.id(context.state.scope.generate('text'));
-		body.push(
-			b.var(id, b.call('$.text', b.literal(trimmed[0].data))),
-			...state.before_init,
-			...state.init
-		);
+		body.push(b.var(id, b.call('$.text', b.literal(trimmed[0].data))));
 		close = b.stmt(b.call('$.append', b.id('$$anchor'), id));
 	} else if (trimmed.length > 0) {
 		const id = b.id(context.state.scope.generate('fragment'));
@@ -153,7 +148,7 @@ export function Fragment(node, context) {
 				state
 			});
 
-			body.push(b.var(id, b.call('$.text')), ...state.before_init, ...state.init);
+			body.push(b.var(id, b.call('$.text')));
 			close = b.stmt(b.call('$.append', b.id('$$anchor'), id));
 		} else {
 			if (is_standalone) {
@@ -182,15 +177,13 @@ export function Fragment(node, context) {
 
 				close = b.stmt(b.call('$.append', b.id('$$anchor'), id));
 			}
-
-			body.push(...state.before_init, ...state.init);
 		}
-	} else {
-		body.push(...state.before_init, ...state.init);
 	}
 
+	body.push(...state.init);
+
 	if (state.update.length > 0) {
-		body.push(build_render_statement(state.update));
+		body.push(build_render_statement(state));
 	}
 
 	body.push(...state.after_update);

diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js
@@ -1,4 +1,4 @@
-/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, ObjectExpression, Statement } from 'estree' */
+/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, Statement } from 'estree' */
 /** @import { AST } from '#compiler' */
 /** @import { SourceLocation } from '#shared' */
 /** @import { ComponentClientTransformState, ComponentContext } from '../types' */
@@ -16,7 +16,7 @@ import { is_event_attribute, is_text_attribute } from '../../../../utils/ast.js'
 import * as b from '../../../../utils/builders.js';
 import { is_custom_element_node } from '../../../nodes.js';
 import { clean_nodes, determine_namespace_for_children } from '../../utils.js';
-import { build_getter, create_derived } from '../utils.js';
+import { build_getter } from '../utils.js';
 import {
 	get_attribute_name,
 	build_attribute_value,
@@ -28,8 +28,8 @@ import { process_children } from './shared/fragment.js';
 import {
 	build_render_statement,
 	build_template_chunk,
-	build_update,
-	build_update_assignment
+	build_update_assignment,
+	get_expression_id
 } from './shared/utils.js';
 import { visit_event_attribute } from './shared/events.js';
 
@@ -409,7 +409,7 @@ export function RegularElement(node, context) {
 			b.block([
 				...child_state.init,
 				...element_state.init,
-				child_state.update.length > 0 ? build_render_statement(child_state.update) : b.empty,
+				child_state.update.length > 0 ? build_render_statement(child_state) : b.empty,
 				...child_state.after_update,
 				...element_state.after_update
 			])
@@ -536,7 +536,10 @@ function build_element_attribute_update_assignment(
 	const name = get_attribute_name(element, attribute);
 	const is_svg = context.state.metadata.namespace === 'svg' || element.name === 'svg';
 	const is_mathml = context.state.metadata.namespace === 'mathml';
-	let { has_call, value } = build_attribute_value(attribute.value, context);
+
+	let { value, has_state } = build_attribute_value(attribute.value, context, (value) =>
+		get_expression_id(state, value)
+	);
 
 	if (name === 'autofocus') {
 		state.init.push(b.stmt(b.call('$.autofocus', node_id, value)));
@@ -557,15 +560,6 @@ function build_element_attribute_update_assignment(
 			value = b.call('$.clsx', value);
 		}
 
-		if (attribute.metadata.expression.has_state && has_call) {
-			// ensure we're not creating a separate template effect for this so that
-			// potential class directives are added to the same effect and therefore always apply
-			const id = b.id(state.scope.generate('class_derived'));
-			state.init.push(b.const(id, create_derived(state, b.thunk(value))));
-			value = b.call('$.get', id);
-			has_call = false;
-		}
-
 		update = b.stmt(
 			b.call(
 				is_svg ? '$.set_svg_class' : is_mathml ? '$.set_mathml_class' : '$.set_class',
@@ -605,14 +599,6 @@ function build_element_attribute_update_assignment(
 	} else if (is_dom_property(name)) {
 		update = b.stmt(b.assignment('=', b.member(node_id, name), value));
 	} else {
-		if (name === 'style' && attribute.metadata.expression.has_state && has_call) {
-			// ensure we're not creating a separate template effect for this so that
-			// potential style directives are added to the same effect and therefore always apply
-			const id = b.id(state.scope.generate('style_derived'));
-			state.init.push(b.const(id, create_derived(state, b.thunk(value))));
-			value = b.call('$.get', id);
-			has_call = false;
-		}
 		const callee = name.startsWith('xlink') ? '$.set_xlink_attribute' : '$.set_attribute';
 		update = b.stmt(
 			b.call(
@@ -625,12 +611,8 @@ function build_element_attribute_update_assignment(
 		);
 	}
 
-	if (attribute.metadata.expression.has_state) {
-		if (has_call) {
-			state.init.push(build_update(update));
-		} else {
-			state.update.push(update);
-		}
+	if (has_state) {
+		state.update.push(update);
 		return true;
 	} else {
 		state.init.push(update);
@@ -648,7 +630,7 @@ function build_element_attribute_update_assignment(
 function build_custom_element_attribute_update_assignment(node_id, attribute, context) {
 	const state = context.state;
 	const name = attribute.name; // don't lowercase, as we set the element's property, which might be case sensitive
-	let { has_call, value } = build_attribute_value(attribute.value, context);
+	let { value, has_state } = build_attribute_value(attribute.value, context);
 
 	// We assume that noone's going to redefine the semantics of the class attribute on custom elements, i.e. it's still used for CSS classes
 	if (name === 'class' && attribute.metadata.needs_clsx) {
@@ -660,12 +642,10 @@ function build_custom_element_attribute_update_assignment(node_id, attribute, co
 
 	const update = b.stmt(b.call('$.set_custom_element_data', node_id, b.literal(name), value));
 
-	if (attribute.metadata.expression.has_state) {
-		if (has_call) {
-			state.init.push(build_update(update));
-		} else {
-			state.update.push(update);
-		}
+	if (has_state) {
+		// this is different from other updates — it doesn't get grouped,
+		// because set_custom_element_data may not be idempotent
+		state.init.push(b.stmt(b.call('$.template_effect', b.thunk(update.expression))));
 		return true;
 	} else {
 		state.init.push(update);
@@ -685,7 +665,9 @@ function build_custom_element_attribute_update_assignment(node_id, attribute, co
  */
 function build_element_special_value_attribute(element, node_id, attribute, context) {
 	const state = context.state;
-	const { value } = build_attribute_value(attribute.value, context);
+	const { value, has_state } = build_attribute_value(attribute.value, context, (value) =>
+		get_expression_id(state, value)
+	);
 
 	const inner_assignment = b.assignment(
 		'=',
@@ -719,7 +701,7 @@ function build_element_special_value_attribute(element, node_id, attribute, cont
 		state.init.push(b.stmt(b.call('$.init_select', node_id, b.thunk(value))));
 	}
 
-	if (attribute.metadata.expression.has_state) {
+	if (has_state) {
 		const id = state.scope.generate(`${node_id.name}_value`);
 		build_update_assignment(
 			state,

diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js
@@ -3,6 +3,7 @@
 /** @import { ComponentContext } from '../types' */
 import * as b from '../../../../utils/builders.js';
 import { build_attribute_value } from './shared/element.js';
+import { memoize_expression } from './shared/utils.js';
 
 /**
  * @param {AST.SlotElement} node
@@ -29,13 +30,15 @@ export function SlotElement(node, context) {
 		if (attribute.type === 'SpreadAttribute') {
 			spreads.push(b.thunk(/** @type {Expression} */ (context.visit(attribute))));
 		} else if (attribute.type === 'Attribute') {
-			const { value } = build_attribute_value(attribute.value, context);
+			const { value, has_state } = build_attribute_value(attribute.value, context, (value) =>
+				memoize_expression(context.state, value)
+			);
 
 			if (attribute.name === 'name') {
 				name = /** @type {Literal} */ (value);
 				is_default = false;
 			} else if (attribute.name !== 'slot') {
-				if (attribute.metadata.expression.has_state) {
+				if (has_state) {
 					props.push(b.get(attribute.name, [b.return(value)]));
 				} else {
 					props.push(b.init(attribute.name, value));

diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteBoundary.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteBoundary.js
@@ -23,7 +23,7 @@ export function SvelteBoundary(node, context) {
 
 		const expression = /** @type {Expression} */ (context.visit(chunk.expression, context.state));
 
-		if (attribute.metadata.expression.has_state) {
+		if (chunk.metadata.expression.has_state) {
 			props.properties.push(b.get(attribute.name, [b.return(expression)]));
 		} else {
 			props.properties.push(b.init(attribute.name, expression));

diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteElement.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteElement.js
@@ -1,12 +1,8 @@
-/** @import { BlockStatement, Expression, ExpressionStatement, Identifier, ObjectExpression, Statement } from 'estree' */
+/** @import { BlockStatement, Expression, ExpressionStatement, Statement } from 'estree' */
 /** @import { AST } from '#compiler' */
 /** @import { ComponentContext } from '../types' */
-import { dev, is_ignored, locator } from '../../../../state.js';
-import {
-	get_attribute_expression,
-	is_event_attribute,
-	is_text_attribute
-} from '../../../../utils/ast.js';
+import { dev, locator } from '../../../../state.js';
+import { is_text_attribute } from '../../../../utils/ast.js';
 import * as b from '../../../../utils/builders.js';
 import { determine_namespace_for_children } from '../../utils.js';
 import {
@@ -15,7 +11,7 @@ import {
 	build_set_attributes,
 	build_style_directives
 } from './shared/element.js';
-import { build_render_statement, build_update } from './shared/utils.js';
+import { build_render_statement } from './shared/utils.js';
 
 /**
  * @param {AST.SvelteElement} node
@@ -49,9 +45,9 @@ export function SvelteElement(node, context) {
 		state: {
 			...context.state,
 			node: element_id,
-			before_init: [],
 			init: [],
 			update: [],
+			expressions: [],
 			after_update: []
 		}
 	};
@@ -123,7 +119,7 @@ export function SvelteElement(node, context) {
 	/** @type {Statement[]} */
 	const inner = inner_context.state.init;
 	if (inner_context.state.update.length > 0) {
-		inner.push(build_render_statement(inner_context.state.update));
+		inner.push(build_render_statement(inner_context.state));
 	}
 	inner.push(...inner_context.state.after_update);
 	inner.push(

diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js
@@ -5,7 +5,7 @@ import { dev, is_ignored } from '../../../../../state.js';
 import { get_attribute_chunks, object } from '../../../../../utils/ast.js';
 import * as b from '../../../../../utils/builders.js';
 import { create_derived } from '../../utils.js';
-import { build_bind_this, validate_binding } from '../shared/utils.js';
+import { build_bind_this, memoize_expression, validate_binding } from '../shared/utils.js';
 import { build_attribute_value } from '../shared/element.js';
 import { build_event_handler } from './events.js';
 import { determine_slot } from '../../../../../utils/slot.js';
@@ -132,7 +132,13 @@ export function build_component(node, component_name, context, anchor = context.
 		} else if (attribute.type === 'Attribute') {
 			if (attribute.name.startsWith('--')) {
 				custom_css_props.push(
-					b.init(attribute.name, build_attribute_value(attribute.value, context).value)
+					b.init(
+						attribute.name,
+						build_attribute_value(attribute.value, context, (value) =>
+							// TODO put the derived in the local block
+							memoize_expression(context.state, value)
+						).value
+					)
 				);
 				continue;
 			}
@@ -145,9 +151,11 @@ export function build_component(node, component_name, context, anchor = context.
 				has_children_prop = true;
 			}
 
-			const { value } = build_attribute_value(attribute.value, context);
+			const { value, has_state } = build_attribute_value(attribute.value, context, (value) =>
+				memoize_expression(context.state, value)
+			);
 
-			if (attribute.metadata.expression.has_state) {
+			if (has_state) {
 				let arg = value;
 
 				// When we have a non-simple computation, anything other than an Identifier or Member expression,