8000 bpo-46323: Reduce stack usage of ctypes python callback function. by corona10 · Pull Request #31224 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-46323: Reduce stack usage of ctypes python callback function. #31224

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Feb 9, 2022
Merged

bpo-46323: Reduce stack usage of ctypes python callback function. #31224

merged 6 commits into from
Feb 9, 2022

Conversation

corona10
Copy link
Member
@corona10 corona10 commented Feb 8, 2022 •
edited
Loading

https://bugs.python.org/issue46323

Mean +- std dev: [before-vectorcall] 1.29 us +- 0.02 us -> [opt] 1.12 us +- 0.02 us: 1.16x faster


Mean +- std dev: [main] 1.20 us +- 0.02 us -> [opt] 1.12 us +- 0.02 us: 1.07x faster

@corona10 corona10 requested a review from vstinner February 8, 2022 22:54
corona10
corona10 commented Feb 8, 2022
View reviewed changes
Modules/_ctypes/callbacks.c
@@ -16,6 +16,11 @@
#include <ffi.h>
#include "ctypes.h"

#ifdef HAVE_ALLOCA_H
Copy link
Member Author

Choose a reason for hiding this comment

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

Same workaroud

cpython/Modules/_ctypes/callproc.c

Lines 80 to 84 in 06e1701

#include "ctypes.h"
#ifdef HAVE_ALLOCA_H
/* AIX needs alloca.h for alloca() */
#include <alloca.h>
#endif

@corona10 corona10 changed the title bpo-46323 Fix stack allocation to avoid stackoverflow bpo-46323: Fix stack allocation to avoid stackoverflow Feb 8, 2022
@corona10
Copy link
Member Author
corona10 commented Feb 8, 2022

@erlend-aasland

@corona10 corona10 force-pushed the bpo-46323-fix branch 2 times, most recently from 52f5730 to dcabd85 Compare February 8, 2022 23:12
vstinner
vstinner reviewed Feb 9, 2022
View reviewed changes
vstinner
vstinner reviewed Feb 9, 2022
View reviewed changes
@corona10 corona10 requested a review from vstinner February 9, 2022 01:06
@corona10 corona10 closed this Feb 9, 2022
@corona10 corona10 reopened this Feb 9, 2022
@corona10 corona10 requested a review from sweeneyde February 9, 2022 01:53
@sweeneyde
Copy link
Member

To clarify, this prevents stackoverflow in situations with repeated calls where argcount is small, but will have roughly the same amount of stack usage when argcount is around 1000 (very rare) , right?

@corona10
Copy link
Member Author
corona10 commented Feb 9, 2022 •
edited
Loading

@sweeneyde Correct and also a little performance issue (See benchmark)

vstinner
vstinner reviewed Feb 9, 2022
View reviewed changes
vstinner
vstinner reviewed Feb 9, 2022
View reviewed changes
@corona10 corona10 requested a review from vstinner February 9, 2022 17:16
vstinner
vstinner approved these changes Feb 9, 2022
View reviewed changes
Copy link
Member
@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@sweeneyde
Copy link
Member

I think the title could be improved: "Reduce stack usage in Ctypes calls" or something.

Right now, the mismatch is confusing between the title implying "use less stack" and the NEWS entry implying "use more stack".

sweeneyde
sweeneyde reviewed Feb 9, 2022
View reviewed changes
@corona10 corona10 changed the title bpo-46323: Fix stack allocation to avoid stackoverflow bpo-46323: Reduce stack usage while calling a ctypes python callback function. Feb 9, 2022
@corona10 corona10 changed the title bpo-46323: Reduce stack usage while calling a ctypes python callback function. bpo-46323: Reduce stack usage of ctypes python callback function. Feb 9, 2022
@corona10
Copy link
Member Author
corona10 commented Feb 9, 2022 •
edited
Loading

@sweeneyde

I think the title could be improved: "Reduce stack usage in Ctypes calls" or something.

I've changed the title, Looks proper?
'Reduce stack usage of ctypes python callback function.'

sweeneyde
sweeneyde reviewed Feb 9, 2022
View reviewed changes
Misc/NEWS.d/next/Core and Builtins/2022-02-10-02-29-12.bpo-46323.HK_cs0.rst
@@ -0,0 +1,3 @@
:mod:`ctypes` now allocates memory on the stack instead of on the heap
Copy link
Member

Choose a reason for hiding this comment

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

There was no heap allocation directly before this PR, and there will be no heap allocation after, right? It's just eliminating that dead code path.

Copy link
Member Author
@corona10 corona10 Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@sweeneyde Yeah but this is the result of bpo-46323

Copy link
Member Author

Choose a reason for hiding this comment

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

Victor and I decided to add NEWS entry for all this change at this moment.
Theoretically, we had to add NEWS entry at the first PR, but it's too late.

@sweeneyde
Copy link
< 67ED div class="d-none d-sm-flex"> Member

The change and title LGTM!

@corona10
Copy link
Member Author
corona10 commented Feb 9, 2022

@sweeneyde
Thanks!

@corona10 corona10 merged commit d18120c into python:main Feb 9, 2022
@corona10 corona10 deleted the bpo-46323-fix branch February 9, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sweeneyde sweeneyde sweeneyde left review comments

@vstinner vstinner vstinner approved these changes

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@corona10 @sweeneyde @vstinner @the-knights-who-say-ni @bedevere-bot
0