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 tolink the related issue
[n/a] new and changed code istested
[n/a]Plotting related features are demonstrated in anexample
[n/a]New Features andAPI Changes are noted with adirective and release note
[n/a] Documentation complies withgeneral anddocstring guidelines

greglucas added GUI: MacOSX Performance labels

Oct 16, 2024

Copy link

ContributorAuthor

greglucas commentedOct 16, 2024

Running the example from#26869 on current main and waiting forinput() 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 added this to thev3.9.3 milestone

Oct 17, 2024

QuLogic reviewed

Oct 18, 2024

View reviewed changes

src/_macosx.m Outdated

		[superclose];
		--FigureWindowCount;
		if (!FigureWindowCount)[NSAppstop:self];
		if (!FigureWindowCount)stop(NULL);

Copy link

Member

QuLogicOct 18, 2024

Choose a reason for hiding this comment

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

This leaks theNone reference returned fromstop (at least on versions before it became immortal).

Copy link

ContributorAuthor

greglucasOct 18, 2024

Choose a reason for hiding this comment

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

👍 Added a decref to the return object now. I could change this back to the original[NSApp stop: self] and add thePyOS_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.

FIX: macos: remove our custom PyOS_InputHook after our runloop stops

b1822cf

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

greglucas force-pushed themacos-pyos-input-hook branch from298243c tob1822cfCompare

October 18, 2024 12:08

Rework stdin handling on macos objective C

18909c7

Copy link

Member

tacaswell commentedOct 20, 2024

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

Copy link

ContributorAuthor

greglucas commentedOct 21, 2024

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

Copy link

Member

ksunden commentedOct 21, 2024

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

Essentially:

removing the hook actually causes some problems (most notablyplt.close("all") did not actually close windows until the event loop was started again)
If we don't remove the hook, I don'tthink 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.

Copy link

ContributorAuthor

greglucas commentedOct 21, 2024

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 inabcbf3d

	while (true) {
	while (true) {
	event = [NSAppnextEventMatchingMask: NSEventMaskAny
	untilDate: [NSDatedistantPast]
	inMode:NSDefaultRunLoopMode
	dequeue:YES];
	if (!event) {break; }
	[NSAppsendEvent: 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 inabcbf3d

CFRunLoopRun();

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

Short circuit input hook when no windows are open

0e8baa5

tacaswell reviewed

Oct 30, 2024

View reviewed changes

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.

Copy link

Member

tacaswellOct 30, 2024

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

greglucasOct 30, 2024

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

greglucasOct 30, 2024

Choose a reason for hiding this comment

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

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

Copy link

Member

tacaswellOct 30, 2024

Choose a reason for hiding this comment

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

That is its own bit of fun, but I think this will not intersect with that issue because the only place thePyOS_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.

Copy link

Member

tacaswellOct 30, 2024

Choose a reason for hiding this comment

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

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

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

tacaswell reviewed

Oct 30, 2024

View reviewed changes

src/_macosx.m Outdated

		staticvoidlazy_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;

Copy link

Member

tacaswellOct 30, 2024

Choose a reason for hiding this comment

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

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

greglucas commented

Oct 30, 2024

View reviewed changes

Copy link

ContributorAuthor

greglucas left a comment

Choose a reason for hiding this comment

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

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.

src/_macosx.m Outdated

Comment on lines 72 to 75

		NSEvent* event = [NSAppnextEventMatchingMask: NSEventMaskAny
		untilDate: [NSDatedistantPast]
		inMode:NSDefaultRunLoopMode
		dequeue:YES];

Copy link

ContributorAuthor

greglucasOct 30, 2024

Choose a reason for hiding this comment

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

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

The name of this function isflushEvents() but above you addedstop_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];

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.

Copy link

ContributorAuthor

greglucasOct 30, 2024

Choose a reason for hiding this comment

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

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

Review comments

b2eaad1

tacaswell approved these changes

Oct 30, 2024

View reviewed changes

Copy link

Member

tacaswell left a comment

Choose a reason for hiding this comment

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

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

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

Copy link

ContributorAuthor

greglucas commentedOct 30, 2024

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.

Copy link

Member

tacaswell commentedOct 30, 2024

I'm going to go ahead and merge this.

tacaswell merged commit4468488 intomatplotlib: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 PRmatplotlib#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#29046from meeseeksmachine/auto-backport-of-pr-28…

3157f23

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

Labels

GUI: MacOSX Performance

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

greglucas commentedOct 16, 2024

PR summary

PR checklist

Uh oh!

greglucas commentedOct 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commentedOct 20, 2024

Uh oh!

greglucas commentedOct 21, 2024

Uh oh!

ksunden commentedOct 21, 2024

Uh oh!

greglucas commentedOct 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglucas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

greglucas commentedOct 30, 2024

Uh oh!

tacaswell commentedOct 30, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants