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-3118: remove obsolete write-locks #58411

Closed
skrah mannequin opened this issue Mar 5, 2012 · 15 comments
Closed

PEP-3118: remove obsolete write-locks #58411

skrah mannequin opened this issue Mar 5, 2012 · 15 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@skrah
Copy link
Mannequin

skrah mannequin commented Mar 5, 2012

BPO 14203
Nosy @ncoghlan, @pitrou, @skrah, @serhiy-storchaka
Files
  • bytearray_getbuffer.diff
  • issue14203-2.diff
  • issue14203-3.diff
  • 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 2015-02-04.13:51:46.544>
    created_at = <Date 2012-03-05.18:32:48.582>
    labels = ['interpreter-core', 'performance']
    title = 'PEP-3118: remove obsolete write-locks'
    updated_at = <Date 2015-02-04.13:51:46.543>
    user = 'https://github.com/skrah'

    bugs.python.org fields:

    activity = <Date 2015-02-04.13:51:46.543>
    actor = 'skrah'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-02-04.13:51:46.544>
    closer = 'skrah'
    components = ['Interpreter Core']
    creation = <Date 2012-03-05.18:32:48.582>
    creator = 'skrah'
    dependencies = []
    files = ['24738', '38003', '38004']
    hgrepos = []
    issue_num = 14203
    keywords = ['patch', 'needs review']
    message_count = 15.0
    messages = ['154969', '154971', '154972', '219995', '235346', '235348', '235352', '235354', '235355', '235358', '235359', '235360', '235361', '235363', '235386']
    nosy_count = 6.0
    nosy_names = ['ncoghlan', 'pitrou', 'skrah', 'python-dev', 'serhiy.storchaka', 'kelleynnn']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue14203'
    versions = ['Python 3.5']

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Mar 5, 2012

    bytearray_getbuffer() checks for view==NULL. But this has never been
    allowed:

    PEP: "The second argument is the address to a bufferinfo structure.
    Both arguments must never be NULL."

    DOCS (3.2): "view must point to an existing Py_buffer structure
    allocated by the caller".

    A quick grep through the source tree shows no instances where
    the middle argument of either PyObject_GetBuffer of bf_getbuffer
    is NULL or 0.

    Patch attached, all tests pass. I wouldn't be comfortable to
    commit it without review though (it's just too strange).

    BTW, the next conditional in bytearray_getbuffer ...

        if (ret >= 0) {
            obj->ob_exports++;
        }

    is also superfluous, since PyBuffer_FillInfo() cannot fail
    if readonly==0.

    @skrah skrah mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Mar 5, 2012
    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Mar 5, 2012

    array_buffer_getbuf does a similar thing:

    if (view==NULL) goto finish;

    finish:
    self->ob_exports++; // ???
    return 0

    Where does this view==NULL thing come from?

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Mar 5, 2012

    I seems to be a feature to get a "lock" on an exporter
    without the exporter filling in a buffer. It was there
    from the beginning:

    http://svn.python.org/view/python/branches/release30-maint/Objects/bytearrayobject.c?r1=56849&r2=57181

    Last use that I can see is here:

    http://hg.python.org/cpython/file/df3b2b5db900/Modules/posixmodule.c#l561

    @kelleynnn
    Copy link
    Mannequin

    kelleynnn mannequin commented Jun 7, 2014

    I have verified that this feature is unused in the source tree; in fact, there are no internal calls to bytearray_getbuffer() at all. The only thing bytearray_getbuffer() does with its second arg is pass it to PyBuffer_FillInfo(), which immediately checks it and passes 0 up the call stack if it is NULL. (A comment in PyBuffer_FillInfo() asks why -1 is not passed up instead; it's probably to distinguish this feature from the error condition handled in the immediately following conditional block.)

    There are potentially other issues stemming from this legacy feature in bytearray_getbuffer(), PyBuffer_FillInfo(), and elsewhere. The maintainers may see fit to open tickets on these issues as well.

    There's more relevant commentary on the feature here: http://comments.gmane.org/gmane.comp.python.devel/130521

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 3, 2015

    New patch with tests.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 3, 2015

    New changeset e8fe32d43c96 by Stefan Krah in branch 'default':
    Issue bpo-14203: Remove obsolete support for view==NULL in PyBuffer_FillInfo()
    https://hg.python.org/cpython/rev/e8fe32d43c96

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 3, 2015

    bytesiobuf_getbuffer() also still has this obsolete feature, so
    BufferError should be raised if view==NULL.

    I'm unsure if this plays well with the new SHARED_BUF(b) thing.

    Should the exception be raised before or after?

    @skrah skrah mannequin changed the title bytearray_getbuffer: unnecessary code PEP-3118: remove obsolete write-locks Feb 3, 2015
    @serhiy-storchaka
    Copy link
    Member

    I think before. It doesn't harm, but it doesn't make much sense to unshare the buffer if its address is not used.

    @serhiy-storchaka
    Copy link
    Member

    E.g. the buffer should be unshared before incrementing b->exports, but if an exception is raised instead, there is no need to unshare the buffer before raising.

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 3, 2015

    Ok, thanks! bpo-14203-3.diff should be correct, then.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 3, 2015

    New changeset 0d5095a2422f by Stefan Krah in branch 'default':
    Issue bpo-14203: Remove obsolete support for view==NULL in bytesiobuf_getbuffer()
    https://hg.python.org/cpython/rev/0d5095a2422f

    @serhiy-storchaka
    Copy link
    Member

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 3, 2015

    Argh, the extern _PyBytesIOBuffer_Type hack. I knew it would cause some
    trouble.

    Any ideas how to test bytesiobuf_getbuffer() with a NULL view in
    a more dignified way?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 3, 2015

    New changeset 17a8c5f8ca48 by Stefan Krah in branch 'default':
    Issue bpo-14203: Temporary fix for the compile failure on Windows.
    https://hg.python.org/cpython/rev/17a8c5f8ca48

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 4, 2015

    I think it's sufficient to test bytesiobuf_getbuffer() on
    Linux and FreeBSD. The test just checks that the exception
    is raised.

    @skrah skrah mannequin closed this as completed Feb 4, 2015
    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant