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

err: don't use with_gil internally in PyErr::new() #1724

Merged
merged 2 commits into from
Jul 24, 2021

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jul 13, 2021

This is a partial follow-on from #1683

If we're going to remove the optimization of using PyO3's thread-local to check for the GIL being held in with_gil, then it seems desirable that PyErr::new really doesn't need the GIL at all.

I added a benchmark to accompany the change; it only slightly impacts performance (<2% slower and this is on the error path anyway). It also has the nice side effect that users already creating PyErr outside of a GIL will get a significant speedup, because the GIL won't be temporarily acquired.

@messense
Copy link
Member

messense commented Jul 16, 2021

Warning: Performance alert! Previous value was 5255746.530213239 and current value is 2283663.643596061. It is 2.301453869947796x worse than previous exceeding a ratio threshold 2
Warning: Performance alert! Previous value was 6427231.592283239 and current value is 2608965.9845569944. It is 2.4635168224987765x worse than previous exceeding a ratio threshold 2

This action is dumb. 😅 Looks like it's intended for pytest for whatever reasons: https://github.com/rhysd/github-action-benchmark/blob/a1914d7dcbe14d006e4b5f12c7ff303a82a411f1/src/write.ts#L68

@davidhewitt
Copy link
Member Author

Heh indeed, it could use some work on output formatting! I would write a PR myself but it looks like the maintainer isn't looking after it, and I don't want to devote time to maintaining a fork... https://github.com/rhysd/github-action-benchmark/pulls

@davidhewitt
Copy link
Member Author

Anyone got any comments or concerns here, or should I look to merge this?

@davidhewitt
Copy link
Member Author

Proceeding to merge this!

@davidhewitt davidhewitt merged commit 4a71f82 into PyO3:main Jul 24, 2021
@davidhewitt davidhewitt deleted the err-new-no-gil branch July 24, 2021 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants