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

PEP 3121, 384 Refactoring applied to tkinter module #59926

Closed
RobinSchreiber mannequin opened this issue Aug 18, 2012 · 24 comments
Closed

PEP 3121, 384 Refactoring applied to tkinter module #59926

RobinSchreiber mannequin opened this issue Aug 18, 2012 · 24 comments
Assignees
Labels
extension-modules C modules in the Modules dir performance Performance or resource usage

Comments

@RobinSchreiber
Copy link
Mannequin

RobinSchreiber mannequin commented Aug 18, 2012

BPO 15721
Nosy @loewis, @amauryfa, @pitrou, @ned-deily, @asvetlov, @serhiy-storchaka
Files
  • _tkinter_pep3121-384_v0.patch
  • _tkinter_pep3121-384_v1.patch
  • _tkinter_pep3121-384_v2.patch
  • _tkinter_pep3121-384_v3.patch
  • _tkinter_pep3121-384_v4.patch
  • _tkinter_pep3121-384_v7.patch
  • _tkinter_pep384_v1.patch
  • 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 = 'https://github.com/asvetlov'
    closed_at = <Date 2016-05-08.18:31:21.213>
    created_at = <Date 2012-08-18.10:53:44.006>
    labels = ['extension-modules', 'performance']
    title = 'PEP 3121, 384 Refactoring applied to tkinter module'
    updated_at = <Date 2016-05-08.18:31:21.212>
    user = 'https://bugs.python.org/RobinSchreiber'

    bugs.python.org fields:

    activity = <Date 2016-05-08.18:31:21.212>
    actor = 'serhiy.storchaka'
    assignee = 'asvetlov'
    closed = True
    closed_date = <Date 2016-05-08.18:31:21.213>
    closer = 'serhiy.storchaka'
    components = ['Extension Modules']
    creation = <Date 2012-08-18.10:53:44.006>
    creator = 'Robin.Schreiber'
    dependencies = []
    files = ['26887', '27457', '27458', '27460', '27464', '27470', '27475']
    hgrepos = []
    issue_num = 15721
    keywords = ['pep3121']
    message_count = 24.0
    messages = ['168504', '168505', '168522', '172210', '172213', '172222', '172230', '172245', '172248', '172257', '172288', '172306', '172883', '172969', '172985', '174226', '174227', '187261', '187380', '194842', '194843', '231696', '254680', '265154']
    nosy_count = 8.0
    nosy_names = ['loewis', 'amaury.forgeotdarc', 'pitrou', 'ned.deily', 'asvetlov', 'python-dev', 'serhiy.storchaka', 'Robin.Schreiber']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue15721'
    versions = ['Python 3.4']

    @RobinSchreiber
    Copy link
    Mannequin Author

    RobinSchreiber mannequin commented Aug 18, 2012

    Changes proposed in PEP-3121 and PEP-384 have now been applied to the tkinter module!
    When running the test form inside Python.exe (tkinter._test()), the litte "test-window" is rendered correctly. However there are still some error messages popping up inside the python shell
    (I am running this on OS X 10.8):

    >>> tkinter._test()
    2012-08-18 12:46:24.775 python.exe[17410:707] speedup (null)
    2012-08-18 12:46:24.776 python.exe[17410:707] ERROR: Unable to find method [_NSFullScreenTransition _startFullScreenTransitionForCGWindow:targetFrame:duration:].
    2012-08-18 12:46:24.776 python.exe[17410:707] ERROR: Unable to find method [_NSFullScreenTransition _startEnterFullScreenTransitionForCGWindow:targetFrame:duration:].
    2012-08-18 12:46:24.778 python.exe[17410:707] ERROR: Unable to find method [_NSFullScreenTransition startExitFullScreenTransitionForCGWindow:targetFrame:duration:].
    [87221 refs]
    >>> 

    I have no Idea how and where these are triggered.

    @RobinSchreiber RobinSchreiber mannequin added extension-modules C modules in the Modules dir performance Performance or resource usage labels Aug 18, 2012
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Aug 18, 2012

    See

    http://www.python.org/download/mac/tcltk/

    It may be that using a different build of Tcl/Tk solves that problem.

    @ned-deily
    Copy link
    Member

    What version of 10.8 are you seeing those messages with? And do you see them without the patch applied?

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 6, 2012

    Update patch conforming to current _tkinter code.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 6, 2012

    I would to have all module state inside _tkinterstate structure.
    static variables like tcl_lock, tcl_state, quitMainLoop, errorInCmd etc should be moved into _tkinterstate also.

    @RobinSchreiber
    Copy link
    Mannequin Author

    RobinSchreiber mannequin commented Oct 6, 2012

    Before I submitted this patch, I used to have these variables inside the modulestate, which caused severe problems. I do not know the exact reason, but my guess is that these variables have to be globally available for every thread (tcl variables are used for thread synchronization arent they?). As the modulestate may change depending on the thread, one can no longer guarantee that every thread within the process is operating on the same variable. This might not be nessecary, however as I mentioned, the naive approach of putting the variables inside the modulestate did not work out for me.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 6, 2012

    I'm trying to make patch following myself recommendations.
    Looks good but not finished yet. Will publish it after all work done.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 6, 2012

    Submit patch which store own state into module state.
    Still not finished yet: need to add checks for "ready state" in every access to fields of module state structure.

    @asvetlov asvetlov self-assigned this Oct 6, 2012
    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 6, 2012

    Update patch to support TKINTER_PROTECT_LOADTK option.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 6, 2012

    Upload version which check if _tkinter module was destroyed (for example, if it was call from daemon thread runing Tk app when main thread is exiting with interpreter finalization).

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 7, 2012

    The patch for current default branch.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 7, 2012

    The patch is huge, then I like to apply it in two steps: first implement PEP-384 than 3121.
    Attached first patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 14, 2012

    New changeset bf9d118779f5 by Andrew Svetlov in branch 'default':
    Issue bpo-15721: make _tkinter module PEP-384 compatible.
    http://hg.python.org/cpython/rev/bf9d118779f5

    @amauryfa
    Copy link
    Member

    Andrew,

    I have questions about the following part of commit bf9d118779f5:
    + PyTclObject_Type_slots[3].pfunc = PyObject_GenericGetAttr;

    First, the "3" refers to the position of Py_tp_getattro in the array, which is a fragile thing IMO.

    Then, this hack was not necessary before; why is it needed now?
    IIRC on Windows a static initializer cannot contain dll-imported variables, but function pointers are OK.
    Or do you have evidence of the contrary?

    @asvetlov
    Copy link
    Contributor

    Amaury, I completely agree with your objection.
    I've found this code in xxlimited.c and adapted to _tkinter source.
    If that weird code can be removed I will do it.

    I have no idea how we can catch/reproduce the problem, maybe Martin von Loewis can help as author of xxlimited.c code?

    As I know almost nobody make PEP-384 compliant modules for now and maybe there are couple of dark corners at least in the docs.

    See also bpo-15650 for another question related to adaptation to PEP-3121.

    @asvetlov
    Copy link
    Contributor

    Amaury, you are right:
    PyTclObject_Type_slots[3].pfunc = PyObject_GenericGetAttr;
    is not required.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 30, 2012

    New changeset 4eb6e07e8171 by Andrew Svetlov in branch 'default':
    Issue bpo-15721: apply PEP-384 Refactoring to tkinter module.
    http://hg.python.org/cpython/rev/4eb6e07e8171

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Apr 18, 2013

    Is any more work needed on this issue or can it be closed?

    @asvetlov
    Copy link
    Contributor

    No, PEP-3121 patch is not ready yet but PEP-384 has been applied.

    @pitrou
    Copy link
    Member

    pitrou commented Aug 10, 2013

    The first two patches (_tkinter_pep3121-384_v0.patch and _tkinter_pep3121-384_v1.patch) seemed to have the refcounts right, but Andrew's further patches lost the required Py_INCREF in the various new() functions (and the corresponding Py_DECREF in deallocs), which is the cause for the crash in bpo-15667.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 10, 2013

    New changeset 4d0c938870bc by Antoine Pitrou in branch 'default':
    Fix refcounting issue with extension types in tkinter.
    http://hg.python.org/cpython/rev/4d0c938870bc

    @serhiy-storchaka
    Copy link
    Member

    _tkinter classes are now heap types and they no longer have the __module__ attribute.

    >>> import _tkinter
    >>> _tkinter.TkappType.__module__
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    AttributeError: __module__

    See bpo-20204.

    @serhiy-storchaka
    Copy link
    Member

    This change causes a crash when create instances of _tkinter classes (bpo-23815). If this will not be fixed I suggest to revert the patch.

    @serhiy-storchaka
    Copy link
    Member

    Fixed in bpo-23815. Opened bpo-26979 for documenting the catch.

    @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
    extension-modules C modules in the Modules dir performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants