Skip to content

Avoid recomputing bindings multiple times and bind BindingContext first#24553

Merged
PureWeen merged 1 commit into
dotnet:mainfrom
albyrock87:fix-10806-binding-executed-multiple-times
Sep 6, 2024
Merged

Avoid recomputing bindings multiple times and bind BindingContext first#24553
PureWeen merged 1 commit into
dotnet:mainfrom
albyrock87:fix-10806-binding-executed-multiple-times

Conversation

@albyrock87

@albyrock87 albyrock87 commented Aug 31, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description of Change

The two unit tests shows the expectation we all have about bindings:

  • eventual BindingContext binding should apply before any property binding
  • Binding should happen only once upon binding context inheritance

Benchmarks

Before

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
SetProperty 892.2 us 7.64 us 6.77 us - - - 873 B
ApplyProperty ViaBindingContextInheritance 379.7 us 3.94 us 3.49 us 76.6602 38.0859 3.9063 644003 B
ApplyBindingContext ViaBindingContextInheritance 775.7 us 8.05 us 7.13 us 109.3750 54.6875 11.7188 919289 B

After

Method Mean Error StdDev Gen0 Gen1 Allocated
SetProperty 868.0 us 1.00 us 0.83 us - - 873 B
ApplyProperty ViaBindingContextInheritance 309.0 us 0.54 us 0.50 us 72.7539 36.1328 612000 B
ApplyBindingContext ViaBindingContextInheritance 497.6 us 1.23 us 1.15 us 93.7500 46.8750 784881 B

Issues Fixed

Fixes #10806

@albyrock87 albyrock87 requested a review from a team as a code owner August 31, 2024 12:30
@dotnet-policy-service dotnet-policy-service Bot added the community ✨ Community Contribution label Aug 31, 2024
@albyrock87 albyrock87 mentioned this pull request Aug 31, 2024
Closed
@albyrock87 albyrock87 force-pushed the fix-10806-binding-executed-multiple-times branch 2 times, most recently from 62ac3ce to e6e4544 Compare September 2, 2024 09:55
albyrock87
albyrock87 commented Sep 2, 2024
View reviewed changes
Comment thread src/Controls/src/Core/BindableObject.cs
/// Applies all the current bindings to <see cref="BindingContext" />.
/// </summary>
protected void ApplyBindings() => ApplyBindings(skipBindingContext: false, fromBindingContextChanged: false);
protected void ApplyBindings()

@albyrock87 albyrock87 Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This method appears unused, shall we [Obsolete] it?

@albyrock87 albyrock87 force-pushed the fix-10806-binding-executed-multiple-times branch from e6e4544 to c568f3b Compare September 2, 2024 10:13
@rmarinho

rmarinho commented Sep 2, 2024

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

azure-pipelines Bot commented Sep 2, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@samhouts samhouts added the area-xaml XAML, CSS, Triggers, Behaviors label Sep 4, 2024
StephaneDelcroix
StephaneDelcroix requested changes Sep 6, 2024
View reviewed changes

@StephaneDelcroix StephaneDelcroix left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this looks good. and if it's green, well, go

@PureWeen PureWeen merged commit bc5b90b into dotnet:main Sep 6, 2024
@rmarinho

rmarinho commented Sep 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

@albyrock87 @StephaneDelcroix this test seem to be failing when merging main to net9.0

#24759

https://dev.azure.com/xamarin/public/_build/results?buildId=123788&view=ms.vss-test-web.build-test-results-tab&runId=3145016&resultId=102857&paneView=debug

Assert.Equal() Failure: Strings differ
Expected: "132"
Actual:   "13223"
↑ (pos 3)

@albyrock87

albyrock87 commented Sep 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@rmarinho looking at your PR #24759, I see that all the changes in BindableObject from this PR are missing.
https://github.com/dotnet/maui/pull/24553/files#diff-ebb14ec14b8baeffdd7b0ae40ffc8bf0e8944f76fdea50fcbc737fb142bfb44a
Maybe a mistake solving merge conflicts?

@kubaflo kubaflo mentioned this pull request Sep 14, 2024
Closed
@albyrock87 albyrock87 mentioned this pull request Sep 17, 2024
Closed
@samhouts samhouts added the fixed-in-net9.0-nightly This may be available in a nightly release! label Oct 1, 2024
@samhouts samhouts added fixed-in-9.0.0-rc.2.24503.2 and removed fixed-in-net9.0-nightly This may be available in a nightly release! labels Oct 14, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@StephaneDelcroix StephaneDelcroix StephaneDelcroix approved these changes

@simonrozsival simonrozsival Awaiting requested review from simonrozsival

Assignees

@StephaneDelcroix StephaneDelcroix

Labels

area-xaml XAML, CSS, Triggers, Behaviors community ✨ Community Contribution fixed-in-9.0.0-rc.2.24503.2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bindings are executed twice

5 participants

@albyrock87 @rmarinho @StephaneDelcroix @PureWeen @samhouts