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

fix: improve performance of scheduling effects #13282

Closed
wants to merge 13 commits into from
5 changes: 5 additions & 0 deletions .changeset/fresh-houses-check.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: improve performance of scheduling effects
15 changes: 13 additions & 2 deletions packages/svelte/src/internal/client/reactivity/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import {
import { set } from './sources.js';
import * as e from '../errors.js';
import { DEV } from 'esm-env';
import { define_property } from '../../shared/utils.js';
import { define_property, noop } from '../../shared/utils.js';
import { get_next_sibling } from '../dom/operations.js';

/**
Expand Down Expand Up @@ -237,7 +237,18 @@ export function inspect_effect(fn) {
* @returns {() => void}
*/
export function effect_root(fn) {
const effect = create_effect(ROOT_EFFECT, fn, true);
const effect = create_effect(
ROOT_EFFECT,
() => {
branch(() => {
// We return a noop if no function is returned to ensure that the branch
// is attached to the effect tree otherwise it will count as inert
return fn() || noop;
});
},
true
);

return () => {
destroy_effect(effect);
};
Expand Down
48 changes: 23 additions & 25 deletions packages/svelte/src/internal/client/render.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
} from './dom/operations.js';
import { HYDRATION_END, HYDRATION_ERROR, HYDRATION_START } from '../../constants.js';
import { push, pop, component_context, active_effect } from './runtime.js';
import { effect_root, branch } from './reactivity/effects.js';
import { effect_root } from './reactivity/effects.js';
import {
hydrate_next,
hydrate_node,
Expand Down Expand Up @@ -225,35 +225,33 @@ function _mount(Component, { target, anchor, props = {}, events, context, intro
var component = undefined;

var unmount = effect_root(() => {
branch(() => {
if (context) {
push({});
var ctx = /** @type {ComponentContext} */ (component_context);
ctx.c = context;
}
if (context) {
push({});
var ctx = /** @type {ComponentContext} */ (component_context);
ctx.c = context;
}

if (events) {
// We can't spread the object or else we'd lose the state proxy stuff, if it is one
/** @type {any} */ (props).$$events = events;
}
if (events) {
// We can't spread the object or else we'd lose the state proxy stuff, if it is one
/** @type {any} */ (props).$$events = events;
}

if (hydrating) {
assign_nodes(/** @type {TemplateNode} */ (anchor), null);
}
if (hydrating) {
assign_nodes(/** @type {TemplateNode} */ (anchor), null);
}

should_intro = intro;
// @ts-expect-error the public typings are not what the actual function looks like
component = Component(anchor, props) || {};
should_intro = true;
should_intro = intro;
// @ts-expect-error the public typings are not what the actual function looks like
component = Component(anchor, props) || {};
should_intro = true;

if (hydrating) {
/** @type {Effect} */ (active_effect).nodes_end = hydrate_node;
}
if (hydrating) {
/** @type {Effect} */ (active_effect).nodes_end = hydrate_node;
}

if (context) {
pop();
}
});
if (context) {
pop();
}

return () => {
for (var event_name of registered_events) {
Expand Down
6 changes: 6 additions & 0 deletions packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,12 @@ export function schedule_effect(signal) {
effect = effect.parent;
var flags = effect.f;

// Branch effects are not part of the reactive signal graph as they can never have
// dependencies. As such, we can use effects can be CLEAN or MAYBE_DIRTY to signal
// that they contain an effect that is dirty and needs visiting in `process_effects`,
// and if not we can skip the branch entirely. This also doubles as being able to
// skip propagation up the graph in `schedule_effect` if we encounter a branch that
// is already MAYBE_DIRTY.
trueadm marked this conversation as resolved.
Show resolved Hide resolved
if ((flags & BRANCH_EFFECT) !== 0) {
if ((flags & CLEAN) === 0) return;
set_signal_status(effect, MAYBE_DIRTY);
Expand Down
24 changes: 12 additions & 12 deletions packages/svelte/src/reactivity/map.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,14 +192,14 @@ test('map handling of undefined values', () => {
render_effect(() => {
log.push(map.get(1));
});
});

flushSync(() => {
map.delete(1);
});
flushSync(() => {
map.delete(1);
});
Comment on lines +197 to +199
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the purpose of these changes? they pass on main already, so what are they testing exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because effect_root didn't create a branch before. Because child effects get attached to their parent after their contents run, the flushSync doesn't do anything as the root has no child effects.


flushSync(() => {
map.set(1, 1);
});
flushSync(() => {
map.set(1, 1);
});

assert.deepEqual(log, [undefined, undefined, 1]);
Expand All @@ -220,14 +220,14 @@ test('not invoking reactivity when value is not in the map after changes', () =>
render_effect(() => {
log.push(map.get(2));
});
});

flushSync(() => {
map.delete(1);
});
flushSync(() => {
map.delete(1);
});

flushSync(() => {
map.set(1, 1);
});
flushSync(() => {
map.set(1, 1);
});

assert.deepEqual(log, [1, undefined, undefined, undefined, 1, undefined]);
Expand Down