Skip to content

Commit

Permalink
first pass at tracking mutation to avoid unnecessary update code - sv…
Browse files Browse the repository at this point in the history
…eltejs#1952

also tracks mutation on child refs of const declarations - sveltejs#1917
  • Loading branch information
evs-chris committed Jan 13, 2019
1 parent 8e9f37a commit e85222a
Show file tree
Hide file tree
Showing 8 changed files with 148 additions and 10 deletions.
37 changes: 37 additions & 0 deletions src/compile/Component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export default class Component {
reactive_declarations: Array<{ assignees: Set<string>, dependencies: Set<string>, snippet: string }> = [];
reactive_declaration_nodes: Set<Node> = new Set();
has_reactive_assignments = false;
mutable_props: Set<string> = new Set();

indirectDependencies: Map<string, Set<string>> = new Map();
template_references: Set<string> = new Set();
Expand Down Expand Up @@ -169,6 +170,7 @@ export default class Component {
if (!this.instance_script) {
const props = [...this.template_references];
this.declarations.push(...props);
addToSet(this.mutable_props, this.template_references);
addToSet(this.writable_declarations, this.template_references);
addToSet(this.userVars, this.template_references);

Expand All @@ -177,6 +179,9 @@ export default class Component {
as: name
}));
}

// tell the root fragment scope about all of the mutable names we know from the script
this.mutable_props.forEach(name => this.fragment.scope.mutables.add(name));
}

addSourcemapLocations(node: Node) {
Expand Down Expand Up @@ -405,6 +410,7 @@ export default class Component {
node.declaration.declarations.forEach(declarator => {
extractNames(declarator.id).forEach(name => {
exports.push({ name, as: name });
this.mutable_props.add(name);
});
});
} else {
Expand Down Expand Up @@ -537,13 +543,43 @@ export default class Component {
this.userVars.add(name);
});

this.track_mutations();
this.extract_imports_and_exports(script.content, this.imports, this.props);
this.hoist_instance_declarations();
this.extract_reactive_declarations();
this.extract_reactive_store_references();
this.javascript = this.extract_javascript(script);
}

// TODO merge this with other walks that are independent
track_mutations() {
const component = this;
let { instance_scope: scope, instance_scope_map: map } = this;

walk(this.instance_script.content, {
enter(node, parent) {
let names;
if (map.has(node)) {
scope = map.get(node);
}

if (node.type === 'AssignmentExpression') {
names = node.left.type === 'MemberExpression'
? [getObject(node.left).name]
: extractNames(node.left);
} else if (node.type === 'UpdateExpression') {
names = [getObject(node.argument).name];
}

if (names) {
names.forEach(name => {
if (scope.has(name)) component.mutable_props.add(name);
});
}
}
})
}

extract_reactive_store_references() {
// TODO this pattern happens a lot... can we abstract it
// (or better still, do fewer AST walks)?
Expand Down Expand Up @@ -586,6 +622,7 @@ export default class Component {
const exported = new Set();
this.props.forEach(prop => {
exported.add(prop.name);
this.mutable_props.add(prop.name);
});

const coalesced_declarations = [];
Expand Down
3 changes: 3 additions & 0 deletions src/compile/nodes/Binding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ export default class Binding extends Node {
const { name } = getObject(this.expression.node);
this.isContextual = scope.names.has(name);

// make sure we track this as a mutable ref
scope.setMutable(name);

if (this.expression.node.type === 'MemberExpression') {
prop = `[✂${this.expression.node.property.start}-${this.expression.node.property.end}✂]`;
if (!this.expression.node.computed) prop = `'${prop}'`;
Expand Down
22 changes: 18 additions & 4 deletions src/compile/nodes/shared/Expression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,16 @@ export default class Expression {
const expression = this;
let function_expression;

function add_dependency(name) {
function add_dependency(name, deep = false) {
dependencies.add(name);

if (!function_expression) {
// dynamic_dependencies is used to create `if (changed.foo || ...)`
// conditions — it doesn't apply if the dependency is inside a
// function, and it only applies if the dependency is writable
// or a sub-path of a non-writable
if (component.instance_script) {
if (component.writable_declarations.has(name) || name[0] === '$') {
if (component.writable_declarations.has(name) || name[0] === '$' || (component.userVars.has(name) && deep)) {
dynamic_dependencies.add(name);
}
} else {
Expand Down Expand Up @@ -146,16 +147,29 @@ export default class Expression {

contextual_dependencies.add(name);

template_scope.dependenciesForName.get(name).forEach(add_dependency);
template_scope.dependenciesForName.get(name).forEach(name => add_dependency(name, true));
} else {
add_dependency(name);
add_dependency(name, nodes.length > 1);
component.template_references.add(name);

component.warn_if_undefined(nodes[0], template_scope, true);
}

this.skip();
}

// track any assignments from template expressions as mutable
if (function_expression) {
if (node.type === 'AssignmentExpression') {
const names = node.left.type === 'MemberExpression'
? [getObject(node.left).name]
: extractNames(node.left);
names.forEach(name => template_scope.setMutable(name));
} else if (node.type === 'UpdateExpression') {
const { name } = getObject(node.argument);
template_scope.setMutable(name);
}
}
},

leave(node) {
Expand Down
24 changes: 23 additions & 1 deletion src/compile/nodes/shared/TemplateScope.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
export default class TemplateScope {
names: Set<string>;
dependenciesForName: Map<string, string>;
mutables: Set<string>;
parent?: TemplateScope;

constructor(parent?: TemplateScope) {
this.parent = parent;
this.names = new Set(parent ? parent.names : []);
this.dependenciesForName = new Map(parent ? parent.dependenciesForName : []);
this.mutables = new Set();
}

add(name, dependencies) {
Expand All @@ -14,6 +18,24 @@ export default class TemplateScope {
}

child() {
return new TemplateScope(this);
const child = new TemplateScope(this);
return child;
}

setMutable(name: string) {
if (this.names.has(name)) this.mutables.add(name);
else if (this.parent) this.parent.setMutable(name);
else this.mutables.add(name);
}

containsMutable(names: Iterable<string>) {
for (const name of names) {
if (name[0] === '$') return true;
if (this.mutables.has(name)) return true;
else if (this.dependenciesForName.has(name) && this.containsMutable(this.dependenciesForName.get(name))) return true;
}

if (this.parent) return this.parent.containsMutable(names);
else return false;
}
}
3 changes: 2 additions & 1 deletion src/compile/render-dom/wrappers/Element/Attribute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,8 @@ export default class AttributeWrapper {
updater = `${method}(${element.var}, "${name}", ${shouldCache ? last : value});`;
}

if (this.node.dependencies.size || isSelectValueAttribute) {
// only add an update if mutations are involved (or it's a select?)
if (this.node.parent.scope.containsMutable(this.node.dependencies) || isSelectValueAttribute) {
const dependencies = Array.from(this.node.dependencies);
const changedCheck = (
(block.hasOutros ? `!#current || ` : '') +
Expand Down
11 changes: 7 additions & 4 deletions src/compile/render-dom/wrappers/shared/Tag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,13 @@ export default class Tag extends Wrapper {
: updateCachedValue
: changedCheck;

block.builders.update.addConditional(
condition,
update(content)
);
// only update if there's a mutation involved
if (this.node.expression.template_scope.containsMutable(dependencies)) {
block.builders.update.addConditional(
condition,
update(content)
);
}
}

return { init: content };
Expand Down
53 changes: 53 additions & 0 deletions test/js/samples/non-mutable-reference/expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/* generated by Svelte vX.Y.Z */
import { SvelteComponent as SvelteComponent_1, append, createElement, createText, detachNode, init, insert, noop, run, safe_not_equal } from "svelte/internal";

function create_fragment($$, ctx) {
var h1, text0, text1, text2, current;

return {
c() {
h1 = createElement("h1");
text0 = createText("Hello ");
text1 = createText(ctx.name);
text2 = createText("!");
},

m(target, anchor) {
insert(target, h1, anchor);
append(h1, text0);
append(h1, text1);
append(h1, text2);
current = true;
},

p: noop,

i(target, anchor) {
if (current) return;
this.m(target, anchor);
},

o: run,

d(detach) {
if (detach) {
detachNode(h1);
}
}
};
}

function instance($$self) {
let name = 'world';

return { name };
}

class SvelteComponent extends SvelteComponent_1 {
constructor(options) {
super();
init(this, options, instance, create_fragment, safe_not_equal);
}
}

export default SvelteComponent;
5 changes: 5 additions & 0 deletions test/js/samples/non-mutable-reference/input.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<script>
let name = 'world';
</script>

<h1>Hello {name}!</h1>

0 comments on commit e85222a

Please sign in to comment.