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

[WinUI] Cache gesture event subscriptions #21959

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Apr 20, 2024

Description of Change

Interops operations on Windows are costly. This PR avoids unsubscribing events that were not subscribed in the first place and thus improving performance.

Speedscope

image

-> 70% improvement for that particular method.

I test with my complex grid sample basically.

Issues Fixed

Contributes to #21787

@MartyIX MartyIX requested a review from a team as a code owner April 20, 2024 17:09
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Apr 20, 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.

_areContainerTapAndRightTabEventSubscribed = false;

_container.Tapped -= OnTap;
_container.RightTapped -= OnTap;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this event WAS NOT unsubscribed at all.


_container.DragOver -= HandleDragOver;
_container.Drop -= HandleDrop;
_container.DragLeave -= HandleDragLeave;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this event WAS NOT unsubscribed at all.

@jsuarezruiz jsuarezruiz added platform/windows 🪟 legacy-area-perf Startup / Runtime performance area-gestures Gesture types labels Apr 22, 2024
@MartyIX
Copy link
Contributor Author

MartyIX commented Apr 22, 2024

@jonathanpeppers would you mind taking a look please?

Comment on lines 26 to 31
bool _areContainerDragEventsSubscribed;
bool _areContainerDropEventsSubscribed;
bool _areContainerPgrPointerEventsSubscribed;
bool _areContainerManipulationAndPointerEventsSubscribed;
bool _areContainerTapAndRightTabEventSubscribed;
bool _isContainerDoubleTapEventSubscribed;
Copy link
Member

@jonathanpeppers jonathanpeppers Apr 22, 2024

Choose a reason for hiding this comment

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

One concern here is adding 6 bool fields is actually adding 6 bytes of memory usage for every View.

So, some ideas:

  • Could we make this one bool field instead? If a default view doesn't subscribe, maybe we could just store the state of all the events in one bool _subscribed;? If one event subscribes it would attempt to unsubscribe all?

  • If we need all 6 distinct values, we could consider:

[Flags]
enum SubscriptionFlags : byte
{
    None = 0,
    ContainerDragEventsSubscribed = 1 << 0,
    ContainerDropEventsSubscribed = 1 << 1,
    ContainerPgrPointerEventsSubscribed = 1 << 2,
    ContainerManipulationAndPointerEventsSubscribed= 1 << 3,
// etc.
}

And store a single SubscriptionFlags enum in a field, that would take 1 byte instead of 6.

If there were a BenchmarkDotNet benchmark, we might know exactly how much extra memory is used. But I don't think we have a Windows equivalent of this project:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsubscriptions are costly because of interops and there are always some events subscribed AFAIK so I don't think that we can go with bool _subscribed. I went with the other suggestion instead.

@MartyIX MartyIX force-pushed the feature/2024-04-20-GesturePlatformManager-cache-event-subscriptions branch from 9f71d57 to 0e5a07c Compare April 23, 2024 08:11
@MartyIX
Copy link
Contributor Author

MartyIX commented Apr 23, 2024

@jonathanpeppers
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

This looks good to me from a performance perspective. Someone familiar with these gestures on Windows should review as well. 👍

@MartyIX
Copy link
Contributor Author

MartyIX commented Apr 24, 2024

@rmarinho Could you take a look please?

@MartyIX
Copy link
Contributor Author

MartyIX commented Apr 26, 2024

@jfversluis Could you take a look please?

@jfversluis
Copy link
Member

@Foda might be the right person here

@MartyIX
Copy link
Contributor Author

MartyIX commented May 3, 2024

@Foda Could you take a look please?

Copy link
Member

@Foda Foda left a comment

Choose a reason for hiding this comment

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

LGTM

@MartyIX
Copy link
Contributor Author

MartyIX commented May 9, 2024

@rmarinho Is it good to be merged or does it need to wait until SR6?

@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
@MartyIX
Copy link
Contributor Author

MartyIX commented May 14, 2024

@rmarinho Anything else to do here?

@rmarinho rmarinho merged commit c527393 into dotnet:main May 16, 2024
49 checks passed
@MartyIX MartyIX deleted the feature/2024-04-20-GesturePlatformManager-cache-event-subscriptions branch May 16, 2024 11:27
@github-actions github-actions bot locked and limited conversation to collaborators Jun 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-gestures Gesture types community ✨ Community Contribution fixed-in-8.0.60 fixed-in-9.0.0-preview.5.24307.10 platform/windows 🪟 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants