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

fix: group sync statements (#17977)

Browse Source 
We were just putting each statement into its own promise. Besides this
being bad for perf, it also introduces subtle timing issues - the
execution order of the code could change in bad ways. Fixes #17940
pull/17965/head
Simon H 4 months ago committed by GitHub
parent 425fba33fe
commit 6b33dd2a1e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 184 additions and 74 deletions
Show Stats Download Patch File Download Diff File

5
.changeset/puny-masks-run.md
Unescape View File

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: group sync statements

36
packages/svelte/src/compiler/phases/2-analyze/index.js
Unescape View File

@ -1074,6 +1074,9 @@ function calculate_blockers(instance, analysis) {
let awaited = false;
/** @type {Array<ESTree.Statement | ESTree.VariableDeclarator>} */
let sync_group = [];
// TODO this should probably be attached to the scope?
const promises = b.id('$$promises');
@ -1088,6 +1091,13 @@ function calculate_blockers(instance, analysis) {
binding.blocker = blocker;
}
function flush_sync_group() {
if (sync_group.length === 0) return;
analysis.instance_body.async.push({ nodes: sync_group, has_await: false });
sync_group = [];
}
/**
* Analysis of blockers for functions is deferred until we know which statements are async/blockers
* @type {Array<ESTree.FunctionDeclaration | ESTree.VariableDeclarator>}
@ -1149,6 +1159,9 @@ function calculate_blockers(instance, analysis) {
trace_references(declarator, reads, writes, instance.scope);
// Needs to happen before blocker computation
if (has_await) flush_sync_group();
const blocker = /** @type {NonNullable<Binding['blocker']>} */ (
b.member(promises, b.literal(analysis.instance_body.async.length), true)
);
@ -1161,11 +1174,12 @@ function calculate_blockers(instance, analysis) {
push_declaration(id, blocker);
}
// one declarator per declaration, makes things simpler
analysis.instance_body.async.push({
node: declarator,
has_await
});
if (has_await) {
// one declarator per declaration, makes things simpler
analysis.instance_body.async.push({ nodes: [declarator], has_await: true });
} else {
sync_group.push(declarator);
}
}
}
} else if (awaited) {
@ -1177,6 +1191,9 @@ function calculate_blockers(instance, analysis) {
trace_references(node, reads, writes, instance.scope);
// Needs to happen before blocker computation
if (has_await) flush_sync_group();
const blocker = /** @type {NonNullable<Binding['blocker']>} */ (
b.member(promises, b.literal(analysis.instance_body.async.length), true)
);
@ -1187,15 +1204,20 @@ function calculate_blockers(instance, analysis) {
if (node.type === 'ClassDeclaration') {
push_declaration(node.id, blocker);
analysis.instance_body.async.push({ node, has_await });
}
if (has_await) {
analysis.instance_body.async.push({ nodes: [node], has_await: true });
} else {
analysis.instance_body.async.push({ node, has_await });
sync_group.push(node);
}
} else {
analysis.instance_body.sync.push(node);
}
}
flush_sync_group();
for (const fn of functions) {
/** @type {Set<Binding>} */
const reads_writes = new Set();

124
packages/svelte/src/compiler/phases/3-transform/shared/transform-async.js
Unescape View File

@ -47,64 +47,24 @@ export function transform_body(instance_body, runner, transform) {
// Thunks for the await expressions
if (instance_body.async.length > 0) {
const thunks = instance_body.async.map((s) => {
if (s.node.type === 'VariableDeclarator') {
const visited = /** @type {ESTree.VariableDeclaration | ESTree.EmptyStatement} */ (
transform(b.var(s.node.id, s.node.init))
);
const statements =
visited.type === 'VariableDeclaration'
? visited.declarations.map((node) => {
if (
node.id.type === 'Identifier' &&
(node.id.name.startsWith('$$d') || node.id.name.startsWith('$$array'))
) {
// this is an intermediate declaration created in VariableDeclaration.js;
// subsequent statements depend on it
return b.var(node.id, node.init);
}
return b.stmt(b.assignment('=', node.id, node.init ?? b.void0));
})
: [];
if (statements.length === 1) {
const statement = /** @type {ESTree.ExpressionStatement} */ (statements[0]);
return b.thunk(statement.expression, s.has_await);
}
return b.thunk(b.block(statements), s.has_await);
}
const thunks = instance_body.async.map((entry) => {
/** @type {ESTree.Statement[]} */
const entry_statements = [];
if (s.node.type === 'ClassDeclaration') {
return b.thunk(
b.assignment(
'=',
s.node.id,
/** @type {ESTree.ClassExpression} */ ({ ...s.node, type: 'ClassExpression' })
),
s.has_await
);
for (const node of entry.nodes) {
entry_statements.push(...transform_async_node(node, transform));
}
if (s.node.type === 'ExpressionStatement') {
// the expression may be a $inspect call, which will be transformed into an empty statement
const expression = /** @type {ESTree.Expression | ESTree.EmptyStatement} */ (
transform(s.node.expression)
);
if (expression.type === 'EmptyStatement') {
// Keep indices stable for async sequencing while avoiding array holes in run([...]).
return b.thunk(b.void0, false);
}
if (entry_statements.length === 0) {
// Keep indices stable for async sequencing while avoiding array holes in run([...]).
return b.thunk(b.void0, false);
}
return expression.type === 'AwaitExpression'
? b.thunk(expression, true)
: b.thunk(b.unary('void', expression), s.has_await);
if (entry_statements.length === 1 && entry_statements[0].type === 'ExpressionStatement') {
return b.thunk(entry_statements[0].expression, entry.has_await);
}
return b.thunk(b.block([/** @type {ESTree.Statement} */ (transform(s.node))]), s.has_await);
return b.thunk(b.block(entry_statements), entry.has_await);
});
// TODO get the `$$promises` ID from scope
@ -113,3 +73,63 @@ export function transform_body(instance_body, runner, transform) {
return statements;
}
/**
* @param {ESTree.Statement | ESTree.VariableDeclarator} node
* @param {(node: ESTree.Node) => ESTree.Node} transform
* @returns {ESTree.Statement[]}
*/
function transform_async_node(node, transform) {
if (node.type === 'VariableDeclarator') {
const visited = /** @type {ESTree.VariableDeclaration | ESTree.EmptyStatement} */ (
transform(b.var(node.id, node.init))
);
return visited.type === 'VariableDeclaration'
? visited.declarations.map((node) => {
if (
node.id.type === 'Identifier' &&
(node.id.name.startsWith('$$d') || node.id.name.startsWith('$$array'))
) {
// This intermediate declaration is created in VariableDeclaration.js;
// subsequent statements may depend on it.
return b.var(node.id, node.init);
}
return b.stmt(b.assignment('=', node.id, node.init ?? b.void0));
})
: [];
}
if (node.type === 'ClassDeclaration') {
return [
b.stmt(
b.assignment(
'=',
node.id,
/** @type {ESTree.ClassExpression} */ ({ ...node, type: 'ClassExpression' })
)
)
];
}
if (node.type === 'ExpressionStatement') {
// The expression may be a $inspect call, which will be transformed into an empty statement.
const expression = /** @type {ESTree.Expression | ESTree.EmptyStatement} */ (
transform(node.expression)
);
if (expression.type === 'EmptyStatement') {
return [];
}
if (expression.type === 'AwaitExpression') {
return [b.stmt(expression)];
}
return [b.stmt(b.unary('void', expression))];
}
const statement = /** @type {ESTree.Statement | ESTree.EmptyStatement} */ (transform(node));
return statement.type === 'EmptyStatement' ? [] : [statement];
}

2
packages/svelte/src/compiler/phases/types.d.ts vendored
Unescape View File

@ -131,7 +131,7 @@ export interface ComponentAnalysis extends Analysis {
instance_body: {
hoisted: Array<Statement | ModuleDeclaration>;
sync: Array<Statement | ModuleDeclaration | VariableDeclaration>;
async: Array<{ node: Statement | VariableDeclarator; has_await: boolean }>;
async: Array<{ nodes: Array<Statement | VariableDeclarator>; has_await: boolean }>;
declarations: Array<Identifier>;
};
}

16
packages/svelte/tests/snapshot/samples/async-in-derived/_expected/client/index.svelte.js
Unescape View File

@ -10,13 +10,15 @@ export default function Async_in_derived($$anchor, $$props) {
var $$promises = $.run([
async () => yes1 = await $.async_derived(() => 1),
async () => yes2 = await $.async_derived(async () => foo(await 1)),
() => no1 = $.derived(async () => {
return await 1;
}),
() => no2 = $.derived(() => async () => {
return await 1;
})
() => {
no1 = $.derived(async () => {
return await 1;
});
no2 = $.derived(() => async () => {
return await 1;
});
}
]);
var fragment = $.comment();

16
packages/svelte/tests/snapshot/samples/async-in-derived/_expected/server/index.svelte.js
Unescape View File

@ -8,13 +8,15 @@ export default function Async_in_derived($$renderer, $$props) {
var $$promises = $$renderer.run([
async () => yes1 = await $.async_derived(() => 1),
async () => yes2 = await $.async_derived(async () => foo(await 1)),
() => no1 = $.derived(async () => {
return await 1;
}),
() => no2 = $.derived(() => async () => {
return await 1;
})
() => {
no1 = $.derived(async () => {
return await 1;
});
no2 = $.derived(() => async () => {
return await 1;
});
}
]);
if (true) {

3
packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_config.js
Unescape View File

@ -0,0 +1,3 @@
import { test } from '../../test';
export default test({ compileOptions: { experimental: { async: true } } });

26
packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_expected/client/index.svelte.js
Unescape View File

@ -0,0 +1,26 @@
import 'svelte/internal/disclose-version';
import 'svelte/internal/flags/async';
import * as $ from 'svelte/internal/client';
export default function Async_top_level_group_sync_run($$anchor) {
var a,
// these should be grouped into one, having an async tick inbetween
// would change how the code runs and could introduce subtle timing bugs
b,
c;
var $$promises = $.run([
async () => a = await Promise.resolve(1),
() => {
b = a + 1;
c = b + 1;
}
]);
$.next();
var text = $.text();
$.template_effect(() => $.set_text(text, c), void 0, void 0, [$$promises[1]]);
$.append($$anchor, text);
}

21
packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_expected/server/index.svelte.js
Unescape View File

@ -0,0 +1,21 @@
import 'svelte/internal/flags/async';
import * as $ from 'svelte/internal/server';
export default function Async_top_level_group_sync_run($$renderer) {
var a,
// these should be grouped into one, having an async tick inbetween
// would change how the code runs and could introduce subtle timing bugs
b,
c;
var $$promises = $$renderer.run([
async () => a = await Promise.resolve(1),
() => {
b = a + 1;
c = b + 1;
}
]);
$$renderer.push(`<!---->`);
$$renderer.async([$$promises[1]], ($$renderer) => $$renderer.push(() => $.escape(c)));
}

9
packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/index.svelte
Unescape View File

@ -0,0 +1,9 @@
<script>
let a = await Promise.resolve(1);
// these should be grouped into one, having an async tick inbetween
// would change how the code runs and could introduce subtle timing bugs
let b = a + 1;
let c = b + 1;
</script>
{c}
Write Preview
Loading…
Cancel
Save