redundant checks and a weird use of goto statements in long_rshift #71409

Activity

1. orenmn commented on Jun 4, 2016

   

   orenmnmannequin

   on Jun 4, 2016

   MannequinAuthor

   More actions

   ------------ current state ------------

   1. long_rshift first checks whether a is a negative int. If it is, it does (edited for brevity) 'z = long_invert(long_rshift(long_invert(a), b));'.
      Otherwise, it calculates the result of the shift and stores it in z. In this block, there is a check whether a is a negative int.

   The second check was always there - since revision 443, in which long_rshift was first added. However, in revision 590, the first aforementioned check (whether a is a negative int), along with a (edited for brevity) 'return long_invert(long_rshift(long_invert(a), b));' were added, but the second check whether a is a negative int wasn't removed, and remained there to this day.

   2. Ultimately, long_rshift does 'return (PyObject *) maybe_small_long(z);' for both cases (whether a is a negative int or not).
      Calling maybe_small_long in case a is a negative int is redundant, as long_invert returns (in case it doesn't fail) by either doing 'return PyLong_FromLong(-(MEDIUM_VALUE(v)+1));' or 'return (PyObject *)maybe_small_long(x);'. In both cases, long_invert would return a reference to an element of small_ints if it should.

   Calls to maybe_small_long were added both to long_rshift and long_invert in revision 48567, as part of an effort to wipe out different places in the code where small_ints could be used (to save up memory), but was not. I am not sure why maybe_small_long was added to long_rshift where it would be redundant in case a is a negative int.

   3. In different cases of failure, long_rshift does 'goto rshift_error;'.
      The destination of these goto statements is actually a return statement that would also be reached in almost any case of success (except for a certain case in which the result of the shift is obviously zero).

   That goto was added in revision 15725. Back then, CONVERT_BINOP was added, and calling it in long_rshift required calling Py_DECREF for a and b before returning.
   Later on, in revision 43313, CONVERT_BINOP was removed, along with the calls to Py_DECREF it required, but the goto was left untouched, and remained there to this day.

   ------------ proposed changes ------------
   All of the proposed changes are in Objects/longobject.c in long_rshift:
   1. Remove the check whether a is a negative int in the block that gets executed in case a is not a negative int.

   2. Move the call to maybe_small_long inside the block that runs in case a is not a negative int.
   
   3. Replace every 'goto rshift_error;' with a 'return NULL;', and remove the rshift_error label. 
   
   I could have kept the goto statements, with 'return (PyObject *)z;' as their destination, but I believe it would have been less clear. Also, there are many similar places in longobject.c where 'return NULL;' is done on failure.
   

   ------------ diff ------------
   The proposed patches diff file is attached.

   ------------ tests ------------
   I built the patched CPython for x86, and played with it a little. Everything seemed to work as usual.
   Specifically, I did:
   for i in range(10000):
   if not all(smallInt is ((smallInt << i) >> i) for (
   smallInt) in range(-5, 257)):
   break
   print(i)
   And indeed 9999 was printed.

   In addition, I ran 'python_d.exe -m test -j3' (on my 64-bit Windows 10) with and without the patches, and got quite the same output.
   the outputs of both runs are attached.
2. orenmn

   added

   interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)

   performancePerformance or resource usagePerformance or resource usage

   on Jun 4, 2016
3. orenmn

   added

   3.7 (EOL)end of lifeend of life

   on Sep 13, 2016
4. mdickinson commented on Sep 13, 2016

   

   mdickinson

   on Sep 13, 2016

   Member

   More actions

   Sorry, this fell off my list of things to look at.
5. mdickinson

   self-assigned this

   on Sep 13, 2016
6. python-dev commented on Sep 17, 2016

   

   python-devmannequin

   on Sep 17, 2016

   Mannequin

   More actions

   New changeset 21b70c835b5b by Mark Dickinson in branch 'default':
   Issue bpo-27222: various cleanups in long_rshift. Thanks Oren Milman.
   https://hg.python.org/cpython/rev/21b70c835b5b
7. mdickinson commented on Sep 17, 2016

   

   mdickinson

   on Sep 17, 2016

   Member

   More actions

   Thanks for the patch and the thorough analysis!
8. mdickinson

   closed this as completedon Sep 17, 2016
9. ezio-melotti

   transferred this issue fromon Apr 10, 2022