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

PYTHONDUMPREFS segfaults on exit #74342

Closed
orent mannequin opened this issue Apr 24, 2017 · 15 comments
Closed

PYTHONDUMPREFS segfaults on exit #74342

orent mannequin opened this issue Apr 24, 2017 · 15 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@orent
Copy link
Mannequin

orent mannequin commented Apr 24, 2017

BPO 30156
Nosy @rhettinger, @vstinner, @orent, @serhiy-storchaka
PRs
  • bpo-30156: Fix a crash in debug build when PYTHONDUMPREFS is set. #1272
  • bpo-30156: Remove property_descr_get() hack #3985
  • bpo-30156: Remove property_descr_get() optimization #9541
  • bpo-32492: 1.6x speed up in namedtuple attribute access using C fast-path #10495
  • 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 = <Date 2018-10-01.10:09:45.161>
    created_at = <Date 2017-04-24.17:43:47.576>
    labels = ['interpreter-core', '3.7', '3.8', 'type-crash']
    title = 'PYTHONDUMPREFS segfaults on exit'
    updated_at = <Date 2018-11-13.10:42:51.577>
    user = 'https://github.com/orent'

    bugs.python.org fields:

    activity = <Date 2018-11-13.10:42:51.577>
    actor = 'pablogsal'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-10-01.10:09:45.161>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2017-04-24.17:43:47.576>
    creator = 'orent'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30156
    keywords = ['patch']
    message_count = 15.0
    messages = ['292232', '292234', '292235', '292239', '292252', '292253', '304342', '304343', '304566', '304711', '326257', '326258', '326436', '326779', '326782']
    nosy_count = 4.0
    nosy_names = ['rhettinger', 'vstinner', 'orent', 'serhiy.storchaka']
    pr_nums = ['1272', '3985', '9541', '10495']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue30156'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @orent
    Copy link
    Mannequin Author

    orent mannequin commented Apr 24, 2017

    Reproduce:

    Py_DEBUG build
    PYTHONDUMPREFS=1 ./python -c pass
    (big dump of reference information)
    Segmentation fault

    git-bisected to commit 7822f15

    @orent orent mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Apr 24, 2017
    @orent
    Copy link
    Mannequin Author

    orent mannequin commented Apr 24, 2017

    In addition to fixing this - perhaps PYTHONDUMPREFS or something similar should be added to test automation? It is apparently capable of uncovering some bugs that none of the other reference and recnt debugging tools could find.

    @serhiy-storchaka
    Copy link
    Member

    The regression was added by the fix for bpo-26811. PR 1272 applies the alternate patch from bpo-26811. This doesn't harm the performance.

    $ ./python.patched -m perf timeit -q --compare-to ./python.default -s "from collections import namedtuple; P = namedtuple('P', 'x y'); p = P(1, 2)" --duplicate 1000 "p.x"
    Mean +- std dev: [python.default] 128 ns +- 7 ns -> [python.patched] 121 ns +- 7 ns: 1.05x faster (-5%)

    I thought about tests, but I don't know what is the best place for them. Seems other environment variables that controls debug output are not tested too.

    @serhiy-storchaka serhiy-storchaka added the 3.7 (EOL) end of life label Apr 24, 2017
    @vstinner
    Copy link
    Member

    Come on, yet another crash from property_descr_get()??? It's the 3rd time... Do we really need this micro-optimization?

    Previous bugs and workarounds:

    Using the FASTCALL calling convention, no temporary tuple is created to pass arguments if you use the _PyObject_FastCall() API and if the called function supports this calling convention.

    Sadly, namedtuple.attr uses operator.itergetter, itemgetter_call() doesn't support the FASTCALL, and my tp_fastcall patch was rejected: issue bpo-29259.

    @serhiy-storchaka
    Copy link
    Member

    Removing this micro-optimization makes attribute access in namedtuple more than 1.5 times slower:

    Mean +- std dev: [python.default] 126 ns +- 4 ns -> [python] 200 ns +- 7 ns: 1.58x slower (+58%)

    This would be a significant regression.

    @serhiy-storchaka
    Copy link
    Member

    The previous result was got when use _PyObject_FastCallDict(). Using PyObject_Call() is slightly faster:

    Mean +- std dev: [python.default] 127 ns +- 4 ns -> [python] 185 ns +- 9 ns: 1.45x slower (+45%)

    @vstinner
    Copy link
    Member

    Removing this micro-optimization makes attribute access in namedtuple more than 1.5 times slower:
    Mean +- std dev: [python.default] 126 ns +- 4 ns -> [python] 200 ns +- 7 ns: 1.58x slower (+58%)

    I wrote the PR 3985, it's only 20 ns slower (1.3x slower):

    [ref] 80.4 ns +- 3.3 ns -> [fastcall] 103 ns +- 5 ns: 1.28x slower (+28%)

    Maybe Python was optimized further in the meanwhile, or the slowdown is higher on your computer?

    @vstinner
    Copy link
    Member

    Note: if you care of namedtuple performance, Raymond Hettinger wrote that he would be interested to reuse the C structseq sequence. I measured that getting an attribute by name is faster in structseq than with the current property cached tuple hack.

    @serhiy-storchaka
    Copy link
    Member

    Maybe the difference is processor depending. I'm going to repeat benchmarking on 32-bit processors. Or maybe fast call was optimized in master. Or passing via an arguments tuple was pessimized.

    @serhiy-storchaka
    Copy link
    Member

    Result on the same computer:

    Mean +- std dev: [python0] 146 ns +- 5 ns -> [python] 206 ns +- 2 ns: 1.41x slower (+41%)

    The difference now is smaller (41% instead of 58%) because calling with a tuple now is 16% slower.

    @vstinner
    Copy link
    Member

    PR 9541: my new attempt to remove the micro-optimization. Commit message:

    bpo-30156: Remove property_descr_get() optimization

    property_descr_get() uses a "cached" tuple to optimize function
    calls. But this tuple can be discovered in debug mode with
    sys.getobjects(). Remove the optimization, it's not really worth it
    and it causes 3 different crashes last years.

    Microbenchmark:

    ./python -m perf timeit -v \
    -s "from collections import namedtuple; P = namedtuple('P', 'x y'); p = P(1, 2)" \
    --duplicate 1024 "p.x"

    Result:

    Mean +- std dev: [ref] 32.8 ns +- 0.8 ns -> [patch] 40.4 ns +- 1.3 ns: 1.23x slower (+23%)

    @vstinner
    Copy link
    Member

    Is this optiization really worth it? 7 nanoseconds faster, but leak a weird tuple which caused 3 different crashes...

    @vstinner
    Copy link
    Member

    Note: I reported a similar issue which has then been marked as a duplicate of this one: bpo-34223. Copy of my first message:
    ---
    It seems like Python has an invalid object somewhere. PYTHONDUMPREFS=1 makes Python crash at exit.

    $ PYTHONDUMPREFS=1 ./python -c pass
    (...)
    0x7f1992292448 [1] (<class '_thread._localdummy'>, <class 'object'>)
    0x7f1992241aa8 [1] {'__doc__': 'Thread-local dummy'}
    0x7f199222c740 [1] (<class 'object'>,)
    0x9c98a0 [2] <class '_thread._localdummy'>
    0x7f1992217dc0 [1] 
    Segmentation fault (core dumped)

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2018

    New changeset e972c13 by Victor Stinner in branch 'master':
    bpo-30156: Remove property_descr_get() optimization (GH-9541)
    e972c13

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2018

    I proposeto leave Python 3.6 and 3.7 unchanged. I don't think that this specific bug matters enough to remove to remove an optimization from these stable branches. Since the optimization has been added (2015, bpo-23910), I'm only aware of two bug reports on PYTHONDUMPREFS (this one, and my duplicate).

    If someone considers that the optimization must die in 3.6 and 3.7, please reopen the issue (or just add a comment).

    @vstinner vstinner added the 3.8 only security fixes label Oct 1, 2018
    @vstinner vstinner closed this as completed Oct 1, 2018
    @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.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants