Issue33955
Created on 2018-06-25 11:18 by ronaldoussoren, last changed 2022-04-11 14:59 by admin.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 8046 | closed | corona10, 2018-07-02 09:54 | |
| Messages (15) | |||
|---|---|---|---|
| msg320415 - (view) | Author: Ronald Oussoren (ronaldoussoren) * ![]() |
Date: 2018-06-25 11:18 | |
The (non-portable) pthread APIs "pthread_get_stacksize_np()" and "pthread_get_stackaddr_np()" can be used to implement PyOS_CheckStack on macOS, which would give nicer behavior than crashing when the recursion limit is too high for the stack size of the current thread. Creating a patch should be fairly easy, but does require C knowledge. |
|||
| msg320786 - (view) | Author: Dong-hee Na (corona10) * ![]() |
Date: 2018-06-30 15:21 | |
@ronaldoussoren Hi, Ronald Can I take a look this issue? |
|||
| msg320842 - (view) | Author: Ronald Oussoren (ronaldoussoren) * ![]() |
Date: 2018-07-01 18:57 | |
Sure. I'm not working on this at the moment. |
|||
| msg320868 - (view) | Author: Dong-hee Na (corona10) * ![]() |
Date: 2018-07-02 10:47 | |
@ronaldoussoren I wrote a first proto type PR and it works well on my MacBook Pro. As you commented, there is a issue with pthread_get_stacksize_np when pthread_main_np() == 1 on some old mac system. I will take a look more deeply at this issue. If you have any feedback for my first PR please left it. Thanks, Dong-hee |
|||
| msg320874 - (view) | Author: Ronald Oussoren (ronaldoussoren) * ![]() |
Date: 2018-07-02 12:08 | |
Have you looked at benchmark results for this patch? In particular, PyOS_CheckStack is called quite often and I wonder how those calls affect performance. If there is a clear performance impact it would be useful to store some information in the python thread state. In what versions of macOS are there problems with pthread_get_stacksize_np() on the main thread? |
|||
| msg320879 - (view) | Author: Dong-hee Na (corona10) * ![]() |
Date: 2018-07-02 13:04 | |
@ronaldoussoren 1. I will try to run a benchmark is there any good benchmark to try it? 2. As far as I know, OS X 10.9 Mavericks has an issue with pthread_get_stacksize_np, I added a new commit on PR with this issue. Thanks! |
|||
| msg320880 - (view) | Author: Ronald Oussoren (ronaldoussoren) * ![]() |
Date: 2018-07-02 13:12 | |
W.r.t. benchmarks: The full benchmark suite that's often used to assess the performance impact on real world code is: http://pyperformance.readthedocs.io Running this takes some time, and the interesting bit is comparing the performance with and without your patch. You could also test with a small script that uses the timeit module to run some code that performs a lot of function calls. |
|||
| msg320907 - (view) | Author: Dong-hee Na (corona10) * ![]() |
Date: 2018-07-02 19:00 | |
@ronaldoussoren I updated the PR and we need some discussion. I benchmarked new patch with Fibonacci codes. baseline(master): 21.330535484 without call sysctlbyname(PR 8046): 22.857963209 secs with call sysctlbyname(PR 8046): 37.125129114 secs So my choice is just believe pthread_get_stacksize_np() like rust do. (https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/thread.rs#L246) code: def fib(n): if n == 0: return 0 if n == 1: return 1 return fib(n-2) + fib(n-1) if __name__ == '__main__': import timeit print(timeit.timeit("fib(10)", setup="from __main__ import fib")) |
|||
| msg321076 - (view) | Author: Dong-hee Na (corona10) * ![]() |
Date: 2018-07-05 05:39 | |
@ronaldoussoren I updated the PR Please take a look. |
|||
| msg321078 - (view) | Author: Dong-hee Na (corona10) * ![]() |
Date: 2018-07-05 06:08 | |
benchmark result: 22.639114022999998 secs master (+0%) 22.806662271 secs PR 8046 (-0.74%) def fib(n): if n == 0: return 0 if n == 1: return 1 return fib(n-2) + fib(n-1) if __name__ == '__main__': import timeit print(timeit.timeit("fib(10)", setup="from __main__ import fib")) |
|||
| msg321360 - (view) | Author: Ronald Oussoren (ronaldoussoren) * ![]() |
Date: 2018-07-10 07:30 | |
Sorry about the slow response. The patch looks OK, although I'm entirely happy yet about the workaround for the stack size on the main thread. Primarily because framework builds use a non-standard stack size as the default one is too small for the recursion limit (IIRC especially with debug builds). BTW. My own use case for this feature is primarily the use of python on threads created by the system (such as threads created by Cocoa APIs and libdispatch), for threads created in Python we can (and do) create them with a large enough stack (although this guard is helpful there too). My next step will be to test the patch locally. This might take a while because I will be traveling the rest of this month. Worst case is that I'll report back during EuroPython 2018 (starting at 23th of July) |
|||
| msg321643 - (view) | Author: Dong-hee Na (corona10) * ![]() |
Date: 2018-07-14 04:49 | |
FYI, I've updated the patch to use rlim_cur() on the main thread when pthread_get_stacksize_np() is not accurate. This way works pretty well on my local machine. Enjoy your travel! |
|||
| msg347981 - (view) | Author: Zackery Spytz (ZackerySpytz) * ![]() |
Date: 2019-07-15 16:42 | |
It seems that this issue is a duplicate of bpo-25518. |
|||
| msg350497 - (view) | Author: Dong-hee Na (corona10) * ![]() |
Date: 2019-08-26 07:15 | |
@ronaldoussoren Do you have any ideas to resume this issue again? |
|||
| msg388319 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2021-03-08 23:56 | |
See also https://www.python.org/dev/peps/pep-0651/ |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:59:02 | admin | set | github: 78136 |
| 2021-03-08 23:56:35 | vstinner | set | messages: + msg388319 |
| 2019-08-31 18:18:01 | ned.deily | link | issue25518 superseder |
| 2019-08-29 00:13:29 | vstinner | set | nosy:
+ vstinner |
| 2019-08-26 07:15:41 | corona10 | set | messages: + msg350497 |
| 2019-07-15 16:42:20 | ZackerySpytz | set | nosy:
+ ZackerySpytz messages: + msg347981 |
| 2018-07-14 04:49:12 | corona10 | set | messages: + msg321643 |
| 2018-07-10 07:30:35 | ronaldoussoren | set | messages: + msg321360 |
| 2018-07-05 06:08:35 | corona10 | set | messages: + msg321078 |
| 2018-07-05 05:39:53 | corona10 | set | messages: + msg321076 |
| 2018-07-02 19:00:13 | corona10 | set | messages: + msg320907 |
| 2018-07-02 13:12:03 | ronaldoussoren | set | messages: + msg320880 |
| 2018-07-02 13:04:24 | corona10 | set | messages: + msg320879 |
| 2018-07-02 12:08:40 | ronaldoussoren | set | messages: + msg320874 |
| 2018-07-02 10:47:08 | corona10 | set | messages: + msg320868 |
| 2018-07-02 09:54:38 | corona10 | set | keywords:
+ patch stage: needs patch -> patch review pull_requests: + pull_request7655 |
| 2018-07-01 18:57:48 | ronaldoussoren | set | messages: + msg320842 |
| 2018-06-30 15:21:32 | corona10 | set | nosy:
+ corona10 messages: + msg320786 |
| 2018-06-25 11:18:15 | ronaldoussoren | create | |

