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

redundant checks in long_add and long_sub #71260

Closed
orenmn mannequin opened this issue May 21, 2016 · 14 comments
Closed

redundant checks in long_add and long_sub #71260

orenmn mannequin opened this issue May 21, 2016 · 14 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@orenmn
Copy link
Mannequin

orenmn mannequin commented May 21, 2016

BPO 27073
Nosy @mdickinson, @vstinner, @serhiy-storchaka, @1st1, @orenmn
Files
  • issue.diff: proposed patches diff file
  • CPythonTestOutput.txt: test output of CPython without my patches (tested on my PC)
  • patchedCPythonTestOutput.txt: test output of CPython with my patches (tested on my PC)
  • issue27073.diff: proporsed patches diff file update1
  • issue27073.diff: proporsed patches diff file update2
  • issue27073.diff: proporsed patches diff file update3
  • issue27073.diff: proporsed patches diff file update4
  • patchedCPythonTestOutput2.txt: test output of CPython with the updated patches (tested on my PC)
  • issue27073.diff: proposed patches diff file update5
  • patchedCPythonTestOutput.txt: test output of CPython with the updated patches (tested on my PC) 2
  • 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 2016-06-03.21:08:30.520>
    created_at = <Date 2016-05-21.07:41:40.833>
    labels = ['interpreter-core', 'performance']
    title = 'redundant checks in long_add and long_sub'
    updated_at = <Date 2016-06-03.21:11:56.525>
    user = 'https://github.com/orenmn'

    bugs.python.org fields:

    activity = <Date 2016-06-03.21:11:56.525>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-06-03.21:08:30.520>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2016-05-21.07:41:40.833>
    creator = 'Oren Milman'
    dependencies = []
    files = ['42920', '42921', '42922', '42926', '43044', '43045', '43144', '43145', '43163', '43164']
    hgrepos = []
    issue_num = 27073
    keywords = ['patch']
    message_count = 14.0
    messages = ['265989', '266001', '266002', '266004', '266560', '266562', '266563', '266612', '267093', '267098', '267125', '267152', '267164', '267165']
    nosy_count = 6.0
    nosy_names = ['mark.dickinson', 'vstinner', 'python-dev', 'serhiy.storchaka', 'yselivanov', 'Oren Milman']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue27073'
    versions = ['Python 3.6']

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented May 21, 2016

    ------------ the proposed changes ------------
    I believe the following checks are redundant:
    1. in Objects/longobject.c in long_add:
    In case both a and b are negative, their absolute values are added using x_add, with the result stored in z.
    If (z != NULL), it must be that x_add succeeded, and also it must be that (Py_SIZE(z) > 0), as it is guaranteed that the absolute values of a and b are both bigger than zero.
    Thus, the check (Py_SIZE(z) != 0) here is redundant.
    2. in Objects/longobject.c in long_sub:
    In case a is negative, the absolute values of a and b are subtracted or added using x_sub or x_add, with the result stored in z.
    Later on, if (z != NULL && Py_SIZE(z) != 0), then Py_SIZE(z) is negated. However, even though it might be that Py_SIZE(z) == 0, it doesn't really matter.
    doing 'Py_SIZE(z) = -(Py_SIZE(z));' in that case would do nothing.
    Thus, the check (Py_SIZE(z) != 0) here is redundant.

    The original versions of both of these checks were added in revision 443 (November 1991!). Back then, ob_size's was implemented using one's complement, and negating it was actually doing 'z->ob_size = ~z->ob_size;'. 
    Of course, in that case the check (z->ob_size != 0) was necessary, but then, in revision 590, ob_size was changed to use two's complement, and the check (z->ob_size != 0) was left untouched, and remained there to this day.
    

    ------------ diff ------------
    The patches diff is attached.

    ------------ tests ------------
    I built the patched CPython for x86, and played with it a little. Everything seemed to work as usual.

    In addition, I ran 'python -m test' (on my 64-bit Windows 10) before and after applying the patch, and got quite the same output.
    the outputs of both runs are attached.

    @orenmn orenmn mannequin added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label May 21, 2016
    @SilentGhost SilentGhost mannequin added the type-bug An unexpected behavior, bug, or error label May 21, 2016
    @mdickinson
    Copy link
    Member

    Your analysis and patch look good to me.

    @vstinner
    Copy link
    Member

    Sorry, I didn't check if the change is valid or not, but:

    issue.diff

    Please keep the check but as an assertion (Put it in the if block).

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented May 21, 2016

    Thanks for the reviews!

    I added an assert in long_add (in long_sub it might be that the result is 0).
    The updated diff file is attached.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented May 28, 2016

    After giving it some more thought (while working on another, somewhat related issue - http://bugs.python.org/issue27145), I realized that that assert in long_add could further verify that the int x_add returned is a multiple-digit int (as x_add had received two multiple-digit ints to begin with).

    The important thing about this updated assert is that it verifies that x_add didn't return a reference to an element in small_ints (as small ints must be single-digit ints), so negating it in-place is safe.

    I have updated the assert and added an appropriate comment. The updated diff file is attached.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented May 28, 2016

    And after quadruple checking myself, I found a foolish mistake - in that flow, x_add received at least one multiple-digit int (not necessarily two :().

    I fixed that mistake in the comment. The updated diff file is attached.

    @serhiy-storchaka
    Copy link
    Member

    I don't think this assert is needed. Nothing bad happens if the asserted condition is false. On other side, additional assert can slow down debug build (that is already slower than release build).

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented May 29, 2016

    I agree. This assert only indirectly verifies that something bad doesn't happen.

    The bad thing that might happen is an in-place negating of an element of small_ints, so the most direct assert should be 'assert(Py_REFCNT(z) == 1);'.
    This is exactly what Victor did in long_lshift back in revision 84698...

    What do you think?

    @serhiy-storchaka
    Copy link
    Member

    It would be nice.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Jun 3, 2016

    All right. The updated diff file is attached.

    I compiled and ran the tests again. They are quite the same. The test output is attached.

    @serhiy-storchaka
    Copy link
    Member

    Maybe add an assert for the second size negation?

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Jun 3, 2016

    I considered doing that, but I had already opened another issue (http://bugs.python.org/issue27145) in which I had proposed to replace that in-place negate in long_sub with a call to _PyLong_Negate.
    But I guess I shouldn't worry about my patches colliding.

    Anyway, the second assert would be 'assert(Py_SIZE(z) == 0 || Py_REFCNT(z) == 1);', because if someone does (in Python) 'x = (-2 ** PyLong_SHIFT) - (-2 ** PyLong_SHIFT)', x_sub would do 'return (PyLongObject *)PyLong_FromLong(0);'.

    The updated diff file and the new test output are attached.

    (No idea why test_netrc failed there. I ran it specifically five times right after that, and it passed all of them. Maybe some race condition? (To run the tests, I do 'python_d.exe -m test -j3'.)
    Anyway, here is the relevant output (I am not sure the last line is relevant):

    Warning -- files was modified by test_netrc
    test test_netrc failed -- Traceback (most recent call last):
      File "C:\Users\orenmn\cpython\lib\test\test_netrc.py", line 52, in test_password_with_trailing_hash
        """, 'pass#')
      File "C:\Users\orenmn\cpython\lib\test\test_netrc.py", line 41, in _test_passwords
        nrc = self.make_nrc(nrc)
      File "C:\Users\orenmn\cpython\lib\test\test_netrc.py", line 13, in make_nrc
        with open(temp_filename, mode) as fp:
    PermissionError: [Errno 13] Permission denied: '@test_3652_tmp'
    minkernel\crts\ucrt\src\appcrt\lowio\write.cpp(49) : Assertion failed: (_osfile(fh) & FOPEN)

    )

    @serhiy-storchaka
    Copy link
    Member

    LGTM (except a trailing space in a comment). Thank you for your contribution Oren.

    @serhiy-storchaka serhiy-storchaka added performance Performance or resource usage and removed type-bug An unexpected behavior, bug, or error labels Jun 3, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 3, 2016

    New changeset c21bf38a9d07 by Serhiy Storchaka in branch 'default':
    Issue bpo-27073: Removed redundant checks in long_add and long_sub.
    https://hg.python.org/cpython/rev/c21bf38a9d07

    @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

    3 participants