From 8c4769db374c923c515e988a3f3e7d31e4366f82 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Sun, 31 Mar 2024 15:32:28 +0100 Subject: [PATCH] chore: improve internal performance of effect runtime (#10999) * chore: improve internal performance of effect runtime * add TODOs * add TODOs --- .../src/internal/client/dom/blocks/each.js | 2 + .../src/internal/client/reactivity/effects.js | 20 +-- .../svelte/src/internal/client/runtime.js | 118 ++++++++++-------- 3 files changed, 84 insertions(+), 56 deletions(-) diff --git a/packages/svelte/src/internal/client/dom/blocks/each.js b/packages/svelte/src/internal/client/dom/blocks/each.js index 8ad815a7a7..ec380eee2d 100644 --- a/packages/svelte/src/internal/client/dom/blocks/each.js +++ b/packages/svelte/src/internal/client/dom/blocks/each.js @@ -423,6 +423,8 @@ function reconcile_tracked_array(array, state, anchor, render_fn, flags, keys) { }); } + // TODO: would be good to avoid this closure in the case where we have no + // transitions at all. It would make it far more JIT friendly in the hot cases. pause_effects(to_destroy, () => { state.items = b_items; }); diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 54aba6c07c..fafd3d1442 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -274,9 +274,8 @@ export function destroy_effect(effect) { } } - effect.first = - effect.last = - effect.next = + // `first` and `child` are nulled out in destroy_effect_children + effect.next = effect.prev = effect.teardown = effect.ctx = @@ -318,14 +317,17 @@ export function pause_effect(effect, callback = noop) { export function pause_effects(effects, callback = noop) { /** @type {import('#client').TransitionManager[]} */ var transitions = []; + var length = effects.length; - for (var effect of effects) { - pause_children(effect, transitions, true); + for (var i = 0; i < length; i++) { + pause_children(effects[i], transitions, true); } + // TODO: would be good to avoid this closure in the case where we have no + // transitions at all. It would make it far more JIT friendly in the hot cases. out(transitions, () => { - for (var effect of effects) { - destroy_effect(effect); + for (var i = 0; i < length; i++) { + destroy_effect(effects[i]); } callback(); }); @@ -370,6 +372,8 @@ function pause_children(effect, transitions, local) { var sibling = child.next; var transparent = (child.f & IS_ELSEIF) !== 0 || (child.f & BRANCH_EFFECT) !== 0; // TODO we don't need to call pause_children recursively with a linked list in place + // it's slightly more involved though as we have to account for `transparent` changing + // through the tree. pause_children(child, transitions, transparent ? local : false); child = sibling; } @@ -404,6 +408,8 @@ function resume_children(effect, local) { var sibling = child.next; var transparent = (child.f & IS_ELSEIF) !== 0 || (child.f & BRANCH_EFFECT) !== 0; // TODO we don't need to call resume_children recursively with a linked list in place + // it's slightly more involved though as we have to account for `transparent` changing + // through the tree. resume_children(child, transparent ? local : false); child = sibling; } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index a7373a99b5..a41f8a6f7f 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -484,72 +484,92 @@ export function schedule_effect(signal) { /** * - * This function recursively collects effects in topological order from the starting effect passed in. - * Effects will be collected when they match the filtered bitwise flag passed in only. The collected - * array will be populated with all the effects. - * - * In an ideal world, we could just execute effects as we encounter them using this approach. However, - * this isn't possible due to how effects in Svelte are modelled to be possibly side-effectful. Thus, - * executing an effect might invalidate other parts of the tree, which means this this tree walking function - * will possibly pick up effects that are dirty too soon. + * This function both runs render effects and collects user effects in topological order + * from the starting effect passed in. Effects will be collected when they match the filtered + * bitwise flag passed in only. The collected effects array will be populated with all the user + * effects to be flushed. * * @param {import('./types.js').Effect} effect * @param {number} filter_flags * @param {boolean} shallow - * @param {import('./types.js').Effect[]} collected_user + * @param {import('./types.js').Effect[]} collected_effects * @returns {void} */ -function recursively_process_effects(effect, filter_flags, shallow, collected_user) { - var current_child = effect.first; - var user = []; - - while (current_child !== null) { - var child = current_child; - current_child = child.next; - var flags = child.f; - var is_inactive = (flags & (DESTROYED | INERT)) !== 0; - if (is_inactive) continue; +function process_effects(effect, filter_flags, shallow, collected_effects) { + var current_effect = effect.first; + var effects = []; + + main_loop: while (current_effect !== null) { + var flags = current_effect.f; + // TODO: we probably don't need to check for destroyed as it shouldn't be encountered? + var is_active = (flags & (DESTROYED | INERT)) === 0; var is_branch = flags & BRANCH_EFFECT; var is_clean = (flags & CLEAN) !== 0; + var child = current_effect.first; - if (is_branch) { - // Skip this branch if it's clean - if (is_clean) continue; - set_signal_status(child, CLEAN); - } - - if ((flags & RENDER_EFFECT) !== 0) { + // Skip this branch if it's clean + if (is_active && (!is_branch || !is_clean)) { if (is_branch) { - if (shallow) continue; - // TODO we don't need to call recursively_process_effects recursively with a linked list in place - recursively_process_effects(child, filter_flags, false, collected_user); - } else { - if (check_dirtiness(child)) { - execute_effect(child); + set_signal_status(current_effect, CLEAN); + } + + if ((flags & RENDER_EFFECT) !== 0) { + if (is_branch) { + if (!shallow && child !== null) { + current_effect = child; + continue; + } + } else { + if (check_dirtiness(current_effect)) { + execute_effect(current_effect); + // Child might have been mutated since running the effect + child = current_effect.first; + } + if (!shallow && child !== null) { + current_effect = child; + continue; + } + } + } else if ((flags & EFFECT) !== 0) { + if (is_branch || is_clean) { + if (!shallow && child !== null) { + current_effect = child; + continue; + } + } else { + effects.push(current_effect); } - // TODO we don't need to call recursively_process_effects recursively with a linked list in place - recursively_process_effects(child, filter_flags, false, collected_user); } - } else if ((flags & EFFECT) !== 0) { - if (is_branch || is_clean) { - if (shallow) continue; - // TODO we don't need to call recursively_process_effects recursively with a linked list in place - recursively_process_effects(child, filter_flags, false, collected_user); - } else { - user.push(child); + } + var sibling = current_effect.next; + + if (sibling === null) { + let parent = current_effect.parent; + + while (parent !== null) { + if (effect === parent) { + break main_loop; + } + var parent_sibling = parent.next; + if (parent_sibling !== null) { + current_effect = parent_sibling; + continue main_loop; + } + parent = parent.parent; } } + + current_effect = sibling; } - if (user.length > 0) { + if (effects.length > 0) { if ((filter_flags & EFFECT) !== 0) { - collected_user.push(...user); + collected_effects.push(...effects); } if (!shallow) { - for (var i = 0; i < user.length; i++) { - // TODO we don't need to call recursively_process_effects recursively with a linked list in place - recursively_process_effects(user[i], filter_flags, false, collected_user); + for (var i = 0; i < effects.length; i++) { + process_effects(effects[i], filter_flags, false, collected_effects); } } } @@ -568,7 +588,7 @@ function recursively_process_effects(effect, filter_flags, shallow, collected_us */ function flush_nested_effects(effect, filter_flags, shallow = false) { /** @type {import('#client').Effect[]} */ - var user_effects = []; + var collected_effects = []; var previously_flushing_effect = is_flushing_effect; is_flushing_effect = true; @@ -578,8 +598,8 @@ function flush_nested_effects(effect, filter_flags, shallow = false) { if (effect.first === null && (effect.f & BRANCH_EFFECT) === 0) { flush_queued_effects([effect]); } else { - recursively_process_effects(effect, filter_flags, shallow, user_effects); - flush_queued_effects(user_effects); + process_effects(effect, filter_flags, shallow, collected_effects); + flush_queued_effects(collected_effects); } } finally { is_flushing_effect = previously_flushing_effect;