classification
Title: Implement PyOS_CheckStack on macOS using pthread_get_stack*_np
Type: enhancement Stage: patch review
Components: Interpreter Core Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: ZackerySpytz, corona10, ronaldoussoren, vstinner
Priority: low Keywords: patch

Created on 2018-06-25 11:18 by ronaldoussoren, last changed 2019-08-29 00:13 by vstinner.

Pull Requests
URL Status Linked Edit
PR 8046 open corona10, 2018-07-02 09:54
Messages (14)
msg320415 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) 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) * (Python committer) Date: 2018-06-30 15:21
@ronaldoussoren

Hi, Ronald

Can I take a look this issue?
msg320842 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2018-07-01 18:57
Sure. I'm not working on this at the moment.
msg320868 - (view) Author: Dong-hee Na (corona10) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2018-07-05 05:39
@ronaldoussoren

I updated the PR

Please take a look.
msg321078 - (view) Author: Dong-hee Na (corona10) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python triager) Date: 2019-07-15 16:42
It seems that this issue is a duplicate of bpo-25518.
msg350497 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2019-08-26 07:15
@ronaldoussoren
Do you have any ideas to resume this issue again?
History
Date User Action Args
2019-08-31 18:18:01ned.deilylinkissue25518 superseder
2019-08-29 00:13:29vstinnersetnosy: + vstinner
2019-08-26 07:15:41corona10setmessages: + msg350497
2019-07-15 16:42:20ZackerySpytzsetnosy: + ZackerySpytz
messages: + msg347981
2018-07-14 04:49:12corona10setmessages: + msg321643
2018-07-10 07:30:35ronaldoussorensetmessages: + msg321360
2018-07-05 06:08:35corona10setmessages: + msg321078
2018-07-05 05:39:53corona10setmessages: + msg321076
2018-07-02 19:00:13corona10setmessages: + msg320907
2018-07-02 13:12:03ronaldoussorensetmessages: + msg320880
2018-07-02 13:04:24corona10setmessages: + msg320879
2018-07-02 12:08:40ronaldoussorensetmessages: + msg320874
2018-07-02 10:47:08corona10setmessages: + msg320868
2018-07-02 09:54:38corona10setkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request7655
2018-07-01 18:57:48ronaldoussorensetmessages: + msg320842
2018-06-30 15:21:32corona10setnosy: + corona10
messages: + msg320786
2018-06-25 11:18:15ronaldoussorencreate