msb-public
/
svelte
mirror of https://github.com/sveltejs/svelte
1
0
Code Issues Projects Releases Wiki Activity

fix: remove `WAS_MARKED` flag in favor of `Set` (#18127)

Browse Source 
Removes the `WAS_MARKED` logic in favor of a simple `Set` heuristic: If
`mark_reactions` goes beyond a certain count, initialize it, otherwise
keep it `null`. Balances the common case of not having many transitive
dependencies with the edge case of cyclic dependencies as was seen in
#16658 (I rechecked that reproduction and it remains fast with this
change).

I ran the benchmark against this and it doesn't hit the `Seen` heuristic
once (i.e. even the benchmark doesn't have this extreme level of
dependencies where it becomes noticeable).

Fixes #18123, supersedes #18124

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>
pull/18772/merge
Simon H 23 hours ago committed by GitHub
parent 5895c637b0
commit d16099ef0f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 38 additions and 63 deletions
Show Stats Download Patch File Download Diff File

5
.changeset/grumpy-pens-press.md
Unescape View File

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: remove `WAS_MARKED` flag in favor of `Set`

5
benchmarking/benchmarks/reactivity/index.js
Unescape View File

@ -13,7 +13,7 @@ import {
sbench_create_4to1,
sbench_create_signals
} from './sbench.js';
import { fileURLToPath } from 'node:url';
import { fileURLToPath, pathToFileURL } from 'node:url';
import { create_test } from './util.js';
// This benchmark has been adapted from the js-reactivity-benchmark (https://github.com/milomg/js-reactivity-benchmark)
@ -39,7 +39,8 @@ for (const file of fs.readdirSync(`${dirname}/tests`)) {
const name = file.replace('.bench.js', '');
const module = await import(`${dirname}/tests/${file}`);
const module_url = pathToFileURL(path.join(dirname, 'tests', file));
const module = await import(module_url.href);
const { owned, unowned } = create_test(name, module.default);
reactivity_benchmarks.push(owned, unowned);

9
packages/svelte/src/internal/client/constants.js
Unescape View File

@ -51,15 +51,6 @@ export const EFFECT_PRESERVED = 1 << 19;
export const USER_EFFECT = 1 << 20;
export const EFFECT_OFFSCREEN = 1 << 25;
// Flags exclusive to deriveds
/**
* Tells that we marked this derived and its reactions as visited during the "mark as (maybe) dirty"-phase.
* Will be lifted during execution of the derived and during checking its dirty state (both are necessary
* because a derived might be checked but not executed). This is a pure performance optimization flag and
* should not be used for any other purpose!
*/
export const WAS_MARKED = 1 << 16;
// Flags used for async
export const REACTION_IS_UPDATING = 1 << 21;
export const ASYNC = 1 << 22;

2
packages/svelte/src/internal/client/dev/debug.js
Unescape View File

@ -15,7 +15,6 @@ import {
MAYBE_DIRTY,
RENDER_EFFECT,
ROOT_EFFECT,
WAS_MARKED,
MANAGED_EFFECT
} from '#client/constants';
import { snapshot } from '../../shared/clone.js';
@ -204,7 +203,6 @@ export function log_reactions(signal) {
if ((flags & DIRTY) !== 0) names.push('DIRTY');
if ((flags & MAYBE_DIRTY) !== 0) names.push('MAYBE_DIRTY');
if ((flags & CONNECTED) !== 0) names.push('CONNECTED');
if ((flags & WAS_MARKED) !== 0) names.push('WAS_MARKED');
if ((flags & INERT) !== 0) names.push('INERT');
if ((flags & DESTROYED) !== 0) names.push('DESTROYED');

3
packages/svelte/src/internal/client/reactivity/deriveds.js
Unescape View File

@ -9,7 +9,6 @@ import {
EFFECT_PRESERVED,
STALE_REACTION,
ASYNC,
WAS_MARKED,
DESTROYED,
CLEAN,
REACTION_RAN,
@ -365,7 +364,6 @@ export function execute_derived(derived) {
stack.push(derived);
derived.f &= ~WAS_MARKED;
destroy_derived_effects(derived);
value = update_reaction(derived);
} finally {
@ -375,7 +373,6 @@ export function execute_derived(derived) {
}
} else {
try {
derived.f &= ~WAS_MARKED;
destroy_derived_effects(derived);
value = update_reaction(derived);
} finally {

44
packages/svelte/src/internal/client/reactivity/sources.js
Unescape View File

@ -25,10 +25,7 @@ import {
MAYBE_DIRTY,
BLOCK_EFFECT,
ROOT_EFFECT,
ASYNC,
WAS_MARKED,
CONNECTED,
REACTION_IS_UPDATING
ASYNC
} from '#client/constants';
import * as e from '../errors.js';
import { legacy_mode_flag, tracing_mode_flag } from '../../flags/index.js';
@ -171,6 +168,17 @@ export function set(source, value, should_proxy = false) {
return internal_set(source, new_value, legacy_updates);
}
/**
* A set of signals we have already seen while traversing in mark_reactions.
* Not always set to balance the common case of sources only having a couple
* of (transitive) dependencies (where always creating a Set would be bad for perf)
* with the edge case of extremely deep or wide dependency arrays with cycles.
* @type {Set<any> | null}
*/
var seen = null;
/** Number of transitive dependencies, see {@link seen} for more info */
var count_deps = 0;
/**
* @template V
* @param {Source<V>} source
@ -240,7 +248,10 @@ export function internal_set(source, value, updated_during_traversal = null) {
// For debugging, in case you want to know which reactions are being scheduled:
// log_reactions(source);
seen = null;
count_deps = 0;
mark_reactions(source, DIRTY, updated_during_traversal);
seen = null;
// It's possible that the current reaction might not have up-to-date dependencies
// whilst it's actively running. So in the case of ensuring it registers the reaction
@ -347,6 +358,18 @@ function mark_reactions(signal, status, updated_during_traversal) {
var runes = is_runes();
var length = reactions.length;
count_deps += length;
// Activate the `seen` Set if we think from the unusually high number of deps that
// there might be cycles in the graph, to avoid repeated lookups for reactions
// Example: https://github.com/sveltejs/svelte/issues/16658 has a graph with one source
// reaching ~10000 distinct deriveds/effects each, resulting in 65 million walks through repeated visits.
if (count_deps > 100000 && seen === null) seen = new Set();
if (seen !== null) {
if (seen.has(signal)) return;
seen.add(signal);
}
for (var i = 0; i < length; i++) {
var reaction = reactions[i];
var flags = reaction.f;
@ -370,18 +393,7 @@ function mark_reactions(signal, status, updated_during_traversal) {
var derived = /** @type {Derived} */ (reaction);
batch_values?.delete(derived);
if ((flags & WAS_MARKED) === 0) {
// Only connected deriveds being executed outside the update cycle can be reliably unmarked right away
if (
flags & CONNECTED &&
(active_effect === null || (active_effect.f & REACTION_IS_UPDATING) === 0)
) {
reaction.f |= WAS_MARKED;
}
mark_reactions(derived, MAYBE_DIRTY, updated_during_traversal);
}
mark_reactions(derived, MAYBE_DIRTY, updated_during_traversal);
} else if (not_dirty) {
var effect = /** @type {Effect} */ (reaction);

25
packages/svelte/src/internal/client/reactivity/utils.js
Unescape View File

@ -1,24 +1,7 @@
/** @import { Derived, Effect, Value } from '#client' */
import { CLEAN, DERIVED, DIRTY, MAYBE_DIRTY, WAS_MARKED } from '#client/constants';
/** @import { Effect } from '#client' */
import { CLEAN, DIRTY, MAYBE_DIRTY } from '#client/constants';
import { set_signal_status } from './status.js';
/**
* @param {Value[] | null} deps
*/
function clear_marked(deps) {
if (deps === null) return;
for (const dep of deps) {
if ((dep.f & DERIVED) === 0 || (dep.f & WAS_MARKED) === 0) {
continue;
}
dep.f ^= WAS_MARKED;
clear_marked(/** @type {Derived} */ (dep).deps);
}
}
/**
* @param {Effect} effect
* @param {Set<Effect>} dirty_effects
@ -31,10 +14,6 @@ export function defer_effect(effect, dirty_effects, maybe_dirty_effects) {
maybe_dirty_effects.add(effect);
}
// Since we're not executing these effects now, we need to clear any WAS_MARKED flags
// so that other batches can correctly reach these effects during their own traversal
clear_marked(effect.deps);
// mark as clean so they get scheduled if they depend on pending async state
set_signal_status(effect, CLEAN);
}

8
packages/svelte/src/internal/client/runtime.js
Unescape View File

@ -21,7 +21,6 @@ import {
REACTION_IS_UPDATING,
STALE_REACTION,
ERROR_VALUE,
WAS_MARKED,
MANAGED_EFFECT,
REACTION_RAN
} from './constants.js';
@ -164,10 +163,6 @@ export function is_dirty(reaction) {
return true;
}
if (flags & DERIVED) {
reaction.f &= ~WAS_MARKED;
}
if ((flags & MAYBE_DIRTY) !== 0) {
var dependencies = /** @type {Value[]} */ (reaction.deps);
var length = dependencies.length;
@ -408,11 +403,8 @@ function remove_reaction(signal, dependency) {
) {
var derived = /** @type {Derived} */ (dependency);
// If we are working with a derived that is owned by an effect, then mark it as being
// disconnected and remove the mark flag, as it cannot be reliably removed otherwise
if ((derived.f & CONNECTED) !== 0) {
derived.f ^= CONNECTED;
derived.f &= ~WAS_MARKED;
}
// In a fork it's possible that a derived is executed and gets reactions, then commits, but is

Write Preview
Loading…
Cancel
Save