Skip to content

Commit

Permalink
Merge pull request #3438 from sveltejs/gh-2355
Browse files Browse the repository at this point in the history
more conservative if block updates
  • Loading branch information
Rich-Harris authored Aug 22, 2019
2 parents 415dc5f + 393757d commit 38001ce
Show file tree
Hide file tree
Showing 10 changed files with 211 additions and 42 deletions.
44 changes: 25 additions & 19 deletions src/compiler/compile/render_dom/wrappers/AwaitBlock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,29 +188,35 @@ export default class AwaitBlockWrapper extends Wrapper {
conditions.push(
`(${dependencies.map(dep => `'${dep}' in changed`).join(' || ')})`
);
}

conditions.push(
`${promise} !== (${promise} = ${snippet})`,
`@handle_promise(${promise}, ${info})`
);
conditions.push(
`${promise} !== (${promise} = ${snippet})`,
`@handle_promise(${promise}, ${info})`
);

block.builders.update.add_line(
`${info}.ctx = ctx;`
);
block.builders.update.add_line(
`${info}.ctx = ctx;`
);

if (this.pending.block.has_update_method) {
block.builders.update.add_block(deindent`
if (${conditions.join(' && ')}) {
// nothing
} else {
${info}.block.p(changed, @assign(@assign({}, ctx), ${info}.resolved));
}
`);
if (this.pending.block.has_update_method) {
block.builders.update.add_block(deindent`
if (${conditions.join(' && ')}) {
// nothing
} else {
${info}.block.p(changed, @assign(@assign({}, ctx), ${info}.resolved));
}
`);
} else {
block.builders.update.add_block(deindent`
${conditions.join(' && ')}
`);
}
} else {
block.builders.update.add_block(deindent`
${conditions.join(' && ')}
`);
if (this.pending.block.has_update_method) {
block.builders.update.add_block(deindent`
${info}.block.p(changed, @assign(@assign({}, ctx), ${info}.resolved));
`);
}
}

if (this.pending.block.has_outro_method) {
Expand Down
79 changes: 59 additions & 20 deletions src/compiler/compile/render_dom/wrappers/IfBlock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import create_debugging_comment from './shared/create_debugging_comment';
import ElseBlock from '../../nodes/ElseBlock';
import FragmentWrapper from './Fragment';
import deindent from '../../utils/deindent';
import { walk } from 'estree-walker';

function is_else_if(node: ElseBlock) {
return (
Expand All @@ -17,7 +18,9 @@ function is_else_if(node: ElseBlock) {
class IfBlockBranch extends Wrapper {
block: Block;
fragment: FragmentWrapper;
condition: string;
dependencies?: string[];
condition?: string;
snippet?: string;
is_dynamic: boolean;

var = null;
Expand All @@ -32,13 +35,35 @@ class IfBlockBranch extends Wrapper {
) {
super(renderer, block, parent, node);

this.condition = (node as IfBlock).expression && (node as IfBlock).expression.render(block);
const { expression } = (node as IfBlock);
const is_else = !expression;

if (expression) {
const dependencies = expression.dynamic_dependencies();

// TODO is this the right rule? or should any non-reference count?
// const should_cache = !is_reference(expression.node, null) && dependencies.length > 0;
let should_cache = false;
walk(expression.node, {
enter(node) {
if (node.type === 'CallExpression' || node.type === 'NewExpression') {
should_cache = true;
}
}
});

if (should_cache) {
this.condition = block.get_unique_name(`show_if`);
this.snippet = expression.render(block);
this.dependencies = dependencies;
} else {
this.condition = expression.render(block);
}
}

this.block = block.child({
comment: create_debugging_comment(node, parent.renderer.component),
name: parent.renderer.component.get_unique_name(
(node as IfBlock).expression ? `create_if_block` : `create_else_block`
)
name: parent.renderer.component.get_unique_name(is_else ? `create_else_block` : `create_if_block`)
});

this.fragment = new FragmentWrapper(renderer, this.block, node.children, parent, strip_whitespace, next_sibling);
Expand Down Expand Up @@ -157,6 +182,10 @@ export default class IfBlockWrapper extends Wrapper {
const detaching = (parent_node && parent_node !== '@_document.head') ? '' : 'detaching';

if (this.node.else) {
this.branches.forEach(branch => {
if (branch.snippet) block.add_variable(branch.condition);
});

if (has_outros) {
this.render_compound_with_outros(block, parent_node, parent_nodes, dynamic, vars, detaching);

Expand Down Expand Up @@ -212,16 +241,18 @@ export default class IfBlockWrapper extends Wrapper {

/* eslint-disable @typescript-eslint/indent,indent */
block.builders.init.add_block(deindent`
function ${select_block_type}(ctx) {
${this.branches
.map(({ condition, block }) => `${condition ? `if (${condition}) ` : ''}return ${block.name};`)
.join('\n')}
function ${select_block_type}(changed, ctx) {
${this.branches.map(({ dependencies, condition, snippet, block }) => condition
? deindent`
${dependencies && `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})`}
if (${condition}) return ${block.name};`
: `return ${block.name};`)}
}
`);
/* eslint-enable @typescript-eslint/indent,indent */

block.builders.init.add_block(deindent`
var ${current_block_type} = ${select_block_type}(ctx);
var ${current_block_type} = ${select_block_type}(null, ctx);
var ${name} = ${current_block_type_and}${current_block_type}(ctx);
`);

Expand All @@ -245,15 +276,15 @@ export default class IfBlockWrapper extends Wrapper {

if (dynamic) {
block.builders.update.add_block(deindent`
if (${current_block_type} === (${current_block_type} = ${select_block_type}(ctx)) && ${name}) {
if (${current_block_type} === (${current_block_type} = ${select_block_type}(changed, ctx)) && ${name}) {
${name}.p(changed, ctx);
} else {
${change_block}
}
`);
} else {
block.builders.update.add_block(deindent`
if (${current_block_type} !== (${current_block_type} = ${select_block_type}(ctx))) {
if (${current_block_type} !== (${current_block_type} = ${select_block_type}(changed, ctx))) {
${change_block}
}
`);
Expand Down Expand Up @@ -293,23 +324,25 @@ export default class IfBlockWrapper extends Wrapper {
var ${if_blocks} = [];
function ${select_block_type}(ctx) {
${this.branches
.map(({ condition }, i) => `${condition ? `if (${condition}) ` : ''}return ${i};`)
.join('\n')}
function ${select_block_type}(changed, ctx) {
${this.branches.map(({ dependencies, condition, snippet }, i) => condition
? deindent`
${dependencies && `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})`}
if (${condition}) return ${String(i)};`
: `return ${i};`)}
${!has_else && `return -1;`}
}
`);
/* eslint-enable @typescript-eslint/indent,indent */

if (has_else) {
block.builders.init.add_block(deindent`
${current_block_type_index} = ${select_block_type}(ctx);
${current_block_type_index} = ${select_block_type}(null, ctx);
${name} = ${if_blocks}[${current_block_type_index}] = ${if_block_creators}[${current_block_type_index}](ctx);
`);
} else {
block.builders.init.add_block(deindent`
if (~(${current_block_type_index} = ${select_block_type}(ctx))) {
if (~(${current_block_type_index} = ${select_block_type}(null, ctx))) {
${name} = ${if_blocks}[${current_block_type_index}] = ${if_block_creators}[${current_block_type_index}](ctx);
}
`);
Expand Down Expand Up @@ -363,7 +396,7 @@ export default class IfBlockWrapper extends Wrapper {
if (dynamic) {
block.builders.update.add_block(deindent`
var ${previous_block_index} = ${current_block_type_index};
${current_block_type_index} = ${select_block_type}(ctx);
${current_block_type_index} = ${select_block_type}(changed, ctx);
if (${current_block_type_index} === ${previous_block_index}) {
${if_current_block_type_index}${if_blocks}[${current_block_type_index}].p(changed, ctx);
} else {
Expand All @@ -373,7 +406,7 @@ export default class IfBlockWrapper extends Wrapper {
} else {
block.builders.update.add_block(deindent`
var ${previous_block_index} = ${current_block_type_index};
${current_block_type_index} = ${select_block_type}(ctx);
${current_block_type_index} = ${select_block_type}(changed, ctx);
if (${current_block_type_index} !== ${previous_block_index}) {
${change_block}
}
Expand All @@ -395,6 +428,8 @@ export default class IfBlockWrapper extends Wrapper {
) {
const branch = this.branches[0];

if (branch.snippet) block.add_variable(branch.condition, branch.snippet);

block.builders.init.add_block(deindent`
var ${name} = (${branch.condition}) && ${branch.block.name}(ctx);
`);
Expand Down Expand Up @@ -431,6 +466,10 @@ export default class IfBlockWrapper extends Wrapper {
}
`;

if (branch.snippet) {
block.builders.update.add_block(`if (${branch.dependencies.map(n => `changed.${n}`).join(' || ')}) ${branch.condition} = ${branch.snippet}`);
}

// no `p()` here — we don't want to update outroing nodes,
// as that will typically result in glitching
if (branch.block.has_outro_method) {
Expand Down
6 changes: 3 additions & 3 deletions test/js/samples/if-block-no-update/expected.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,12 @@ function create_if_block(ctx) {
function create_fragment(ctx) {
var if_block_anchor;

function select_block_type(ctx) {
function select_block_type(changed, ctx) {
if (ctx.foo) return create_if_block;
return create_else_block;
}

var current_block_type = select_block_type(ctx);
var current_block_type = select_block_type(null, ctx);
var if_block = current_block_type(ctx);

return {
Expand All @@ -77,7 +77,7 @@ function create_fragment(ctx) {
},

p(changed, ctx) {
if (current_block_type !== (current_block_type = select_block_type(ctx))) {
if (current_block_type !== (current_block_type = select_block_type(changed, ctx))) {
if_block.d(1);
if_block = current_block_type(ctx);
if (if_block) {
Expand Down
16 changes: 16 additions & 0 deletions test/runtime/samples/await-conservative-update/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { sleep } from './sleep.js';

export default {
html: `
<p>loading...</p>
`,

test({ assert, component, target }) {
return sleep(50).then(() => {
assert.htmlEqual(target.innerHTML, `
<p>the answer is 42</p>
<p>count: 1</p>
`);
});
}
};
19 changes: 19 additions & 0 deletions test/runtime/samples/await-conservative-update/main.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<script>
import { sleep } from './sleep.js';
let count = 0;
const get_promise = () => {
return sleep(10).then(() => {
count += 1;
return 42;
});
};
</script>

{#await get_promise()}
<p>loading...</p>
{:then value}
<p>the answer is {value}</p>
<p>count: {count}</p>
{/await}
11 changes: 11 additions & 0 deletions test/runtime/samples/await-conservative-update/sleep.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export let stopped = false;

export const stop = () => stopped = true;

export const sleep = ms => new Promise(f => {
if (stopped) return;
setTimeout(() => {
if (stopped) return;
f();
}, ms);
});
22 changes: 22 additions & 0 deletions test/runtime/samples/if-block-conservative-update/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
let count = 0;

export default {
props: {
foo: 'potato',
fn: () => {
count += 1;
return true;
}
},

html: `<p>potato</p>`,

test({ assert, component, target }) {
assert.equal(count, 1);

component.foo = 'soup';
assert.equal(count, 1);

assert.htmlEqual(target.innerHTML, `<p>soup</p>`);
}
}
8 changes: 8 additions & 0 deletions test/runtime/samples/if-block-conservative-update/main.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<script>
export let fn;
export let foo;
</script>

{#if fn()}
<p>{foo}</p>
{/if}
37 changes: 37 additions & 0 deletions test/runtime/samples/if-block-else-conservative-update/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
let a = true;
let count_a = 0;
let count_b = 0;

export default {
props: {
foo: 'potato',
fn: () => {
count_a += 1;
return a;
},
other_fn: () => {
count_b += 1;
return true;
}
},

html: `<p>potato</p>`,

test({ assert, component, target }) {
assert.equal(count_a, 1);
assert.equal(count_b, 0);

a = false;
component.foo = 'soup';
assert.equal(count_a, 2);
assert.equal(count_b, 1);

assert.htmlEqual(target.innerHTML, `<p>SOUP</p>`);

component.foo = 'salad';
assert.equal(count_a, 3);
assert.equal(count_b, 1);

assert.htmlEqual(target.innerHTML, `<p>SALAD</p>`);
}
}
11 changes: 11 additions & 0 deletions test/runtime/samples/if-block-else-conservative-update/main.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<script>
export let fn;
export let other_fn;
export let foo;
</script>

{#if fn(foo)}
<p>{foo}</p>
{:else if other_fn()}
<p>{foo.toUpperCase()}</p>
{/if}

0 comments on commit 38001ce

Please sign in to comment.