Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement PyOS_CheckStack on macOS using pthread_get_stack*_np #78136

Open
ronaldoussoren opened this issue Jun 25, 2018 · 15 comments
Open

Implement PyOS_CheckStack on macOS using pthread_get_stack*_np #78136

ronaldoussoren opened this issue Jun 25, 2018 · 15 comments
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@ronaldoussoren
Copy link
Contributor

BPO 33955
Nosy @ronaldoussoren, @vstinner, @corona10, @ZackerySpytz
PRs
  • bpo-33955: Support USE_STACKCHECK on macOS #8046
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2018-06-25.11:18:15.677>
    labels = ['interpreter-core', 'type-feature', '3.8']
    title = 'Implement PyOS_CheckStack on macOS using pthread_get_stack*_np'
    updated_at = <Date 2021-03-08.23:56:35.506>
    user = 'https://github.com/ronaldoussoren'

    bugs.python.org fields:

    activity = <Date 2021-03-08.23:56:35.506>
    actor = 'vstinner'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2018-06-25.11:18:15.677>
    creator = 'ronaldoussoren'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33955
    keywords = ['patch']
    message_count = 15.0
    messages = ['320415', '320786', '320842', '320868', '320874', '320879', '320880', '320907', '321076', '321078', '321360', '321643', '347981', '350497', '388319']
    nosy_count = 4.0
    nosy_names = ['ronaldoussoren', 'vstinner', 'corona10', 'ZackerySpytz']
    pr_nums = ['8046']
    priority = 'low'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue33955'
    versions = ['Python 3.8']

    @ronaldoussoren
    Copy link
    Contributor Author

    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.

    @ronaldoussoren ronaldoussoren added 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Jun 25, 2018
    @corona10
    Copy link
    Member

    @ronaldoussoren

    Hi, Ronald

    Can I take a look this issue?

    @ronaldoussoren
    Copy link
    Contributor Author

    Sure. I'm not working on this at the moment.

    @corona10
    Copy link
    Member

    corona10 commented Jul 2, 2018

    @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

    @ronaldoussoren
    Copy link
    Contributor Author

    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?

    @corona10
    Copy link
    Member

    corona10 commented Jul 2, 2018

    @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!

    @ronaldoussoren
    Copy link
    Contributor Author

    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.

    @corona10
    Copy link
    Member

    corona10 commented Jul 2, 2018

    @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"))

    @corona10
    Copy link
    Member

    corona10 commented Jul 5, 2018

    @ronaldoussoren

    I updated the PR

    Please take a look.

    @corona10
    Copy link
    Member

    corona10 commented Jul 5, 2018

    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"))

    @ronaldoussoren
    Copy link
    Contributor Author

    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)

    @corona10
    Copy link
    Member

    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!

    @ZackerySpytz
    Copy link
    Mannequin

    ZackerySpytz mannequin commented Jul 15, 2019

    It seems that this issue is a duplicate of bpo-25518.

    @corona10
    Copy link
    Member

    @ronaldoussoren
    Do you have any ideas to resume this issue again?

    @vstinner
    Copy link
    Member

    vstinner commented Mar 8, 2021

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants