From a79a4351b5d6a8ecdb15df6a400e3b1c63a7545e Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Wed, 19 May 2021 13:48:45 +0200 Subject: [PATCH] Be smarter at detecting the constant property and at not generating bindings this implies that we need to make sure the property are initialized in order so that constant properties that depends on other constant properties are correctly computed --- sixtyfps_compiler/expression_tree.rs | 50 +++++++++++++++++++--- sixtyfps_compiler/generator.rs | 64 ++++++++++++++++++++++++++++ sixtyfps_compiler/generator/cpp.rs | 26 ++++++----- sixtyfps_compiler/generator/rust.rs | 32 +++++++++----- sixtyfps_compiler/namedreference.rs | 46 ++++++++++++++++++++ 5 files changed, 189 insertions(+), 29 deletions(-) diff --git a/sixtyfps_compiler/expression_tree.rs b/sixtyfps_compiler/expression_tree.rs index 9bd28e029..de59514f7 100644 --- a/sixtyfps_compiler/expression_tree.rs +++ b/sixtyfps_compiler/expression_tree.rs @@ -121,6 +121,34 @@ impl BuiltinFunction { } } } + + /// It is pure if the return value only depends on its argument and has no side effect + fn is_pure(&self) -> bool { + match self { + BuiltinFunction::GetWindowScaleFactor => false, + // Even if it is not pure, we optimize it away anyway + BuiltinFunction::Debug => true, + BuiltinFunction::Mod + | BuiltinFunction::Round + | BuiltinFunction::Ceil + | BuiltinFunction::Floor + | BuiltinFunction::Sqrt + | BuiltinFunction::Cos + | BuiltinFunction::Sin + | BuiltinFunction::Tan + | BuiltinFunction::ACos + | BuiltinFunction::ASin + | BuiltinFunction::ATan => true, + BuiltinFunction::SetFocusItem => false, + BuiltinFunction::ShowPopupWindow => false, + BuiltinFunction::StringToFloat | BuiltinFunction::StringIsFloat => true, + BuiltinFunction::ColorBrighter | BuiltinFunction::ColorDarker => true, + BuiltinFunction::Rgb => true, + BuiltinFunction::ImplicitLayoutInfo => false, + BuiltinFunction::RegisterCustomFontByPath + | BuiltinFunction::RegisterCustomFontByMemory => false, + } + } } #[derive(Debug, Clone, Eq, PartialEq)] @@ -721,26 +749,33 @@ impl Expression { match self { Expression::Invalid => true, Expression::Uncompiled(_) => false, - Expression::TwoWayBinding(..) => false, + Expression::TwoWayBinding(nr, expr) => { + nr.is_constant() && expr.as_ref().map_or(true, |e| e.is_constant()) + } Expression::StringLiteral(_) => true, Expression::NumberLiteral(_, _) => true, Expression::BoolLiteral(_) => true, Expression::CallbackReference { .. } => false, - Expression::PropertyReference { .. } => false, - Expression::BuiltinFunctionReference { .. } => false, + Expression::PropertyReference(nr) => nr.is_constant(), + Expression::BuiltinFunctionReference(func) => func.is_pure(), Expression::MemberFunction { .. } => false, Expression::ElementReference(_) => false, Expression::RepeaterIndexReference { .. } => false, Expression::RepeaterModelReference { .. } => false, Expression::FunctionParameterReference { .. } => false, - Expression::BuiltinMacroReference { .. } => false, + Expression::BuiltinMacroReference { .. } => true, Expression::StructFieldAccess { base, .. } => base.is_constant(), Expression::Cast { from, .. } => from.is_constant(), Expression::CodeBlock(sub) => sub.len() == 1 && sub.first().unwrap().is_constant(), - Expression::FunctionCall { .. } => false, + Expression::FunctionCall { function, arguments, .. } => { + // Assume that constant function are, in fact, pure + function.is_constant() && arguments.iter().all(|a| a.is_constant()) + } Expression::SelfAssignment { .. } => false, Expression::ImageReference { .. } => true, - Expression::Condition { .. } => false, + Expression::Condition { condition, false_expr, true_expr } => { + condition.is_constant() && false_expr.is_constant() && true_expr.is_constant() + } Expression::BinaryExpression { lhs, rhs, .. } => lhs.is_constant() && rhs.is_constant(), Expression::UnaryOp { sub, .. } => sub.is_constant(), Expression::Array { values, .. } => values.iter().all(Expression::is_constant), @@ -755,6 +790,7 @@ impl Expression { } } Expression::StoreLocalVariable { .. } => false, + // we should somehow find out if this is constant or not Expression::ReadLocalVariable { .. } => false, Expression::EasingCurve(_) => true, Expression::LinearGradient { angle, stops } => { @@ -764,7 +800,7 @@ impl Expression { Expression::ReturnStatement(expr) => { expr.as_ref().map_or(true, |expr| expr.is_constant()) } - + // TODO: detect constant property within layouts Expression::LayoutCacheAccess { .. } => false, Expression::ComputeLayoutInfo(_) => false, Expression::SolveLayout(_) => false, diff --git a/sixtyfps_compiler/generator.rs b/sixtyfps_compiler/generator.rs index e4a4f8847..3a88fcf7a 100644 --- a/sixtyfps_compiler/generator.rs +++ b/sixtyfps_compiler/generator.rs @@ -13,7 +13,12 @@ The module responsible for the code generation. There is one sub module for every language */ +use std::collections::HashSet; +use std::rc::{Rc, Weak}; + use crate::diagnostics::BuildDiagnostics; +use crate::expression_tree::{BindingExpression, Expression}; +use crate::namedreference::NamedReference; use crate::object_tree::{Component, Document, ElementRc}; #[cfg(feature = "cpp")] @@ -128,3 +133,62 @@ pub fn build_array_helper(component: &Component, mut visit_item: impl FnMut(&Ele } } } + +/// Will wall the `handle_property` callback for every property that needs to be initialized. +/// This function makes sure to call them in order so that if constant binding need to access +/// constant properties, these are already initialized +pub fn handle_property_bindings_init( + component: &Rc, + mut handle_property: impl FnMut(&ElementRc, &str, &BindingExpression), +) { + fn handle_property_inner( + component: &Weak, + elem: &ElementRc, + prop_name: &str, + binding_expression: &BindingExpression, + handle_property: &mut impl FnMut(&ElementRc, &str, &BindingExpression), + processed: &mut HashSet, + ) { + let nr = NamedReference::new(elem, prop_name); + if processed.contains(&nr) { + return; + } + processed.insert(nr); + if binding_expression.analysis.borrow().as_ref().map_or(false, |a| a.is_const) { + // We must first handle all dependent properties in case it is a constant property + binding_expression.expression.visit_recursive(&mut |e| match e { + Expression::PropertyReference(nr) => { + let elem = nr.element(); + if Weak::ptr_eq(&elem.borrow().enclosing_component, component) { + if let Some(be) = elem.borrow().bindings.get(nr.name()) { + handle_property_inner( + component, + &elem, + nr.name(), + be, + handle_property, + processed, + ); + } + } + } + _ => {} + }) + } + handle_property(elem, prop_name, binding_expression); + } + + let mut processed = HashSet::new(); + crate::object_tree::recurse_elem(&component.root_element, &(), &mut |elem: &ElementRc, ()| { + for (prop_name, binding_expression) in &elem.borrow().bindings { + handle_property_inner( + &Rc::downgrade(component), + elem, + prop_name, + binding_expression, + &mut handle_property, + &mut processed, + ); + } + }); +} diff --git a/sixtyfps_compiler/generator/cpp.rs b/sixtyfps_compiler/generator/cpp.rs index f5a8cc7d8..59ad737e4 100644 --- a/sixtyfps_compiler/generator/cpp.rs +++ b/sixtyfps_compiler/generator/cpp.rs @@ -332,6 +332,7 @@ fn handle_property_binding( elem: &ElementRc, prop_name: &str, binding_expression: &Expression, + is_constant: bool, init: &mut Vec, ) { let item = elem.borrow(); @@ -371,7 +372,7 @@ fn handle_property_binding( p2 = access_named_reference(nr, &component, "this") )); if let Some(next) = next { - handle_property_binding(elem, prop_name, next, init) + handle_property_binding(elem, prop_name, next, is_constant || next.is_constant(), init) } } else { let component = &item.enclosing_component.upgrade().unwrap(); @@ -379,7 +380,7 @@ fn handle_property_binding( let init_expr = compile_expression_wrap_return(binding_expression, component); let cpp_prop = format!("{}{}", accessor_prefix, prop_name); - init.push(if binding_expression.is_constant() { + init.push(if is_constant { format!("{}.set({});", cpp_prop, init_expr) } else { let binding_code = format!( @@ -437,7 +438,7 @@ fn handle_property_binding( } } -fn handle_item(elem: &ElementRc, main_struct: &mut Struct, init: &mut Vec) { +fn handle_item(elem: &ElementRc, main_struct: &mut Struct) { let item = elem.borrow(); main_struct.members.push(( Access::Private, @@ -447,10 +448,6 @@ fn handle_item(elem: &ElementRc, main_struct: &mut Struct, init: &mut Vec, ) { let rust_property = access_member(item_rc, prop_name, component, quote!(_self), false); @@ -188,11 +189,18 @@ fn handle_property_binding( Property::link_two_way(#rust_property, #p2); )); if let Some(next) = next { - handle_property_binding(component, item_rc, prop_name, next, init) + handle_property_binding( + component, + item_rc, + prop_name, + next, + is_constant || next.is_constant(), + init, + ) } } else { let tokens_for_expression = compile_expression(binding_expression, &component); - init.push(if binding_expression.is_constant() { + init.push(if is_constant { let t = rust_type(&prop_type).unwrap_or(quote!(_)); quote! { #rust_property.set((||-> #t { (#tokens_for_expression) as #t })()); } } else { @@ -385,9 +393,6 @@ fn generate_component( let item = item_rc.borrow(); if item.base_type == Type::Void { assert!(component.is_global()); - for (k, binding_expression) in &item.bindings { - handle_property_binding(component, item_rc, k, binding_expression, &mut init); - } } else if let Some(repeated) = &item.repeated { let base_component = item.base_type.as_component(); let repeater_index = repeated_element_names.len(); @@ -561,9 +566,6 @@ fn generate_component( parent_index: #parent_index } )); - for (k, binding_expression) in &item.bindings { - handle_property_binding(component, item_rc, k, binding_expression, &mut init); - } } else { let field_name = format_ident!("{}", item.id); let children_count = item.children.len() as u32; @@ -576,14 +578,22 @@ fn generate_component( parent_index: #parent_index, } )); - for (k, binding_expression) in &item.bindings { - handle_property_binding(component, item_rc, k, binding_expression, &mut init); - } item_names.push(field_name); item_types.push(format_ident!("{}", item.base_type.as_native().class_name)); } }); + super::handle_property_bindings_init(component, |elem, prop, binding| { + handle_property_binding( + component, + elem, + prop, + binding, + binding.analysis.borrow().as_ref().map_or(false, |a| a.is_const), + &mut init, + ) + }); + let resource_symbols: Vec = component .embedded_file_resources .borrow() diff --git a/sixtyfps_compiler/namedreference.rs b/sixtyfps_compiler/namedreference.rs index fe59f93e6..7f45a527c 100644 --- a/sixtyfps_compiler/namedreference.rs +++ b/sixtyfps_compiler/namedreference.rs @@ -56,6 +56,52 @@ impl NamedReference { pub fn ty(&self) -> Type { self.element().borrow().lookup_property(self.name()).property_type } + + /// return true if the property has a constant value for the lifetime of the program + pub fn is_constant(&self) -> bool { + let mut elem = self.element(); + let e = elem.borrow(); + if e.property_analysis.borrow().get(self.name()).map_or(false, |a| a.is_set) { + // if the property is set somewhere, it is not constant + return false; + } + if let Some(decl) = e.property_declarations.get(self.name()) { + if decl.expose_in_public_api { + // could be set by the public API + return false; + } + } + drop(e); + + let mut check_binding = true; + loop { + let e = elem.borrow(); + if check_binding { + if let Some(b) = e.bindings.get(self.name()) { + if !b.is_constant() { + return false; + } + check_binding = false; + } + } + if e.property_declarations.contains_key(self.name()) { + return true; + } + match &e.base_type { + Type::Native(_) => return false, // after resolving we don't know anymore if the property can be changed natively + Type::Component(c) => { + let next = c.root_element.clone(); + drop(e); + elem = next; + continue; + } + Type::Builtin(b) => { + return b.properties.get(self.name()).map_or(true, |pi| !pi.is_native_output) + } + _ => return true, + } + } + } } impl Eq for NamedReference {}