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

[Windows] Add WindowsBatchPropertyMapper to save unnecessary work when initializing views #24417

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Aug 24, 2024

Description of Change

This PR is based on the idea that for each view, many MapTranslation* methods are called but the they are called for no good reason because the operation is always the same. So in effect, there are many unnecessary interop calls which are costly.

The implementation is based on https://github.com/dotnet/maui/blob/c1566589d1794e5adcbcf2e93bd77fc85f859253/src/Core/src/Handlers/View/AndroidBatchPropertyMapper.cs (basically a copy).

Performance impact

image

Issues Fixed

Contributes to #21787
Replaces #21818

@MartyIX MartyIX requested a review from a team as a code owner August 24, 2024 19:42
@MartyIX MartyIX requested review from Eilon and rmarinho August 24, 2024 19:42
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Aug 24, 2024
Copy link
Contributor

Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@MartyIX MartyIX requested review from jonathanpeppers and removed request for Eilon August 24, 2024 19:44
@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

My gut here is that I'm still not a big fan of the Batching concept in general. I think it causes mappers to act in an unexpected way because now these mappers don't fire at all during the first pass. We've had a number of issues where we've put setup code into a mapper thinking it'll init with that property and then it never runs except for on updates.

I think a mechanism of defaults would make the most sense

Ideally, we'd have some mechanism where the mapper fires, but we don't pay the cost of interop.

Ideally the translation paths are just smart enough to know they don't need to fire an update.

Like, I'd almost rather see us just copy paste something into all these mappers that's like

if (IsInitialRun)
return;

versus trying to be too clever in a way that's kind of unexpected

@MartyIX
Copy link
Contributor Author

MartyIX commented Sep 3, 2024

Like, I'd almost rather see us just copy paste something into all these mappers that's like

if (IsInitialRun) return;

This PR #23987 does that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-platform Integration with platforms community ✨ Community Contribution platform/windows 🪟
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants