From 5a05f6371a994286626a44168cb2c02f8a2ad567 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Wed, 31 Jul 2024 05:02:53 +0200 Subject: [PATCH] chore: perf tweaks for actions/styles/classes (#12654) * chore: perf tweaks for actions/styles/classes - check if we really need to add/remove the class (calling `includes` first is cheaper than always setting/removing it) - check if we really need to update a style (calling `getPropertyValue/setProperty` is expensive) - check if we should call the action's update function (this is not only a perf tweak but also a correctness fix) closes #12652 * changeset --------- Co-authored-by: Rich Harris --- .changeset/sharp-fishes-serve.md | 5 +++ .../3-transform/client/visitors/template.js | 32 ++++++++++++++++--- .../internal/client/dom/elements/actions.js | 6 +++- .../src/internal/client/dom/elements/class.js | 2 ++ .../src/internal/client/dom/elements/style.js | 22 +++++++++---- 5 files changed, 55 insertions(+), 12 deletions(-) create mode 100644 .changeset/sharp-fishes-serve.md diff --git a/.changeset/sharp-fishes-serve.md b/.changeset/sharp-fishes-serve.md new file mode 100644 index 0000000000..89c8063606 --- /dev/null +++ b/.changeset/sharp-fishes-serve.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +feat: perf tweaks for actions/styles/classes diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index 7d2de190e9..dfe3e93743 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -83,8 +83,15 @@ function get_attribute_name(element, attribute, context) { * @param {Identifier} element_id * @param {ComponentContext} context * @param {boolean} is_attributes_reactive + * @param {boolean} force_check Should be `true` if we can't rely on our cached value, because for example there's also a `style` attribute */ -function serialize_style_directives(style_directives, element_id, context, is_attributes_reactive) { +function serialize_style_directives( + style_directives, + element_id, + context, + is_attributes_reactive, + force_check +) { const state = context.state; for (const directive of style_directives) { @@ -99,7 +106,8 @@ function serialize_style_directives(style_directives, element_id, context, is_at element_id, b.literal(directive.name), value, - /** @type {Expression} */ (directive.modifiers.includes('important') ? b.true : undefined) + /** @type {Expression} */ (directive.modifiers.includes('important') ? b.true : undefined), + force_check ? b.true : undefined ) ); @@ -2062,6 +2070,7 @@ export const template_visitors = { let img_might_be_lazy = false; let might_need_event_replaying = false; let has_direction_attribute = false; + let has_style_attribute = false; if (is_custom_element) { // cloneNode is faster, but it does not instantiate the underlying class of the @@ -2080,6 +2089,9 @@ export const template_visitors = { if (attribute.name === 'dir') { has_direction_attribute = true; } + if (attribute.name === 'style') { + has_style_attribute = true; + } if ( (attribute.name === 'value' || attribute.name === 'checked') && !is_text_attribute(attribute) @@ -2229,7 +2241,13 @@ export const template_visitors = { // class/style directives must be applied last since they could override class/style attributes serialize_class_directives(class_directives, node_id, context, is_attributes_reactive); - serialize_style_directives(style_directives, node_id, context, is_attributes_reactive); + serialize_style_directives( + style_directives, + node_id, + context, + is_attributes_reactive, + has_style_attribute || node.metadata.has_spread + ); if (might_need_event_replaying) { context.state.after_update.push(b.stmt(b.call('$.replay_events', node_id))); @@ -2390,7 +2408,13 @@ export const template_visitors = { // class/style directives must be applied last since they could override class/style attributes serialize_class_directives(class_directives, element_id, inner_context, is_attributes_reactive); - serialize_style_directives(style_directives, element_id, inner_context, is_attributes_reactive); + serialize_style_directives( + style_directives, + element_id, + inner_context, + is_attributes_reactive, + true + ); const get_tag = b.thunk(/** @type {Expression} */ (context.visit(node.tag))); diff --git a/packages/svelte/src/internal/client/dom/elements/actions.js b/packages/svelte/src/internal/client/dom/elements/actions.js index f741843681..dad3ce6fef 100644 --- a/packages/svelte/src/internal/client/dom/elements/actions.js +++ b/packages/svelte/src/internal/client/dom/elements/actions.js @@ -1,5 +1,6 @@ /** @import { ActionPayload } from '#client' */ import { effect, render_effect } from '../../reactivity/effects.js'; +import { safe_not_equal } from '../../reactivity/equality.js'; import { deep_read_state, untrack } from '../../runtime.js'; /** @@ -15,6 +16,8 @@ export function action(dom, action, get_value) { if (get_value && payload?.update) { var inited = false; + /** @type {P} */ + var prev = /** @type {any} */ ({}); // initialize with something so it's never equal on first run render_effect(() => { var value = get_value(); @@ -24,7 +27,8 @@ export function action(dom, action, get_value) { // together with actions and mutation, it wouldn't notice the change without a deep read. deep_read_state(value); - if (inited) { + if (inited && safe_not_equal(prev, value)) { + prev = value; /** @type {Function} */ (payload.update)(value); } }); diff --git a/packages/svelte/src/internal/client/dom/elements/class.js b/packages/svelte/src/internal/client/dom/elements/class.js index 7ab725208c..22f3da0f44 100644 --- a/packages/svelte/src/internal/client/dom/elements/class.js +++ b/packages/svelte/src/internal/client/dom/elements/class.js @@ -107,8 +107,10 @@ function to_class(value) { */ export function toggle_class(dom, class_name, value) { if (value) { + if (dom.classList.contains(class_name)) return; dom.classList.add(class_name); } else { + if (!dom.classList.contains(class_name)) return; dom.classList.remove(class_name); } } diff --git a/packages/svelte/src/internal/client/dom/elements/style.js b/packages/svelte/src/internal/client/dom/elements/style.js index 96922bdc5f..dd297ff72b 100644 --- a/packages/svelte/src/internal/client/dom/elements/style.js +++ b/packages/svelte/src/internal/client/dom/elements/style.js @@ -3,15 +3,23 @@ * @param {string} key * @param {string} value * @param {boolean} [important] + * @param {boolean} [force_check] Should be `true` if we can't rely on our cached value, because for example there's also a `style` attribute */ -export function set_style(dom, key, value, important) { - const style = dom.style; - const prev_value = style.getPropertyValue(key); +export function set_style(dom, key, value, important, force_check) { + // @ts-expect-error + var attributes = (dom.__attributes ??= {}); + var style = dom.style; + var style_key = 'style-' + key; + + if (attributes[style_key] === value && (!force_check || style.getPropertyValue(key) === value)) { + return; + } + + attributes[style_key] = value; + if (value == null) { - if (prev_value !== '') { - style.removeProperty(key); - } - } else if (prev_value !== value) { + style.removeProperty(key); + } else { style.setProperty(key, value, important ? 'important' : ''); } }