Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler optimizations #7680

Merged
merged 12 commits into from
Feb 21, 2025

Conversation

badicsalex
Copy link
Contributor

We've been experiencing slow builds with medium-sized slint files, and the main culprit seems to be the sheer amount of code generated. I understand that rustc will do a good job optimizing that code, but we can help it a bit on the generation side. We have a few patterns which were handled relatively poorly by slint, and that's what this PR fixes.

Summary of the changes:

  • Return elimination has been improved so less functions will need a return_object, allowing full const resolution in a lot of cases 1
  • Functions where the parameters are const and the body only accesses consts are now automatically inlined and evaluated. This usually results with them being replaced by just a literal.2
  • If a component is used exactly once, it is auto-inlined
  • If a component is conditional or repeated, it is no longer force-inlined. This almost always results in smaller code (combined with the above change), because duplicating components is worse than just having more bindings in all but the simplest cases. 3

These changes should be 99% transparent, as they shouldn't change behavior, only make the generated code smaller. Tests needed to be adjusted, because diagnostics are moved around due to the new inlining rules.

In the future I want to use the new, more powerful const propagation to eliminate the repeaters of unconditional conditional elements, but that's a different PR for sure.

1: Functions with the pattern function f(a: float) -> string { if a>5 { return "yes" }; return "no" } needed a return object, now they don't

2: We have a lot of global pure functions that are called like Colors.get(Palette.primary) everywhere, and they caused a lot of unpropagated constants.

3: In one specific case, we had code like:

VerticalLayout {
    if (important-items.length > 0): ItemList {
        important: true;
        items: important-items;
    }
    for section[section-idx] in other-sections: ItemList {
        header: section.header;
        items: section.items;
    }
}

Wrapping the ItemLists in a Rectangle made the code compile some 30% faster (ItemList is a complicated component here), and I didn't want to do that because it hampers readability, and there are a lot of other, similar code in our codebase.

@CLAassistant
Copy link

CLAassistant commented Feb 20, 2025

CLA assistant check
All committers have signed the CLA.

@badicsalex
Copy link
Contributor Author

badicsalex commented Feb 20, 2025

Seems like the CI was more thorough with its test running than I was :P

I haven;t checked yet, but I assume this is caused by the remaining cases where the LLR inlining phase inlines some attributes that the item tree inliner didn't, and that causes very obvious dead code in the generated rust files. It does show how well the elimination works in general though. The main problem I saw is is_constant on properties is too strict, but that seemed like a hard to fix issue.

(e.g. the following case:

component Icon inherits Image {
    in property <length> size: Size.sz6;
    source: @image_url("whatever.png");
    width: size;
    height: size;
}

component MultipleIcons {
    HorizontalLayout {
        Icon { size: 15px; }
        Icon { size: 30px; }
    }
}

@badicsalex
Copy link
Contributor Author

Unfortunately it seems like the typeloader keeps the loaded components, some of which refer to the Icons global, which kills the named_reference refcount check in remove_unused_properties

Removing that check of course caused a panic.

Any help on how to approach this would be appreciated.

@badicsalex badicsalex force-pushed the compiler-optimizations branch from 17ef6c3 to 68a1f3c Compare February 20, 2025 12:27
@badicsalex
Copy link
Contributor Author

Nevermind, find an issue where I didn't simplify conditional expressions enough.

The deeper issue (LLR discarding properties that previous passes don't) will remain though.

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for these changes!
Overall looking very good.
I just added a few comments inline.

Do you have any numbers on how good it improves the source code size or compilation speed?

None => SmolStr::default(),
};

let expr = match (name, builtin_name.as_str()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In implicit_layout_info_call called by this function, we we already have special case in other part for the rectangle and empty,

if matches!(

My hope was that then this could be optimized. But this still create something like a { min: 0, max: ...., ... }.min which might not be optimized correctly, and it might indeed be good to handle here.

Otherwise it would make more sense to handle the Image in implicit_layout_info_call as well

Also this might not be working well if the Image contains elements such as
Image{ HorizontalLayout { ... } }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The struct declaration + struct field call gets transformed for sure in the const propagation phase.

Putting it into implicit_layout_info_call would be non-trivial, because it'd need to asssemble a struct that has mostly fixed fields, except the preferred lengths. This would need to be done with either multiple implicit calls, or a non-trivial local variable thing, which is not simplified properly (yet). And where the full layoutinfo struct is needed, it is in fact better to just call the implicit layout info without trying to be smart.

You were right with the Image with children case, I'm going to fix that.

@@ -211,6 +211,9 @@ impl<T> Display for DisplayPropertyRef<'_, T> {
}
PropertyReference::InParent { level, parent_reference } => {
for _ in 0..level.get() {
if ctx.parent.is_none() {
return write!(f, "<invalid parent reference>");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this actually happen?
I guess it's good to be resilient in this debug code anyway. But it is a bit worrisome if that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something as simple as cargo run --bin slint-compiler -- -f llr ./tests/cases/widgets/tableview.slint crashes the printer on the most recent master.

I guess there's an actual code generation issue here, but I didn't want to investigate. In fact I didn't want to push this commit at all, but it accidentally happened :D

@badicsalex
Copy link
Contributor Author

badicsalex commented Feb 20, 2025

Do you have any numbers on how good it improves the source code size or compilation speed?

I didn't measure code size (and actually don't expect much of a change), but compilation times improved 20-40% for us (i.e. down to 2 minutes from 3). We use tons of conditional elements to implement general components though, which might not be common. This is mainly due to the inlining change. The constant propagation didn't have much of an impact, they are more of a base for the conditional-elimination I want to do.

Looking at CI times, it didn't help the runtime of that all that much.

@badicsalex
Copy link
Contributor Author

badicsalex commented Feb 20, 2025

I was wrong; executable size for some of our binaries went down by a lot (12MB => 7.2MB in one example, stripped armv7)

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for this high quality PR.
I think this can be merged.

@@ -98,6 +98,29 @@ fn simplify_expression(expr: &mut Expression) -> bool {
}
('|', Expression::BoolLiteral(false), e) => Some(std::mem::take(e)),
('|', e, Expression::BoolLiteral(false)) => Some(std::mem::take(e)),
('>', Expression::NumberLiteral(a, un1), Expression::NumberLiteral(b, un2))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also work for ≤ and ≥

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering how >= fits in one character, but I guess I got my answer lol

@@ -199,6 +254,30 @@ fn extract_constant_property_reference(nr: &NamedReference) -> Option<Expression
Some(expression)
}

fn try_inline_function(function: &Callable, arguments: &[Expression]) -> Option<Expression> {
let Callable::Function(function) = function else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for future improvements: we could also try to inline some built-in functions.

@ogoffart
Copy link
Member

I did test locally:

cargo build --release -p slint-lsp
11m 03s -> 10m 06s (I think release is especially slow because we specify lto)

cargo test -p test-driver-rust (build time)
43m 25s -> 38m 13s (yes, that's long!)

So we can see about 10% improvements, which is not bad.

Thank you.

@ogoffart ogoffart merged commit 3372823 into slint-ui:master Feb 21, 2025
39 checks passed
@tronical
Copy link
Member

I've tried this on a code base from a customer of ours, and results are fantastic. The binary size shrunk by ~15% and compile time reduced by almost 50%. But I've noticed an issue: Commit bf716ff now yields a compiler error about a binding loop. I've filed #7693 with the test-case attached.

ogoffart added a commit that referenced this pull request Feb 21, 2025
This reverts commit bf716ff.

This exposed the bug #7693 and more issues mentined in
#7693 (comment)

Fixes #7693
CC #7680
@ogoffart
Copy link
Member

We decided to revert commit bf716ff for now because of issues mentioned in #7693 (and #7693 (comment) )

The other fixes still bring good improvements.
We may consider bringing this back later because the bug is clearly not in that patch, but more likely a problem in layout with regards to inline items.
So we'll put that commit back at a later point.

@badicsalex
Copy link
Contributor Author

Well, good thing you caught it so fast, I have no problem with reverting commits that break customer code.

Fingers crossed for fixing the other unearthed issues.

ogoffart added a commit that referenced this pull request Feb 21, 2025
This reverts commit bf716ff.
(And also revert the test part of 259756c)

This exposed the bug #7693 and more issues mentined in
#7693 (comment)

Fixes #7693
CC #7680
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants