This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: int << 0: return the number unmodified
Type: performance Stage:
Components: Versions: Python 3.5
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, francismb, jcea, mark.dickinson, pitrou, python-dev, r.david.murray, rhettinger, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2014-05-03 12:34 by vstinner, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
long_lshift0.patch vstinner, 2014-05-03 12:34 review
Messages (17)
msg217822 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-05-03 12:34
I propose to add a micro-optimization for int << 0: return the number unmodified.

See attached patch.
msg217834 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-05-03 22:15
Every branch has a cost (in particular, it tends to contaminate global branch prediction tables and blow other code out of the L1 code cache).  The cost isn't big, but branches shouldn't be added unless we know there is a real benefit.

I would think that in real-world code, this branch will almost never be taken.  The common case will pay a price (albiet a small one) for almost zero benefit.
msg217837 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-05-03 22:41
> I would think that in real-world code, this branch will almost never be
taken.  The common case will pay a price (albiet a small one) for almost
zero benefit.

I think that x << 0 is common even if it's not written like that (it's more
for i in range(8): ... x << i).

The benefit is to reduce the memory footprint.

Can you sow the overhead of the branch in a microbenchmark? The test is
only done once, it's out of a loop.

*If* it's possible to notice an overhead, we can maybe use GCC macro
unlikely().
msg217862 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2014-05-04 09:27
> Can you sow the overhead of the branch in a microbenchmark?

Conversely, can you show a case where this optimisation provides a benefit in real code?  We should be looking for a reason *to* apply the patch, not a reason *not* to apply the patch.
msg217886 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-05-04 20:03
The reason to apply the patch is to reduce the memory footprint.
msg217896 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-05-04 22:01
Reduce the memory footprint in which actual workload? This looks rather gratuitous to me.
msg217913 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2014-05-05 08:54
BTW, there's also a behaviour change here.  Before the patch:

>>> True << 0
1

After the patch:

>>> True << 0
True

Which demonstrates another good reason to avoid trivial-looking optimisations: it's far too easy to accidentally change behaviour.
msg218343 - (view) Author: Francis MB (francismb) * Date: 2014-05-12 18:17
Hi,
sorry if it's trivial but shouldn't we add a 'shifted_true' test
some were to make sure that this behavior change gets noticed next time?

def test_shifted_true(self):
    self.assertEqual(True << 0, 1)
    self.assertEqual(True << 1, 2)
msg218348 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-05-12 18:51
Francisco: I'd say that was a good idea.  Would you like to propose a patch? (ie: figure out where it should go)
msg218350 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * (Python triager) Date: 2014-05-12 18:58
Use assertIs, since True == 1, but True is not 1.
msg218358 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-05-12 20:43
New changeset ef49aaad3812 by Victor Stinner in branch '3.4':
Issue #21422: Add a test to check that bool << int and bool >> int return an int
http://hg.python.org/cpython/rev/ef49aaad3812

New changeset 3da4aed1d18a by Victor Stinner in branch 'default':
(Merge 3.4) Issue #21422: Add a test to check that bool << int and bool >> int
http://hg.python.org/cpython/rev/3da4aed1d18a
msg218359 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-05-12 20:44
Ok, 3 core dev are opposed to the change, I close the issue.

I added a test on bool >> int and bool << int to ensure that the result is an int.
msg218363 - (view) Author: Francis MB (francismb) * Date: 2014-05-12 21:04
I Victor you were so fast, I started with one patch also in bool (at least the place was right). The problem is that I was getting some extrage (for me at least). As far I hat:

    def test_shifted_true(self):
    	with self.assertRaises(ValueError):
    	    True << -1
    	self.assertIs(True << 0, 1)
    	self.assertIs(True << 1, 2)
    	self.assertEqual(True << 63, 1 << 63)
    	self.assertIs(True << 63, 1 << 63)

And I'm getting:


======================================================================
FAIL: test_shifted_true (test.test_bool.BoolTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ci/Prog/cpython/src/Lib/test/test_bool.py", line 349, in test_shifted_true
    self.assertIs(True << 63, 1 << 63)
AssertionError: 9223372036854775808 is not 9223372036854775808

----------------------------------------------------------------------

That's:
./python --version
Python 3.5.0a0

>>> type(True<<63)
<class 'int'>
>>> type(1<<63)
<class 'int'>

hg tip
changeset:   90664:4e33c343a264

What I'm doing wrong?
That's:
./python --version
Python 3.5.0a0

>>> type(True<<63)
<class 'int'>
>>> type(1<<63)
<class 'int'>

hg tip
changeset:   90664:4e33c343a264

What I was doing wrong?

Thanks in advance!
francis
msg218364 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-05-12 21:18
> What I was doing wrong?

The "is" operator should only be used to compare identical objects. Small integers (range -5..255 if I remember correctly) are singletons. I prefer to not rely on this implementation detail in a unit test of the Python standard library.
msg218372 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-05-12 22:05
Arfrever's advice was misleading...the test would have needed to be assertIsNot(True << 0, 1), but the fact that True is not preserved is not really what we want to test.  What we want to test is that the return value is of type 'int', which is what Victor's test checks.
msg218376 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-05-12 22:42
Arfrever pointed out on irc that I misread.  It would indeed be assertIs(True << 0, 1), but that wouldn't make a good test because the fact that '1 is 1' is an implementation detail of CPython and can't be relied upon.
msg218472 - (view) Author: Francis MB (francismb) * Date: 2014-05-13 16:28
>
> What we want to test is that the return value is of type 'int', which is what Victor's test checks.
>
Thank you for the explanations!

for 2.7.6 type(2 << 62) is <type 'long'> and type(2 << 61) is
<type 'int'> (I suppose it's analogous in a 32 bit machine
around type(2 << 30) so I just wanted to test on that limits).

Is that still relevant? or is too much detail and we should stop here.

Regards,
francis
History
Date User Action Args
2022-04-11 14:58:03adminsetgithub: 65621
2014-05-13 16:28:57francismbsetmessages: + msg218472
2014-05-12 22:42:21r.david.murraysetmessages: + msg218376
2014-05-12 22:05:50r.david.murraysetmessages: + msg218372
2014-05-12 21:18:41vstinnersetmessages: + msg218364
2014-05-12 21:04:49francismbsetmessages: + msg218363
2014-05-12 20:44:42vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg218359
2014-05-12 20:43:29python-devsetnosy: + python-dev
messages: + msg218358
2014-05-12 18:58:44Arfreversetmessages: + msg218350
2014-05-12 18:51:04r.david.murraysetnosy: + r.david.murray
messages: + msg218348
2014-05-12 18:17:45francismbsetnosy: + francismb
messages: + msg218343
2014-05-11 11:58:40Arfreversetnosy: + Arfrever
2014-05-07 23:47:30jceasetnosy: + jcea
2014-05-05 08:54:21mark.dickinsonsetmessages: + msg217913
2014-05-04 22:01:25pitrousetnosy: + pitrou
messages: + msg217896
2014-05-04 20:03:28vstinnersetmessages: + msg217886
2014-05-04 09:27:50mark.dickinsonsetmessages: + msg217862
2014-05-03 22:41:29vstinnersetmessages: + msg217837
2014-05-03 22:15:59rhettingersetnosy: + rhettinger
messages: + msg217834
2014-05-03 12:34:17vstinnercreate