Skip to content

Commit 2f62c60

Browse files
milianwmilianw
authored
Optimize resolve_expression to operate on a single scope vector (#6664)
Instead of cloning the vector on every iteration level, pass the scope in and out of the visitation function and push/pop the element there as needed. This way we can operate on a single vector that gets moved around, which removes a few thousand memory allocations. The speed impact is not measurable, as the code also triggers rowan API that is much more allocation happy. Still, I believe this patch is still merge-worthy as it also reduces the code duplication a bit and is subjectively better, esp. from a performance pov.
1 parent 1340a49 commit 2f62c60

File tree

1 file changed

+157
-139
lines changed

1 file changed

+157
-139
lines changed

‎internal/compiler/passes/resolving.rs‎

Lines changed: 157 additions & 139 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ fn resolve_expression(
2929
expr: &mut Expression,
3030
property_name: Option<&str>,
3131
property_type: Type,
32-
scope: &ComponentScope,
32+
scope: &[ElementRc],
3333
type_register: &TypeRegister,
3434
type_loader: &crate::typeloader::TypeLoader,
3535
diag: &mut BuildDiagnostics,
@@ -38,7 +38,7 @@ fn resolve_expression(
3838
let mut lookup_ctx = LookupCtx {
3939
property_name,
4040
property_type,
41-
component_scope: &scope.0,
41+
component_scope: scope,
4242
diag,
4343
arguments: vec![],
4444
type_register,
@@ -76,6 +76,23 @@ fn resolve_expression(
7676
}
7777
}
7878

79+
/// Call the visitor for each children of the element recursively, starting with the element itself
80+
///
81+
/// The item that is being visited will be pushed to the scope and popped once visitation is over.
82+
fn recurse_elem_with_scope(
83+
elem: &ElementRc,
84+
mut scope: ComponentScope,
85+
vis: &mut impl FnMut(&ElementRc, &ComponentScope),
86+
) -> ComponentScope {
87+
scope.0.push(elem.clone());
88+
vis(elem, &scope);
89+
for sub in &elem.borrow().children {
90+
scope = recurse_elem_with_scope(sub, scope, vis);
91+
}
92+
scope.0.pop();
93+
scope
94+
}
95+
7996
pub fn resolve_expressions(
8097
doc: &Document,
8198
type_loader: &crate::typeloader::TypeLoader,
@@ -84,19 +101,27 @@ pub fn resolve_expressions(
84101
resolve_two_way_bindings(doc, &doc.local_registry, diag);
85102

86103
for component in doc.inner_components.iter() {
87-
let scope = ComponentScope(vec![]);
88-
89-
recurse_elem(&component.root_element, &scope, &mut |elem, scope| {
90-
let mut new_scope = scope.clone();
91-
let mut is_repeated = elem.borrow().repeated.is_some();
92-
new_scope.0.push(elem.clone());
93-
visit_element_expressions(elem, |expr, property_name, property_type| {
94-
if is_repeated {
95-
// The first expression is always the model and it needs to be resolved with the parent scope
96-
debug_assert!(matches!(
97-
elem.borrow().repeated.as_ref().unwrap().model,
98-
Expression::Invalid
99-
)); // should be Invalid because it is taken by the visit_element_expressions function
104+
recurse_elem_with_scope(
105+
&component.root_element,
106+
ComponentScope(vec![]),
107+
&mut |elem, scope| {
108+
let mut is_repeated = elem.borrow().repeated.is_some();
109+
visit_element_expressions(elem, |expr, property_name, property_type| {
110+
let scope = if is_repeated {
111+
// The first expression is always the model and it needs to be resolved with the parent scope
112+
debug_assert!(matches!(
113+
elem.borrow().repeated.as_ref().unwrap().model,
114+
Expression::Invalid
115+
)); // should be Invalid because it is taken by the visit_element_expressions function
116+
117+
is_repeated = false;
118+
119+
debug_assert!(scope.0.len() > 1);
120+
&scope.0[..scope.0.len() - 1]
121+
} else {
122+
&scope.0
123+
};
124+
100125
resolve_expression(
101126
expr,
102127
property_name,
@@ -106,21 +131,9 @@ pub fn resolve_expressions(
106131
type_loader,
107132
diag,
108133
);
109-
is_repeated = false;
110-
} else {
111-
resolve_expression(
112-
expr,
113-
property_name,
114-
property_type(),
115-
&new_scope,
116-
&doc.local_registry,
117-
type_loader,
118-
diag,
119-
)
120-
}
121-
});
122-
new_scope
123-
})
134+
});
135+
},
136+
);
124137
}
125138
}
126139

@@ -1548,134 +1561,139 @@ fn resolve_two_way_bindings(
15481561
diag: &mut BuildDiagnostics,
15491562
) {
15501563
for component in doc.inner_components.iter() {
1551-
let scope = ComponentScope(vec![]);
1552-
1553-
recurse_elem(&component.root_element, &scope, &mut |elem, scope| {
1554-
let mut new_scope = scope.clone();
1555-
new_scope.0.push(elem.clone());
1556-
for (prop_name, binding) in &elem.borrow().bindings {
1557-
let mut binding = binding.borrow_mut();
1558-
if let Expression::Uncompiled(node) = binding.expression.clone() {
1559-
if let Some(n) = syntax_nodes::TwoWayBinding::new(node.clone()) {
1560-
let lhs_lookup = elem.borrow().lookup_property(prop_name);
1561-
if !lhs_lookup.is_valid() {
1562-
// An attempt to resolve this already failed when trying to resolve the property type
1563-
assert!(diag.has_errors());
1564-
continue;
1565-
}
1566-
let mut lookup_ctx = LookupCtx {
1567-
property_name: Some(prop_name.as_str()),
1568-
property_type: lhs_lookup.property_type.clone(),
1569-
component_scope: &new_scope.0,
1570-
diag,
1571-
arguments: vec![],
1572-
type_register,
1573-
type_loader: None,
1574-
current_token: Some(node.clone().into()),
1575-
};
1576-
1577-
binding.expression = Expression::Invalid;
1578-
1579-
if let Some(nr) = resolve_two_way_binding(n, &mut lookup_ctx) {
1580-
binding.two_way_bindings.push(nr.clone());
1581-
1582-
nr.element()
1583-
.borrow()
1584-
.property_analysis
1585-
.borrow_mut()
1586-
.entry(nr.name().into())
1587-
.or_default()
1588-
.is_linked = true;
1589-
1590-
if matches!(
1591-
lhs_lookup.property_visibility,
1592-
PropertyVisibility::Private | PropertyVisibility::Output
1593-
) && !lhs_lookup.is_local_to_component
1594-
{
1595-
// invalid property assignment should have been reported earlier
1596-
assert!(diag.has_errors());
1597-
continue;
1598-
}
1599-
1600-
// Check the compatibility.
1601-
let mut rhs_lookup = nr.element().borrow().lookup_property(nr.name());
1602-
if rhs_lookup.property_type == Type::Invalid {
1564+
recurse_elem_with_scope(
1565+
&component.root_element,
1566+
ComponentScope(vec![]),
1567+
&mut |elem, scope| {
1568+
for (prop_name, binding) in &elem.borrow().bindings {
1569+
let mut binding = binding.borrow_mut();
1570+
if let Expression::Uncompiled(node) = binding.expression.clone() {
1571+
if let Some(n) = syntax_nodes::TwoWayBinding::new(node.clone()) {
1572+
let lhs_lookup = elem.borrow().lookup_property(prop_name);
1573+
if !lhs_lookup.is_valid() {
16031574
// An attempt to resolve this already failed when trying to resolve the property type
16041575
assert!(diag.has_errors());
16051576
continue;
16061577
}
1607-
rhs_lookup.is_local_to_component &=
1608-
lookup_ctx.is_local_element(&nr.element());
1609-
1610-
if !rhs_lookup.is_valid_for_assignment() {
1611-
match (
1578+
let mut lookup_ctx = LookupCtx {
1579+
property_name: Some(prop_name.as_str()),
1580+
property_type: lhs_lookup.property_type.clone(),
1581+
component_scope: &scope.0,
1582+
diag,
1583+
arguments: vec![],
1584+
type_register,
1585+
type_loader: None,
1586+
current_token: Some(node.clone().into()),
1587+
};
1588+
1589+
binding.expression = Expression::Invalid;
1590+
1591+
if let Some(nr) = resolve_two_way_binding(n, &mut lookup_ctx) {
1592+
binding.two_way_bindings.push(nr.clone());
1593+
1594+
nr.element()
1595+
.borrow()
1596+
.property_analysis
1597+
.borrow_mut()
1598+
.entry(nr.name().into())
1599+
.or_default()
1600+
.is_linked = true;
1601+
1602+
if matches!(
16121603
lhs_lookup.property_visibility,
1613-
rhs_lookup.property_visibility,
1614-
) {
1615-
(PropertyVisibility::Input, PropertyVisibility::Input)
1616-
if !lhs_lookup.is_local_to_component =>
1617-
{
1618-
assert!(rhs_lookup.is_local_to_component);
1619-
marked_linked_read_only(elem, prop_name);
1620-
}
1621-
(
1622-
PropertyVisibility::Output | PropertyVisibility::Private,
1623-
PropertyVisibility::Output | PropertyVisibility::Input,
1624-
) => {
1625-
assert!(lhs_lookup.is_local_to_component);
1626-
marked_linked_read_only(elem, prop_name);
1604+
PropertyVisibility::Private | PropertyVisibility::Output
1605+
) && !lhs_lookup.is_local_to_component
1606+
{
1607+
// invalid property assignment should have been reported earlier
1608+
assert!(diag.has_errors());
1609+
continue;
1610+
}
1611+
1612+
// Check the compatibility.
1613+
let mut rhs_lookup =
1614+
nr.element().borrow().lookup_property(nr.name());
1615+
if rhs_lookup.property_type == Type::Invalid {
1616+
// An attempt to resolve this already failed when trying to resolve the property type
1617+
assert!(diag.has_errors());
1618+
continue;
1619+
}
1620+
rhs_lookup.is_local_to_component &=
1621+
lookup_ctx.is_local_element(&nr.element());
1622+
1623+
if !rhs_lookup.is_valid_for_assignment() {
1624+
match (
1625+
lhs_lookup.property_visibility,
1626+
rhs_lookup.property_visibility,
1627+
) {
1628+
(PropertyVisibility::Input, PropertyVisibility::Input)
1629+
if !lhs_lookup.is_local_to_component =>
1630+
{
1631+
assert!(rhs_lookup.is_local_to_component);
1632+
marked_linked_read_only(elem, prop_name);
1633+
}
1634+
(
1635+
PropertyVisibility::Output
1636+
| PropertyVisibility::Private,
1637+
PropertyVisibility::Output | PropertyVisibility::Input,
1638+
) => {
1639+
assert!(lhs_lookup.is_local_to_component);
1640+
marked_linked_read_only(elem, prop_name);
1641+
}
1642+
(PropertyVisibility::Input, PropertyVisibility::Output)
1643+
if !lhs_lookup.is_local_to_component =>
1644+
{
1645+
assert!(!rhs_lookup.is_local_to_component);
1646+
marked_linked_read_only(elem, prop_name);
1647+
}
1648+
_ => {
1649+
if lookup_ctx.is_legacy_component() {
1650+
diag.push_warning(
1651+
format!(
1652+
"Link to a {} property is deprecated",
1653+
rhs_lookup.property_visibility
1654+
),
1655+
&node,
1656+
);
1657+
} else {
1658+
diag.push_error(
1659+
format!(
1660+
"Cannot link to a {} property",
1661+
rhs_lookup.property_visibility
1662+
),
1663+
&node,
1664+
)
1665+
}
1666+
}
16271667
}
1628-
(PropertyVisibility::Input, PropertyVisibility::Output)
1629-
if !lhs_lookup.is_local_to_component =>
1668+
} else if !lhs_lookup.is_valid_for_assignment() {
1669+
if rhs_lookup.is_local_to_component
1670+
&& rhs_lookup.property_visibility
1671+
== PropertyVisibility::InOut
16301672
{
1631-
assert!(!rhs_lookup.is_local_to_component);
1632-
marked_linked_read_only(elem, prop_name);
1633-
}
1634-
_ => {
16351673
if lookup_ctx.is_legacy_component() {
1636-
diag.push_warning(
1637-
format!(
1638-
"Link to a {} property is deprecated",
1639-
rhs_lookup.property_visibility
1640-
),
1641-
&node,
1642-
);
1674+
debug_assert!(!diag.is_empty()); // warning should already be reported
16431675
} else {
16441676
diag.push_error(
1645-
format!(
1646-
"Cannot link to a {} property",
1647-
rhs_lookup.property_visibility
1648-
),
1677+
"Cannot link input property".into(),
16491678
&node,
1650-
)
1679+
);
16511680
}
1652-
}
1653-
}
1654-
} else if !lhs_lookup.is_valid_for_assignment() {
1655-
if rhs_lookup.is_local_to_component
1656-
&& rhs_lookup.property_visibility == PropertyVisibility::InOut
1657-
{
1658-
if lookup_ctx.is_legacy_component() {
1659-
debug_assert!(!diag.is_empty()); // warning should already be reported
1681+
} else if rhs_lookup.property_visibility
1682+
== PropertyVisibility::InOut
1683+
{
1684+
diag.push_warning("Linking input properties to input output properties is deprecated".into(), &node);
1685+
marked_linked_read_only(&nr.element(), nr.name());
16601686
} else {
1661-
diag.push_error("Cannot link input property".into(), &node);
1687+
// This is allowed, but then the rhs must also become read only.
1688+
marked_linked_read_only(&nr.element(), nr.name());
16621689
}
1663-
} else if rhs_lookup.property_visibility
1664-
== PropertyVisibility::InOut
1665-
{
1666-
diag.push_warning("Linking input properties to input output properties is deprecated".into(), &node);
1667-
marked_linked_read_only(&nr.element(), nr.name());
1668-
} else {
1669-
// This is allowed, but then the rhs must also become read only.
1670-
marked_linked_read_only(&nr.element(), nr.name());
16711690
}
16721691
}
16731692
}
16741693
}
16751694
}
1676-
}
1677-
new_scope
1678-
})
1695+
},
1696+
);
16791697
}
16801698

16811699
fn marked_linked_read_only(elem: &ElementRc, prop_name: &str) {

0 commit comments

Comments
 (0)