msg217822 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
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) * |
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) * |
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) * |
Date: 2014-05-04 20:03 |
The reason to apply the patch is to reduce the memory footprint.
|
msg217896 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
Date: 2014-05-12 18:58 |
Use assertIs, since True == 1, but True is not 1.
|
msg218358 - (view) |
Author: Roundup Robot (python-dev) |
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) * |
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) * |
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) * |
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) * |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:03 | admin | set | github: 65621 |
2014-05-13 16:28:57 | francismb | set | messages:
+ msg218472 |
2014-05-12 22:42:21 | r.david.murray | set | messages:
+ msg218376 |
2014-05-12 22:05:50 | r.david.murray | set | messages:
+ msg218372 |
2014-05-12 21:18:41 | vstinner | set | messages:
+ msg218364 |
2014-05-12 21:04:49 | francismb | set | messages:
+ msg218363 |
2014-05-12 20:44:42 | vstinner | set | status: open -> closed resolution: rejected messages:
+ msg218359
|
2014-05-12 20:43:29 | python-dev | set | nosy:
+ python-dev messages:
+ msg218358
|
2014-05-12 18:58:44 | Arfrever | set | messages:
+ msg218350 |
2014-05-12 18:51:04 | r.david.murray | set | nosy:
+ r.david.murray messages:
+ msg218348
|
2014-05-12 18:17:45 | francismb | set | nosy:
+ francismb messages:
+ msg218343
|
2014-05-11 11:58:40 | Arfrever | set | nosy:
+ Arfrever
|
2014-05-07 23:47:30 | jcea | set | nosy:
+ jcea
|
2014-05-05 08:54:21 | mark.dickinson | set | messages:
+ msg217913 |
2014-05-04 22:01:25 | pitrou | set | nosy:
+ pitrou messages:
+ msg217896
|
2014-05-04 20:03:28 | vstinner | set | messages:
+ msg217886 |
2014-05-04 09:27:50 | mark.dickinson | set | messages:
+ msg217862 |
2014-05-03 22:41:29 | vstinner | set | messages:
+ msg217837 |
2014-05-03 22:15:59 | rhettinger | set | nosy:
+ rhettinger messages:
+ msg217834
|
2014-05-03 12:34:17 | vstinner | create | |