Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
GH-91079: Decouple C stack overflow checks from Python recursion checks.#96510
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bedevere-bot commentedSep 5, 2022
🤖 New build scheduled with the buildbot fleet by@markshannon for commite8a1bed 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot commentedSep 5, 2022
🤖 New build scheduled with the buildbot fleet by@markshannon for commit7e21a74 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot commentedSep 6, 2022
🤖 New build scheduled with the buildbot fleet by@markshannon for commit65cca7d 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot commentedSep 13, 2022
🤖 New build scheduled with the buildbot fleet by@markshannon for commitd75797c 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
| #1,000 on most systems | ||
| limit=sys.getrecursionlimit() | ||
| code="lambda: "+"+".join(f"_number_{i}"foriinrange(limit)) | ||
| #Need more than 256 variables to use EXTENDED_ARGS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I assume EXTENDED_ARGS has stack implications? explaining the "why" of this here would be useful.
Lib/test/test_exceptions.py Outdated
| # and is equal to recursion_limit when _gen_throw() calls | ||
| # PyErr_NormalizeException(). | ||
| recurse(setrecursionlimit(depth + 2) - depth) | ||
| recurse(5000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There's a constant of5000 used in all sorts of tests for the same purpose of being "too high" for recursion with this PR. I suggest making this a namedtest.support constant and referring to it instead of mystery constants spread throughout the test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(and where other constants are derived from that to be higher or lower scale, use math from the main value?)
Include/cpython/pystate.h Outdated
| /* WASI has limited call stack. Python's recursion limit depends on code | ||
| layout, optimization, and WASI runtime. Wasmtime can handle about 700 | ||
| recursions, sometimes less. 500 is a more conservative limit. */ | ||
| #ifndefPy_DEFAULT_RECURSION_LIMIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ifndef C_RECURSION_LIMIT
Include/internal/pycore_ceval.h Outdated
| # definePy_DEFAULT_RECURSION_LIMIT 1000 | ||
| # endif | ||
| #endif | ||
| #definePy_DEFAULT_RECURSION_LIMIT 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
keep the ifndef around this, those exist to allow someone setting their own via CFLAGS.
63f55aa tod1abed5Compare
gpshead left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this tied in nicely with the 3.11 talk :)
tacaswell commentedOct 7, 2022
This broke compilation ofhttps://github.com/python-greenlet/greenlet/ Doing the renames as suggested by the compiler fixes it (although from reading the PR here, maybe Is this an unintended side effect of this change and or does greenlets need to adapt? |
gpshead commentedOct 7, 2022
Not being familiar with greenlet much I suspect it wants |
gpshead commentedOct 7, 2022
BTW@tacaswell major kudos to you for testing against CPython |
tacaswell commentedOct 7, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks, I'll get a PR to greenlet done in the next few days (I am also barely familiar with greenlet but it is a dependency of a dependency of something I care about). I have built a whole rube-goldberg machine to test CPython |
dimpase commentedNov 17, 2023
dimpase commentedNov 18, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
E.g. the very basic, really CS101, recursion speedup with cache # fib.pyimportsyssys.setrecursionlimit(2000)fromfunctoolsimportcache@cachedeffib(n):ifn<1:return0ifn==1:return1returnfib(n-1)+fib(n-2)print(fib(500)) got broken by this PR. Perhaps needless to say, on the latest main branch, --- a/Include/cpython/pystate.h+++ b/Include/cpython/pystate.h@@ -225,7 +225,7 @@ struct _ts { # define Py_C_RECURSION_LIMIT 500 #else // This value is duplicated in Lib/test/support/__init__.py-# define Py_C_RECURSION_LIMIT 1500+# define Py_C_RECURSION_LIMIT 3000 #endif makes it work (I didn't try to find the minimal needed increase for |
gpshead commentedNov 18, 2023
Please stop using this longmerged and closed PR as a forum, nobody liatens here. This is not an issue tracker. If you believe there is a bug, file a new issue. |
dimpase commentedNov 20, 2023
These are filed. I also don't understand why this was merged, while not conforming topython/steering-council#102 (they asked for |
On Python 3.12, this provokes a stack overflow in the scheduler. It isnot quite clear why that's the case; pure-Python recursion even withgenerators seems to respond well to setrecursionlimit():```pydef f(n): if n: yield from f(n-1) else: yield 5import syssys.setrecursionlimit(3500)print(list(f(3400)))```That said, there have been [behavior](python/cpython#96510)[changes](python/cpython#112215)in Py3.12 in this regard, but it is not clear what exactlyabout Loopy's behavior makes it fall into the 'bad' case.
On Python 3.12, this provokes a stack overflow in the scheduler. It isnot quite clear why that's the case; pure-Python recursion even withgenerators seems to respond well to setrecursionlimit():```pydef f(n): if n: yield from f(n-1) else: yield 5import syssys.setrecursionlimit(3500)print(list(f(3400)))```That said, there have been [behavior](python/cpython#96510)[changes](python/cpython#112215)in Py3.12 in this regard, but it is not clear what exactlyabout Loopy's behavior makes it fall into the 'bad' case.
On Python 3.12, this provokes a stack overflow in the scheduler. It isnot quite clear why that's the case; pure-Python recursion even withgenerators seems to respond well to setrecursionlimit():```pydef f(n): if n: yield from f(n-1) else: yield 5import syssys.setrecursionlimit(3500)print(list(f(3400)))```That said, there have been [behavior](python/cpython#96510)[changes](python/cpython#112215)in Py3.12 in this regard, but it is not clear what exactlyabout Loopy's behavior makes it fall into the 'bad' case.
Uh oh!
There was an error while loading.Please reload this page.