FIX: macos: Use standard NSApp run loop in our input hook #28981

greglucas · 2024-10-16T03:31:48Z

PR summary

We want to defer back to Python's native input hook handling when we don't have any figures to interact with, otherwise our runloop will continue running unecessarily.

Additionally, we need to add a short runloop burst in our hotloop to avoid saturating the CPU while waiting for input. (i.e. sitting on an idle figure would keep 100% cpu while waiting for an input() event without this runloop trigger).

closes #28960

PR checklist

"closes #0000" is in the body of the PR description to link the related issue
[n/a] new and changed code is tested
[n/a] Plotting related features are demonstrated in an example
[n/a] New Features and API Changes are noted with a directive and release note
[n/a] Documentation complies with general and docstring guidelines

greglucas · 2024-10-16T03:39:43Z

Running the example from #26869 on current main and waiting for input() would produce 100% cpu too. So the fix in #27528 to remove the runloop call actually caused that 100% cpu usage because the while(True) is running too frequently and saturating the CPU.

The removal of the runloop timer was to fix this issue: #27515 I think that was the wrong "fix" of the issue, and the real way to fix that issue is to remove our own input hook and yield back to Python's terminal handling.

In all of these examples, we should be at minimal CPU usage when sitting still. I'm still not sure how we could test/verify this? But it would be nice to add something to make sure we don't regress on this in the future.

QuLogic · 2024-10-18T05:03:04Z

src/_macosx.m

@@ -1122,7 +1128,7 @@ - (void)close
 {
    [super close];
    --FigureWindowCount;
-    if (!FigureWindowCount) [NSApp stop: self];
+    if (!FigureWindowCount) stop(NULL);


This leaks the None reference returned from stop (at least on versions before it became immortal).

👍 Added a decref to the return object now. I could change this back to the original [NSApp stop: self] and add the PyOS_InputHook = NULL call here too instead if that is better? I just figured it might be nice to try and go through one stop function everywhere we need it, although it is only in two places currently.

We want to defer back to Python's native input hook handling when we don't have any figures to interact with, otherwise our runloop will continue running unecessarily. Additionally, we need to add a short runloop burst in our hotloop to avoid saturating the CPU while waiting for input. (i.e. sitting on an idle figure would keep 100% cpu while waiting for an input() event without this runloop trigger).

tacaswell · 2024-10-20T14:42:07Z

@ksunden and I spent a while looking at this on Friday, but not sure where that landed / if he pushed anything.

greglucas · 2024-10-21T13:41:23Z

Feel free to push directly here or open a separate PR and I'm happy to review it too.

ksunden · 2024-10-21T20:37:04Z

You can see what I have been trying in https://github.com/ksunden/matplotlib/tree/macos-pyos-input-hook

Essentially:

removing the hook actually causes some problems (most notably plt.close("all") did not actually close windows until the event loop was started again)
If we don't remove the hook, I don't think there is much reason to change around the other stopping logic here.
I think this PR only solves the CPU usage by virtue of inserting what effectively amounts to a sleep
- In changing things, I went to just run the loop and adjust how we break from the loop, I discovered that it appears that the loop referenced here is not actually the main app runloop, so effectively the new line is just a sleep.
- I can get mostly working replacing our entire double while loop setup with [NSApp run];, but run into problems after closing plots (specifically via the window's X button) wherein a single character is processed by the repl, but then it hangs with 160% CPU usage and I'm not sure where/why.

greglucas · 2024-10-21T21:30:08Z

FYI, I think v3.6.3 is where some of this logic was referenced from with the double loop and breaking out of it:

matplotlib/src/_macosx.m

Lines 149 to 169 in abcbf3d

    
           while (true) { 
        
               while (true) { 
        
                   event = [NSApp nextEventMatchingMask: NSEventMaskAny 
        
                                              untilDate: [NSDate distantPast] 
        
                                                 inMode: NSDefaultRunLoopMode 
        
                                                dequeue: YES]; 
        
                   if (!event) { break; } 
        
                   [NSApp sendEvent: event]; 
        
               } 
        
               CFRunLoopRun(); 
        
               if (interrupted || CFReadStreamHasBytesAvailable(stream)) { break; } 
        
           } 
        
           if (py_sigint_handler) { PyOS_setsig(SIGINT, py_sigint_handler); } 
        
           CFReadStreamUnscheduleFromRunLoop( 
        
               stream, runloop, kCFRunLoopCommonModes); 
        
           if (sigint_socket) { CFSocketInvalidate(sigint_socket); } 
        
           if (!error) { 
        
               close(channel[0]); 
        
               close(channel[1]); 
        
           }

specifically, there was the call to the cfrunloop:

matplotlib/src/_macosx.m

Line 158 in abcbf3d

CFRunLoopRun();

which is what the "sleep" you mention is from.

tacaswell · 2024-10-30T03:25:19Z

src/_macosx.m

+    // Short circuit if no windows are active
+    // Rely on Python's input handling to manage CPU usage
+    // This queries the NSApp, rather than using our FigureWindowCount because that is decremented when events still
+    // need to be processed to properly close the windows.


This will also make us play nicer with any other OSX apps you launch from Python (which I am not aware of any others....)

Should we use this method in the close() method too instead of keeping track of it ourselves?

There are some folks embedding this in C++/objc it would appear: #27147

That is its own bit of fun, but I think this will not intersect with that issue because the only place the PyOS_InputHook is run is from the Python repl (either readline or the new one) so if they are embedding Python in another application (which means they make an interpreter object and then stuff strings into it to run Python code) so I expect the repl will never run.

oooh, just after I clicked comment I connected some more dots.

Yes, we probably should be using something like this in close and than may fix #27147 .

tacaswell · 2024-10-30T03:26:06Z

src/_macosx.m

@@ -192,16 +216,16 @@ void process_event(char const* cls_name, char const* fmt, ...)
 static bool backend_inited = false;

 static void lazy_init(void) {
+    // Run our own event loop while waiting for stdin on the Python side
+    // this is needed to keep the application responsive while waiting for input
+    PyOS_InputHook = wait_for_stdin;


can probably move this back down so we are not re-setting it all the time?

greglucas

Nice updates @ksunden, thanks for pushing those!

I verified that this new change fixes the slow input typing lag regression I saw before as well.

greglucas · 2024-10-30T12:13:25Z

src/_macosx.m

+        NSEvent* event = [NSApp nextEventMatchingMask: NSEventMaskAny
+                                   untilDate: [NSDate distantPast]
+                                      inMode: NSDefaultRunLoopMode
+                                     dequeue: YES];


We've been aligning on : it seems like, so suggest doing the same here and below after the notificationID indent now.

The name of this function is flushEvents() but above you added stop_with_event() is there a common form we want to use between them?

Suggested change

NSEvent* event = [NSApp nextEventMatchingMask: NSEventMaskAny

untilDate: [NSDate distantPast]

inMode: NSDefaultRunLoopMode

dequeue: YES];

NSEvent* event = [NSApp nextEventMatchingMask: NSEventMaskAny

untilDate: [NSDate distantPast]

inMode: NSDefaultRunLoopMode

dequeue: YES];

greglucas · 2024-10-30T12:17:48Z

src/_macosx.m

+    // Short circuit if no windows are active
+    // Rely on Python's input handling to manage CPU usage
+    // This queries the NSApp, rather than using our FigureWindowCount because that is decremented when events still
+    // need to be processed to properly close the windows.


Should we use this method in the close() method too instead of keeping track of it ourselves?

tacaswell

But I was on a call with Kyle as we worked through this so this should probably count as less than 1 full review.

greglucas · 2024-10-30T19:26:10Z

I have run through this with a few different test cases as well and it looks good to me and is a nice simplification. It has been significantly changed by @ksunden, so I would consider it his PR now :) I "approve", although I can not technically click the button on my own PR.

tacaswell · 2024-10-30T20:05:27Z

I'm going to go ahead and merge this.

… in our input hook

…981-on-v3.9.x Backport PR #28981 on branch v3.9.x (FIX: macos: Use standard NSApp run loop in our input hook)

greglucas added GUI: MacOSX Performance labels Oct 16, 2024

QuLogic added this to the v3.9.3 milestone Oct 17, 2024

QuLogic reviewed Oct 18, 2024

View reviewed changes

greglucas force-pushed the macos-pyos-input-hook branch from 298243c to b1822cf Compare October 18, 2024 12:08

Rework stdin handling on macos objective C

18909c7

Short circuit input hook when no windows are open

0e8baa5

tacaswell reviewed Oct 30, 2024

View reviewed changes

greglucas commented Oct 30, 2024

View reviewed changes

Review comments

b2eaad1

tacaswell approved these changes Oct 30, 2024

View reviewed changes

ksunden mentioned this pull request Oct 30, 2024

[Bug]: Embedded Python uncatchable hard exit on closing last matplotlib window. #27147

Open

ksunden changed the title ~~FIX: macos: remove our custom PyOS_InputHook after our runloop stops~~ FIX: macos: Use standard NSApp run loop in our input hook Oct 30, 2024

tacaswell merged commit 4468488 into matplotlib:main Oct 30, 2024
40 of 41 checks passed

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 30, 2024

Backport PR matplotlib#28981: FIX: macos: Use standard NSApp run loop…

985e8f4

… in our input hook

meeseeksmachine mentioned this pull request Oct 30, 2024

Backport PR #28981 on branch v3.9.x (FIX: macos: Use standard NSApp run loop in our input hook) #29046

Merged

QuLogic added a commit that referenced this pull request Oct 31, 2024

Merge pull request #29046 from meeseeksmachine/auto-backport-of-pr-28…

3157f23

…981-on-v3.9.x Backport PR #28981 on branch v3.9.x (FIX: macos: Use standard NSApp run loop in our input hook)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: macos: Use standard NSApp run loop in our input hook #28981

FIX: macos: Use standard NSApp run loop in our input hook #28981

greglucas commented Oct 16, 2024

greglucas commented Oct 16, 2024

QuLogic Oct 18, 2024

greglucas Oct 18, 2024

tacaswell commented Oct 20, 2024

greglucas commented Oct 21, 2024

ksunden commented Oct 21, 2024

greglucas commented Oct 21, 2024

tacaswell Oct 30, 2024

greglucas Oct 30, 2024

greglucas Oct 30, 2024

tacaswell Oct 30, 2024

tacaswell Oct 30, 2024

tacaswell Oct 30, 2024

greglucas left a comment

greglucas Oct 30, 2024

greglucas Oct 30, 2024

tacaswell left a comment

greglucas commented Oct 30, 2024

tacaswell commented Oct 30, 2024

FIX: macos: Use standard NSApp run loop in our input hook #28981

FIX: macos: Use standard NSApp run loop in our input hook #28981

Conversation

greglucas commented Oct 16, 2024

PR summary

PR checklist

greglucas commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Oct 20, 2024

greglucas commented Oct 21, 2024

ksunden commented Oct 21, 2024

greglucas commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglucas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell left a comment

Choose a reason for hiding this comment

greglucas commented Oct 30, 2024

tacaswell commented Oct 30, 2024