Repository navigation
inline function generates slightly inefficient machine code #82196
Description
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 = 'https://github.com/rhettinger'
closed_at = <Date 2019-09-10.02:18:12.981>
created_at = <Date 2019-09-03.04:02:44.694>
labels = ['interpreter-core', '3.9']
title = 'inline function generates slightly inefficient machine code'
updated_at = <Date 2019-09-18.05:11:29.360>
user = 'https://github.com/animalize'bugs.python.org fields:
activity = <Date 2019-09-18.05:11:29.360>
actor = 'Greg Price'
assignee = 'rhettinger'
closed = True
closed_date = <Date 2019-09-10.02:18:12.981>
closer = 'malin'
components = ['Interpreter Core']
creation = <Date 2019-09-03.04:02:44.694>
creator = 'malin'
dependencies = []
files = ['48583']
hgrepos = []
issue_num = 38015
keywords = ['patch']
message_count = 12.0
messages = ['351052', '351053', '351054', '351228', '351255', '351278', '351315', '351316', '351348', '351387', '351564', '352692']
nosy_count = 7.0
nosy_names = ['rhettinger', 'mark.dickinson', 'benjamin.peterson', 'malin', 'Greg Price', 'sir-sigurd', 'aeros']
pr_nums = ['15710', '15718']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue38015'
versions = ['Python 3.9']
Activity
animalize commentedon Sep 3, 2019
Commit 5e63ab0 replaces macro with this inline function:
(by default, NSMALLNEGINTS is 5, NSMALLPOSINTS is 257)
However, when invoking this function, and
sizeof(value) < sizeof(long long), there is an unnecessary type casting.For example, on 32-bit platform, if
valueisPy_ssize_t, it needs to be converted to 8-bytelong longtype.The following assembly code is the beginning part of
PyLong_FromSsize_t(Py_ssize_t v)function.(32-bit x86 build generated by GCC 9.2, with
-m32 -O2option)Use macro before commit 5e63ab0:
mov eax, DWORD PTR [esp+4]
add eax, 5
cmp eax, 261
ja .L2
sal eax, 4
add eax, OFFSET FLAT:small_ints
add DWORD PTR [eax], 1
ret
.L2: jmp PyLong_FromSsize_t_rest(int)
Use inlined function:
push ebx
mov eax, DWORD PTR [esp+8]
mov edx, 261
mov ecx, eax
mov ebx, eax
sar ebx, 31
add ecx, 5
adc ebx, 0
cmp edx, ecx
mov edx, 0
sbb edx, ebx
jc .L7
cwde
sal eax, 4
add eax, OFFSET FLAT:small_ints+80
add DWORD PTR [eax], 1
pop ebx
ret
.L7: pop ebx
jmp PyLong_FromSsize_t_rest(int)
On 32-bit x86 platform, 8-byte
long longis implemented in using two registers, so the machine code is much longer than macro version.If you want to see assembly code generated by major compilers, you can paste attached file demo.c to https://godbolt.org/
-m32 -O2to generate 32-bit build.rhettinger commentedon Sep 3, 2019
Do you see a way to salvage the commit?
If not, I'm fine with reverting it. Its benefit was only aesthetic.
animalize commentedon Sep 3, 2019
There will always be a new commit, replacing with a macro version also looks good.
I have no opinion, both are fine.
animalize commentedon Sep 6, 2019
Revert commit 5e63ab0 or use PR 15710, both are fine.
sir-sigurd commentedon Sep 6, 2019
I added similar patch that replaces get_small_int() with macro version, since it also induces unnecessary casts and makes machine code less efficient.
Example assembly can be checked at https://godbolt.org/z/1SjG3E.
This change produces tiny, but measurable speed-up for handling small ints:
Mean +- std dev: [/home/sergey/tmp/cpython-master/venv/bin/python] 1.03 us +- 0.08 us -> [/home/sergey/tmp/cpython-dev/venv/bin/python] 973 ns +- 18 ns: 1.05x faster (-5%)
animalize commentedon Sep 7, 2019
This range has not been changed since "preallocated small integer pool" was introduced:
The commit (Jan 2007):
ddefaf3
Is it worth increase the range?
FYI, build with MSVC 2017, the
small_intssize:32-bit build:
sizeof(PyLongObject) 16 bytes
sizeof(small_ints) 4192 bytes
64-bit build:
sizeof(PyLongObject) 32 bytes
sizeof(small_ints) 8384 bytes
animalize commentedon Sep 8, 2019
I didn't get measurable change, I run this command a dozen times and take the best result:
D:\dev\cpython\PCbuild\amd64\python.exe -m pyperf timeit -s "from collections import deque; consume = deque(maxlen=0).extend; r = range(256)" "consume(r)" --duplicate=1000
before: Mean +- std dev: 771 ns +- 16 ns
after: Mean +- std dev: 770 ns +- 10 ns
Environment:
64-bit release build by MSVC 2017
CPU: i3 4160, System: latest Windows 10 64-bit
Check the machine code from godbolt.org, x64 MSVC v19.14 only saves one instruction:
movsxd rax, ecx
x86-64 GCC 9.2 saves two instructions:
lea eax, [rdi+5]
cdqe
sir-sigurd commentedon Sep 8, 2019
I use GCC 9.2.
gnprice commentedon Sep 9, 2019
(Just to help keep discussions together: some earlier discussion was on #59421 .)
Because is_small_int / IS_SMALL_INT is so small, there's not much cost in the source code to making it a macro (as #59915 did).
But I think it'd be a mistake to go a lot farther than that and convert significantly larger chunks of code like get_small_int to macros (as #59923 would), unless the payoff were really commensurate. It'd be better to focus optimization efforts where profiling shows a lot of time is really being spent.
benjaminp commentedon Sep 9, 2019
I agree with msg351348. The performance wins are very context-dependent and even then very small. It's not worth further disturbing the small int code. So, we should close this issue out.
animalize commentedon Sep 10, 2019
PR 15710 has been merged into the master, but the merge message is not shown here.
Commit: 6b51998
Maybe this issue can be closed.
gnprice commentedon Sep 18, 2019
See followup at https://bugs.python.org/issue38205 and https://bugs.python.org/issue37812#msg352670 .
The patch in #59915 had a side effect of introducing a call to
Py_UNREACHABLEinside a comma-expression. A subsequent commit 3ab6147 changedPy_UNREACHABLEin a way that made that a syntax error; the issue wasn't caught then because the code is only compiled ifNSMALLNEGINTS + NSMALLPOSINTS <= 0. Detailed discussion on those threads.