bpo-45256: Remove the usage of the C stack in Python to Python calls #28488

pablogsal · 2021-09-21T10:42:58Z

Remove the usage of the cstack in Python to Python calls
Fix gdb
Move depth to frame structure for easier retrieval in debugging tools
Refactor and simplifications

https://bugs.python.org/issue45256

bedevere-bot · 2021-09-21T10:47:52Z

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit b4149b6d80bc85c73104ca0f98ee3754cf5d485c 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

Include/internal/pycore_frame.h

markshannon

Looking good. A few minor issues.

Tools/gdb/libpython.py

markshannon · 2021-09-21T11:04:50Z

Python/ceval.c

+static InterpreterFrame*
+_PyEval_FrameFromPyFunctionAndArgs(PyThreadState *tstate, PyObject* const *args, int nargs, PyObject *function) {
+    assert(PyFunction_Check(function));
+    size_t nargsf = nargs | PY_VECTORCALL_ARGUMENTS_OFFSET;


nargsf and vector_nargs below are only used in asserts.
I think all the asserts and assignments to nargsf and vector_nargs could be replaced with assert(nargs == 0 || args != NULL);

Include/internal/pycore_frame.h

Python/ceval.c

bedevere-bot · 2021-09-21T11:04:54Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

pablogsal · 2021-09-21T11:12:06Z

@markshannon Damn, unfortunately something is going on with Windows:

D:\a\cpython\cpython\PCbuild\python.vcxproj(125,5): warning MSB3073: The command "setlocal
D:\a\cpython\cpython\PCbuild\python.vcxproj(125,5): warning MSB3073: set PYTHONPATH=D:\a\cpython\cpython\Lib
D:\a\cpython\cpython\PCbuild\python.vcxproj(125,5): warning MSB3073: "D:\a\cpython\cpython\PCbuild\win32\python.exe" "D:\a\cpython\cpython\PC\validate_ucrtbase.py" ucrtbase" exited with code -1073741819.
D:\a\cpython\cpython\PCbuild\python.vcxproj(125,5): warning MSB4181: The "Exec" task returned false but did not log an error.
D:\a\cpython\cpython\PCbuild\regen.targets(107,5): error MSB3073: The command "D:\a\cpython\cpython\PCbuild\win32\python.exe Programs\freeze_test_frozenmain.py Programs/test_frozenmain.h" exited with code -1073741819. [D:\a\cpython\cpython\PCbuild\python.vcxproj]
    6 Warning(s)
    1 Error(s)

Time Elapsed 00:04:54.99

Apparently that is a File System error but I don't understand why this may be happening :(

pablogsal · 2021-09-21T13:24:45Z

@markshannon Oh, I found the source of the Windows failure: it was an existing bug in the PREDICT() macro for the code path without computed gotos. Fixed it in 50276ad8813bfd0bb0455d0e699e2ee301071f00

That was challenging to find indeed!

bedevere-bot · 2021-09-21T13:38:11Z

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 50276ad8813bfd0bb0455d0e699e2ee301071f00 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

Fidget-Spinner

This is really cool. I just have a few comments on readability. No idea why ASAN is failing tbh.

One other concern I have is that this slows down non-python function calls very slightly (with the two additional checks). I highly doubt it's measurable on pyperformance though.

Python/ceval.c

Fidget-Spinner · 2021-09-21T14:46:23Z

Python/ceval.c

+            Py_DECREF(function);
+            _PyFrame_SetStackPointer(frame, stack_pointer);
+            new_frame->depth = frame->depth + 1;
+            tstate->frame = frame = new_frame;


Note to self, no need to pop frame here because it's done on eval frame exit at exit_eval_frame.

Python/ceval.c

Tools/gdb/libpython.py

pablogsal · 2021-09-21T14:57:16Z

I cannot reproduce the ASAN bug. :(

Fidget-Spinner · 2021-09-21T16:15:46Z

There're refleaks but I can't pinpoint where :(

python.exe -m test test_grammar test_builtin -R 3:3
0:00:00 Run tests sequentially
0:00:00 [1/2] test_grammar
beginning 6 repetitions
123456
......
test_grammar leaked [12, 12, 12] references, sum=36
0:00:01 [2/2/1] test_builtin -- test_grammar failed (reference leak)
beginning 6 repetitions
123456
......
test_builtin leaked [13, 13, 13] references, sum=39
test_builtin failed (reference leak)

== Tests result: FAILURE ==

Ths commit inlines calls to Python functions in the eval loop and steals all the arguments in the call from the caller for performance.

…uted gotos

pablogsal · 2021-09-29T00:04:31Z

Commit 0ddc46e fixes the reference leaks (but it needs some cleanup as it requires simplification):

❯ ./python -m test test_extcall test_inspect test_ntpath test_posixpath test_typing test_unittest -R :
0:00:00 load avg: 2.22 Run tests sequentially
0:00:00 load avg: 2.22 [1/6] test_extcall
beginning 9 repetitions
123456789
.........
0:00:00 load avg: 2.22 [2/6] test_inspect
beginning 9 repetitions
123456789
.........
0:00:06 load avg: 2.21 [3/6] test_ntpath
beginning 9 repetitions
123456789
.........
0:00:07 load avg: 2.35 [4/6] test_posixpath
beginning 9 repetitions
123456789
.........
0:00:08 load avg: 2.35 [5/6] test_typing
beginning 9 repetitions
123456789
.........
0:00:09 load avg: 2.35 [6/6] test_unittest
beginning 9 repetitions
123456789
.........
test_unittest passed in 34.2 sec

== Tests result: SUCCESS ==

All 6 tests OK.

Total duration: 43.6 sec
Tests result: SUCCESS

bedevere-bot · 2021-09-29T00:04:46Z

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 0ddc46e25e4885562f7d1e962b9b30eea2b0283d 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

markshannon · 2021-09-29T11:41:25Z

https://wiki.python.org/moin/DebuggingWithGdb

which is quite suboptimal. Maybe I should add a new section to the official docs. I am happy to pack that into this PR but maybe we should do it separately.

What do you think @markshannon ?

I hadn't realized the docs were so sparse. Probably best to do it in another PR.
I've opened https://bugs.python.org/issue45317 so we don't forget.

pablogsal · 2021-09-29T12:17:56Z

@markshannon what do you think of 0ddc46e for now? I dislike this approach (is correct, but is difficult to reason about if you don't have the full picture in mind). Should we do the refactor now, should we do a smaller refactor or should we just do some celanup of 0ddc46e ?

markshannon · 2021-09-30T14:33:13Z

@markshannon what do you think of 0ddc46e for now? I dislike this approach (is correct, but is difficult to reason about if you don't have the full picture in mind). Should we do the refactor now, should we do a smaller refactor or should we just do some celanup of 0ddc46e ?

Just do the minimal cleanup of 0ddc46e that you are happy with for now.
And add a comment as to why it is correct.

bedevere-bot · 2021-10-07T09:58:43Z

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 1de88f6 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

pablogsal · 2021-10-07T14:07:06Z

@markshannon I had to fix some merge conflicts and I have done the cleanup. I am building again with the buildbot fleet but this should be ready for a first version.

bedevere-bot · 2021-10-07T14:07:20Z

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit f313e8c 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

Fidget-Spinner

The C changes LGTM. Some minor formatting nits at https://github.com/python/cpython/pull/28488/files#r717573235 and below.

Huge disclaimer: I am not a Python-GDB expert! It would be better to have someone else reviewing those changes.

Fidget-Spinner · 2021-10-07T14:16:33Z

Python/ceval.c

+            PyObject *function = PEEK(oparg + 1);
+            if (Py_TYPE(function) == &PyFunction_Type) {
+                PyCodeObject *code = (PyCodeObject*)PyFunction_GET_CODE(function);
+                PyObject *locals = code->co_flags & CO_OPTIMIZED ? NULL : PyFunction_GET_GLOBALS(function);


The code from here onwards has waay more than 80 characters.

pablogsal · 2021-10-07T14:22:12Z

@Fidget-Spinner Thanks for the review!

I am not going to push anything new until the buildbots pass to not restart the long refleak builds yet again. I will fix the formatting issues in a new PR unless @markshannon wants to change something fundamental (I also plan to rename the frame->depth in another PR).

Fidget-Spinner · 2021-10-07T14:23:59Z

I am not going to push anything new until the buildbots pass to not restart the long refleak builds yet again.

I was just about to mention that. Good call!

Fidget-Spinner · 2021-10-09T04:10:39Z

Python/ceval.c

+            // *valid* arguments (i.e. the ones that fit into the frame). 
+            PyCodeObject *co = (PyCodeObject*)con->fc_code;
+            const Py_ssize_t total_args = co->co_argcount + co->co_kwonlyargcount;
+            for (Py_ssize_t i = 0; i < Py_MIN(argcount, total_args); i++) {


GHA is warning:

comparison of integer expressions of different signedness: ‘Py_ssize_t’ {aka ‘long int’} and ‘long unsigned int’ [-Wsign-compare

and

'>': signed/unsigned mismatch [D:\a\cpython\cpython\PCbuild\pythoncore.vcxproj]

Suggested change

for (Py_ssize_t i = 0; i < Py_MIN(argcount, total_args); i++) {

for (size_t i = 0; i < Py_MIN(argcount, (size_t)total_args); i++) {

pablogsal · 2021-10-09T16:08:06Z

Cleanups happening here: #28836

@markshannon After that is merged, I will create a PR to address the counter to convert it to a "entry frame" flag.

vstinner · 2021-10-11T08:56:12Z

This is really cool, nice work @pablogsal ;-)

pablogsal requested a review from markshannon as a code owner Sep 21, 2021

the-knights-who-say-ni added the CLA signed label Sep 21, 2021

bedevere-bot added the awaiting core review label Sep 21, 2021

pablogsal added skip news 🔨 test-with-buildbots Test PR w/ buildbots; report in status section and removed 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Sep 21, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 21, 2021

pablogsal commented Sep 21, 2021

View changes

Include/internal/pycore_frame.h Show resolved Hide resolved

pablogsal force-pushed the simple_cframe branch from b4149b6 to 415273d Compare Sep 21, 2021

markshannon requested changes Sep 21, 2021

View changes

bedevere-bot added awaiting changes and removed awaiting core review labels Sep 21, 2021

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 21, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 21, 2021

Fidget-Spinner reviewed Sep 21, 2021

View changes

pablogsal force-pushed the simple_cframe branch from 98fced8 to 21b7d29 Compare Sep 21, 2021

pablogsal added 5 commits Sep 21, 2021

Remove the usage of the cstack in Python to Python calls

8a88fd3

Ths commit inlines calls to Python functions in the eval loop and steals all the arguments in the call from the caller for performance.

Fix gdb

799e366

Move depth to frame structure for easier retrieval in debugging tools

306e29f

Refactor and simplifications

fc36be1

Fix existing bug in the PREDICT() macro for the codepath without comp…

e1091e0

…uted gotos

pablogsal force-pushed the simple_cframe branch from 21b7d29 to e1091e0 Compare Sep 21, 2021

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 29, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 29, 2021

pablogsal mentioned this pull request Sep 29, 2021

bpo-45116 Fix another performance regression on Windows #28630

Closed

Fix reference leaks (needs cleanup)

1de88f6

pablogsal force-pushed the simple_cframe branch from 0ddc46e to 1de88f6 Compare Oct 7, 2021

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 7, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 7, 2021

Merge branch 'main' into simple_cframe

f313e8c

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 7, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 7, 2021

Fidget-Spinner approved these changes Oct 7, 2021

View changes

bedevere-bot added awaiting merge and removed awaiting changes labels Oct 7, 2021

Fidget-Spinner reviewed Oct 9, 2021

View changes

pablogsal merged commit b4903af into python:main Oct 9, 2021
76 checks passed

pablogsal deleted the simple_cframe branch Oct 9, 2021

bedevere-bot removed the awaiting merge label Oct 9, 2021

bpo-45256: Remove the usage of the C stack in Python to Python calls #28488

bpo-45256: Remove the usage of the C stack in Python to Python calls #28488

pablogsal commented Sep 21, 2021 •

edited by bedevere-bot

bedevere-bot commented Sep 21, 2021

markshannon left a comment

markshannon Sep 21, 2021

bedevere-bot commented Sep 21, 2021

pablogsal commented Sep 21, 2021

pablogsal commented Sep 21, 2021

bedevere-bot commented Sep 21, 2021

Fidget-Spinner left a comment

Fidget-Spinner Sep 21, 2021

pablogsal commented Sep 21, 2021 •

edited

Fidget-Spinner commented Sep 21, 2021

pablogsal commented Sep 29, 2021

bedevere-bot commented Sep 29, 2021

markshannon commented Sep 29, 2021

pablogsal commented Sep 29, 2021

markshannon commented Sep 30, 2021

bedevere-bot commented Oct 7, 2021

pablogsal commented Oct 7, 2021

bedevere-bot commented Oct 7, 2021

Fidget-Spinner left a comment

Fidget-Spinner Oct 7, 2021

pablogsal commented Oct 7, 2021 •

edited

Fidget-Spinner commented Oct 7, 2021

Fidget-Spinner Oct 9, 2021

pablogsal commented Oct 9, 2021 •

edited

vstinner commented Oct 11, 2021

	for (Py_ssize_t i = 0; i < Py_MIN(argcount, total_args); i++) {
	for (size_t i = 0; i < Py_MIN(argcount, (size_t)total_args); i++) {

bpo-45256: Remove the usage of the C stack in Python to Python calls #28488

bpo-45256: Remove the usage of the C stack in Python to Python calls #28488

Conversation

pablogsal commented Sep 21, 2021 • edited by bedevere-bot

bedevere-bot commented Sep 21, 2021

markshannon left a comment

markshannon Sep 21, 2021

Choose a reason for hiding this comment

bedevere-bot commented Sep 21, 2021

pablogsal commented Sep 21, 2021

pablogsal commented Sep 21, 2021

bedevere-bot commented Sep 21, 2021

Fidget-Spinner left a comment

Fidget-Spinner Sep 21, 2021

Choose a reason for hiding this comment

pablogsal commented Sep 21, 2021 • edited

Fidget-Spinner commented Sep 21, 2021

pablogsal commented Sep 29, 2021

bedevere-bot commented Sep 29, 2021

markshannon commented Sep 29, 2021

pablogsal commented Sep 29, 2021

markshannon commented Sep 30, 2021

bedevere-bot commented Oct 7, 2021

pablogsal commented Oct 7, 2021

bedevere-bot commented Oct 7, 2021

Fidget-Spinner left a comment

Fidget-Spinner Oct 7, 2021

Choose a reason for hiding this comment

pablogsal commented Oct 7, 2021 • edited

Fidget-Spinner commented Oct 7, 2021

Fidget-Spinner Oct 9, 2021

Choose a reason for hiding this comment

pablogsal commented Oct 9, 2021 • edited

vstinner commented Oct 11, 2021

pablogsal commented Sep 21, 2021 •

edited by bedevere-bot

pablogsal commented Sep 21, 2021 •

edited

pablogsal commented Oct 7, 2021 •

edited

pablogsal commented Oct 9, 2021 •

edited