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

[Perf] Reduce the amount of time the EVENT_OBJECT_LOCATIONCHANGE hook is enabled #1264

Closed
TheMrJukes opened this issue Feb 11, 2020 · 5 comments
Assignees
Labels
Area-Quality Stability, Performance, Etc. Cost-Large Large work item - 3+ days worth of work (chances are needs to be broken down) Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@TheMrJukes
Copy link
Contributor

Summary of the new feature/enhancement

Simply moving the cursor is enough to generate an EVENT_OBJECT_LOCATIONCHANGE.
Last I checked, PT registers for EVENT_MIN/EVENT_MAX in SetWinEventHook.
You'll notice that moving the cursor around the screen quickly is enough to spike PT CPU usage to 3-5% depending on the PC.
The idea is to reduce the amount of time PT is spent registered to receive EVENT_OBJECT_LOCATIONCHANGE and other events it does not need to process.

Proposed technical implementation details (optional)

TheMrJukes/PowerToys@master...TheMrJukes:user/bretan/pt-optimize

This is not completely ready to ship but I haven't had time recently to fully finish it up.
The idea is that we ask each powertoy exactly which events it needs for SetWinEventHook.
This way we only register for the required events.
It is more efficient to have 30 hooks registered for the specific events necessary than it is to have 1 hook registered for a larger range of events. The overhead of having a bunch of hooks enabled to the kernel is minimal.
It also provides a callback to allow the powertoy a way to change which events it requires. This lets FZ only request EVENT_OBJECT_LOCATIONCHANGE while a drag is in progress.

@TheMrJukes TheMrJukes added the Area-Quality Stability, Performance, Etc. label Feb 11, 2020
@enricogior
Copy link
Contributor

Thanks @TheMrJukes this is a great improvement!
When you have time, can you give us your feedback on #961?
Thank you.

@crutkas crutkas added this to the 20.02 release milestone Feb 11, 2020
@crutkas crutkas added this to To do in 0.16 release via automation Feb 11, 2020
@crutkas crutkas modified the milestones: 20.02 release, 20.03 release Feb 11, 2020
@crutkas
Copy link
Member

crutkas commented Feb 11, 2020

putting in 20.03, we may be able to pull this in sooner per @enricogior

@enricogior enricogior removed this from the 20.03 release milestone Feb 27, 2020
@yuyoyuppe yuyoyuppe added Cost-Medium Medium work item - 1-3 Days worth of work. Status-In progress This issue or work-item is under development labels Mar 11, 2020
@yuyoyuppe
Copy link
Collaborator

yuyoyuppe commented Mar 18, 2020

I've rebased the branch to the current master (cherry-picked line by line manually, because every commit had a conflict almost in every file) and considered the proposed idea. It looks like we need to at least partially implement #961 for it to work, so I'm reestimating this a bit. What we need to do:

  • implement the ability to un/subscribe to a certain event dynamically for a certain toy
  • fix the way modules()/powertoys_events() work. currently it's trivial to create a race condition from using them (and we may already have one)
  • settle on the design of Multithreaded design simplification  #961, so we don't do any incompatible features

To finish #961:

  • change the way we launch toys, so the events are always served from a particular thread
  • change the existing PT to accommodate with the updated event system (we can't have two versions of event system at the same time due to interface change and the way hooks work)

I've just finished drafting the design, but need to test some concepts, though it seems like a viable task for 0.16 nevertheless.

@yuyoyuppe yuyoyuppe added Cost-Large Large work item - 3+ days worth of work (chances are needs to be broken down) and removed Cost-Medium Medium work item - 1-3 Days worth of work. Status-In progress This issue or work-item is under development labels Mar 18, 2020
@crutkas crutkas added this to To do in 0.18 release via automation Mar 24, 2020
@crutkas crutkas removed this from To do in 0.16 release Mar 24, 2020
@crutkas
Copy link
Member

crutkas commented Mar 24, 2020

moving this toward 0.18 and 0.19 to wrap up. will be done with setting simplification work

@yuyoyuppe yuyoyuppe added the Status-In progress This issue or work-item is under development label Apr 30, 2020
@yuyoyuppe yuyoyuppe added Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed Status-In progress This issue or work-item is under development labels May 1, 2020
@enricogior enricogior moved this from To do to Done in 0.18 release May 1, 2020
@enricogior
Copy link
Contributor

The fix is now available in 0.18 https://github.com/microsoft/PowerToys/releases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Quality Stability, Performance, Etc. Cost-Large Large work item - 3+ days worth of work (chances are needs to be broken down) Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
No open projects
0.18 release
  
Done
Development

No branches or pull requests

4 participants