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: add explicit noexcept qualifiers to resolve perf regressions (Cython 3) #4390

Merged

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Mar 29, 2023

PR Summary

As tested on my fork, this resolves most of the performance regressions seen in smoke tests with bleeding-edge CI (getting testing airtime from ~17min to ~8 min).

follow up to #4386

@matthewturk
Copy link
Member

Am I understanding that our global settings don't propagate to PXD files?

@neutrinoceros
Copy link
Member Author

which global settings ?

@neutrinoceros neutrinoceros marked this pull request as ready for review March 29, 2023 21:32
@matthewturk
Copy link
Member

Oh, I thought you tested it with setting the noexcept globally -- I thought there was a compiloer option to revert to previous settings.

@neutrinoceros
Copy link
Member Author

Disabling exception handling globally is only possible Cython 3, so we can't do that until we drop Cython 0.29. noexcept is backported as a noop to 0.29.x so it is the only option to transition smoothly. I don't want to add it blindly everywhere so instead I'm targeting functions where regressions are seen :)

@matthewturk
Copy link
Member

Ah, I understand. I think for some reason I thought we gatekept the opt behind a version check. But this is fine.

@neutrinoceros neutrinoceros marked this pull request as draft March 30, 2023 16:54
@neutrinoceros
Copy link
Member Author

pushed a bunch more noexcept for SelectorObject.select_* routines, which I found were responsible for regressions too.
I'm testing the current state of the branch on my fork at https://github.com/neutrinoceros/yt/actions/runs/4566963443

@neutrinoceros neutrinoceros marked this pull request as ready for review March 30, 2023 18:14
@neutrinoceros
Copy link
Member Author

This is not over (performance is still degraded on Cython 3.0.0b2), but we're getting there. I'd prefer to keep my PRs small so I'll wait for this one to be included before I dig further.

@matthewturk matthewturk merged commit 1864708 into yt-project:main Mar 31, 2023
@matthewturk
Copy link
Member

I think one thing that's been slightly unexpected that we're learning here is that some of the inline functions also need the declarations, even though we want them inlined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants