classification
Title: a redundant check in maybe_small_long
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: Oren Milman, mark.dickinson, serhiy.storchaka, vstinner, ztane
Priority: normal Keywords: patch

Created on 2016-09-25 12:38 by Oren Milman, last changed 2017-03-03 11:19 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
patchedCPythonTestOutput_ver1.txt Oren Milman, 2016-09-25 12:41 test output of CPython with my patches (tested on my PC) - ver1
CPythonTestOutput.txt Oren Milman, 2016-09-25 12:42 test output of CPython without my patches (tested on my PC)
issue28272_ver1.diff Oren Milman, 2016-09-25 12:42 proposed patches diff file - ver1 review
Messages (12)
msg277371 - (view) Author: Oren Milman (Oren Milman) * Date: 2016-09-25 12:38
------------ current state ------------
In Objects/longobject.c, the function maybe_small_long first checks whether v (the received PyLongObject pointer) is not NULL. 
However, currently in every call to maybe_small_long, it is already guaranteed that v is not NULL, which makes that check redundant.
(Currently, the following are the only functions that call maybe_small_long:
    * PyLong_FromString
    * long_divrem
    * long_rshift
    * long_lshift
    * long_bitwise)

With regard to relevant changes made in the past, maybe_small_long remained quite the same since it was added, in changeset 48567 (https://hg.python.org/cpython/rev/1ce7e5c5a761) - in particular, the check (whether v is not NULL) was always there.
When it was added, both long_rshift and long_lshift might have called maybe_small_long with v as NULL, which seems like the reason for adding the check back then.


------------ proposed changes ------------
In Objects/longobject.c in maybe_small_long, remove the check whether v is not NULL, and add an 'assert(v != NULL);'.


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


------------ tests ------------
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.
msg277408 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-09-26 08:12
Thanks for the patch. I'm a bit reluctant to make changes like this unless there's a measurable performance benefit. The extra check in maybe_small_long does have a value in terms of readability and safety with respect to future maintenance, and I'd expect a competent compiler to inline maybe_small_long and elide the extra check (though I haven't looked at the assembly output to check that). So I think we shouldn't change this unless there's a clear benefit to doing so.
msg277409 - (view) Author: Oren Milman (Oren Milman) * Date: 2016-09-26 10:19
I looked at the assembly, and saw maybe_small_long is not inlined, but I agree that other compilers (or future compilers) might introduce some optimization that would elide the check.

Also, as expected (thanks to branch prediction, I guess), a little microbenchmark shows my patch doesn't improve performance:
without my patch:
    python.exe -m perf timeit "122 >> 2"
    ....................
    Median +- std dev: 20.5 ns +- 0.5 ns

    python.exe -m perf timeit "215 << 1"
    ....................
    Median +- std dev: 20.6 ns +- 0.5 ns
with my patch:
    python.exe -m perf timeit "122 >> 2"
    ....................
    Median +- std dev: 20.6 ns +- 0.3 ns

    python.exe -m perf timeit "215 << 1"
    ....................
    Median +- std dev: 20.6 ns +- 0.4 ns
msg277417 - (view) Author: Antti Haapala (ztane) * Date: 2016-09-26 13:23
Totally inlined when built with GCC, release mode, there is no trace of the maybe_small_long symbol, I couldn't even really decipher where it was being done in the disassembly of longobject.c. Was this the Windows release build?
msg277418 - (view) Author: Oren Milman (Oren Milman) * Date: 2016-09-26 13:31
my build (release):
Python 3.7.0a0 (default:da2c96cf2ce6, Sep 26 2016, 13:08:47) [MSC v.1900 32 bit (Intel)] on win32

ISTM we can close this issue.
msg288783 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-02 07:57
ping (just to close the issue, I think)
msg288784 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-02 08:15
Try to bench list(range(256)). It should create small ints in tight loop.
msg288789 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-03-02 09:58
issue28272_ver1.diff LGTM.

Mark Dickinson: "The extra check in maybe_small_long does have a value in (...) safety with respect to future maintenance,"

"assert(v != NULL);" has the same purpose. I think that it's ok to require that maybe_small_long() isn't call with NULL. The function is called 5 times and is never called with NULL. Note: long_normalize() cannot fail, it modifies the object in-place and returns the input object.


Mark Dickinson: "I'm a bit reluctant to make changes like this unless there's a measurable performance benefit."

I'm unable to see any impact on performance. Following microbenchmark is not significant:
---
haypo@smithers$ ./python -m perf timeit -s 'x=1' 'x<<1>>1<<1>>1<<1>>1<<1>>1<<1>>1<<1>>1<<1>>1<<1>>1' --duplicate=100 --compare-to ../master-ref/python --python-names=ref:patch
ref: ..................... 531 ns +- 10 ns
patch: ..................... 531 ns +- 2 ns

Median +- std dev: [ref] 531 ns +- 10 ns -> [patch] 531 ns +- 2 ns: 1.00x faster (-0%)
---

Note: python.exe -m perf timeit "122 >> 2" doesn't test long_rshirt() since "122 >> 2" is computed by the Python compiler, not at runtime ;-)
msg288790 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-03-02 10:00
> Try to bench list(range(256)). It should create small ints in tight loop.

This expression doesn't call maybe_small_long().
msg288865 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-03 07:14
So, should I open a pull request?
(as some time had passed, I would also run again the tests, etc.)
msg288875 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-03-03 10:26
> So, should I open a pull request?

I consider that Mark Dickinson is the maintained of longobject.c, so please for his feedback before going further. It seems like Mark consider that the change is worthless and bad for maintainability/sustainability.
msg288878 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-03-03 11:19
Without a demonstrable performance improvement, I'm opposed to making this change. Closing.
History
Date User Action Args
2017-03-03 11:19:44mark.dickinsonsetstatus: open -> closed
resolution: rejected
messages: + msg288878

stage: resolved
2017-03-03 10:26:08vstinnersetmessages: + msg288875
2017-03-03 07:14:12Oren Milmansetmessages: + msg288865
2017-03-02 10:00:35vstinnersetmessages: + msg288790
2017-03-02 09:58:55vstinnersetnosy: + vstinner
messages: + msg288789
2017-03-02 08:15:28serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg288784
2017-03-02 07:57:04Oren Milmansetmessages: + msg288783
2016-09-26 13:31:28Oren Milmansetmessages: + msg277418
2016-09-26 13:23:56ztanesetnosy: + ztane
messages: + msg277417
2016-09-26 10:19:48Oren Milmansetmessages: + msg277409
2016-09-26 08:12:23mark.dickinsonsetassignee: mark.dickinson

messages: + msg277408
nosy: + mark.dickinson
2016-09-25 12:42:48Oren Milmansetfiles: + issue28272_ver1.diff
keywords: + patch
2016-09-25 12:42:21Oren Milmansetfiles: + CPythonTestOutput.txt
2016-09-25 12:41:42Oren Milmansetfiles: + patchedCPythonTestOutput_ver1.txt
2016-09-25 12:40:59Oren Milmansetfiles: - CPythonTestOutput.txt
2016-09-25 12:40:49Oren Milmansetfiles: - patchedCPythonTestOutput_ver1.txt
2016-09-25 12:39:24Oren Milmansetfiles: + patchedCPythonTestOutput_ver1.txt
2016-09-25 12:38:49Oren Milmancreate