Skip to content

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

Merged
corona10 merged 6 commits intopython:mainfrom
corona10:bpo-46323-fix
Feb 9, 2022
Merged

bpo-46323: Reduce stack usage of ctypes python callback function.#31224
corona10 merged 6 commits intopython:mainfrom
corona10:bpo-46323-fix

Conversation

@corona10
Copy link
Copy Markdown
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 Outdated
Copy link
Copy Markdown
Member Author

@corona10 corona10 Feb 8, 2022

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
Copy Markdown
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
Copy Markdown
Member

sweeneyde commented Feb 9, 2022

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

sweeneyde commented Feb 9, 2022

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
Copy Markdown
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
Copy Markdown
Member

@sweeneyde sweeneyde Feb 9, 2022

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
Copy Markdown
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
Copy Markdown
Member Author

@corona10 corona10 Feb 9, 2022

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
Copy Markdown
Member

sweeneyde commented Feb 9, 2022

The change and title LGTM!

@corona10
Copy link
Copy Markdown
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