Raise SystemError if a function returns a result with an exception set

[vstinner]

BPO	23571
Nosy	@jcea, @pitrou, @vstinner, @berkerpeksag, @serhiy-storchaka, @timgraham
Files	check_result.patch check_result-2.patch check_result-3.patch check_result-4.patch check_result-5.patch

^{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/vstinner'
closed_at = <Date 2015-04-06.21:30:56.045>
created_at = <Date 2015-03-03.11:58:41.396>
labels = ['interpreter-core']
title = 'Raise SystemError if a function returns a result with an exception set'
updated_at = <Date 2015-11-04.23:34:05.885>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2015-11-04.23:34:05.885>
actor = 'jcea'
assignee = 'vstinner'
closed = True
closed_date = <Date 2015-04-06.21:30:56.045>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2015-03-03.11:58:41.396>
creator = 'vstinner'
dependencies = []
files = ['38312', '38356', '38357', '38358', '38359']
hgrepos = []
issue_num = 23571
keywords = ['patch']
message_count = 41.0
messages = ['237123', '237128', '237132', '237348', '237349', '237350', '237351', '237353', '237355', '237357', '237359', '237360', '237364', '237387', '237394', '237396', '237755', '238192', '238749', '238786', '238789', '238805', '238806', '238810', '238812', '238814', '238816', '238827', '238861', '238880', '239110', '239113', '239116', '239118', '239144', '239206', '239210', '239551', '239567', '239581', '240185']
nosy_count = 7.0
nosy_names = ['jcea', 'pitrou', 'vstinner', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'Tim.Graham']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue23571'
versions = ['Python 3.5']

[vstinner]

Attached patch changes PyObject_Call() and PyCFunction_Call() to raise a SystemError and destroy the result (Py_DECREF) if a function returns a result with an exception set.

I also refactored PyCFunction_Call() and added "assert(!PyErr_Occurred());" at the beginning of PyCFunction_Call().

I removed duplicate checks in ceval.c.

--

I didn't check the impact on performances.

If the patch has a significant overhead: _Py_CheckFunctionResult() may be marked to be inlined, and PyCFunction_Call() and PyObject_Call() checks may be marked as unlikely using GCC __builtin_expect(), something like:

#ifdef __GNUC__
#  define Py_likely(x)       __builtin_expect((x),1)
#  define Py_unlikely(x)     __builtin_expect((x),0)
#else
#  define Py_likely(x)       x
#  define Py_unlikely(x)     x
#endif

Be careful of __builtin_expect(), misused it can make the code slower! (benchmark, benchmark, benchmark!)

[serhiy-storchaka]

If the patch has a significant overhead: _Py_CheckFunctionResult() may be
marked to be inlined, and PyCFunction_Call() and PyObject_Call() checks may
be marked as unlikely using GCC __builtin_expect(), something like:

Could you please open separate issue or even a topic on Python-Ideas for
Py_likely/Py_unlikely? I prefer spelling _Py_LIKELY.

[vstinner]

2015-03-03 13:49 GMT+01:00 Serhiy Storchaka <report@bugs.python.org>:

> If the patch has a significant overhead: _Py_CheckFunctionResult() may be
> marked to be inlined, and PyCFunction_Call() and PyObject_Call() checks may
> be marked as unlikely using GCC __builtin_expect(), something like:

Could you please open separate issue or even a topic on Python-Ideas for
Py_likely/Py_unlikely? I prefer spelling _Py_LIKELY.

I'm not really interested by micro-benchmark here :-) I also propose
solutions *if* my patch makes the code slower.

Since __builtin_expect() may have a negative effect on performances, I
would prefer to avoid it if we can :-)

[vstinner]

I ran http://hg.python.org/benchmarks/

Result:
---

$ python3 perf.py -r -b default ~/prog/python/default/python.orig  ~/prog/python/default/python.patched 
INFO:root:Automatically selected timer: perf_counter
(...)
Report on Linux smithers 3.18.3-201.fc21.x86_64 #1 SMP Mon Jan 19 15:59:31 UTC 2015 x86_64 x86_64
Total CPU cores: 8

### etree_parse ###
Min: 0.273882 -> 0.297294: 1.09x slower
Avg: 0.277947 -> 0.312787: 1.13x slower
Significant (t=-11.54)
Stddev: 0.00446 -> 0.02987: 6.6906x larger

### fastunpickle ###
Min: 0.510721 -> 0.526520: 1.03x slower
Avg: 0.521282 -> 0.542610: 1.04x slower
Significant (t=-8.33)
Stddev: 0.01024 -> 0.02348: 2.2931x larger

The following not significant results are hidden, use -v to show them:
2to3, django_v2, etree_generate, etree_iterparse, etree_process, fastpickle, json_dump_v2, json_load, nbody, regex_v8, tornado_http.
---

I ran again etree_parse alone and the slowdown disappeared :-p
---
The following not significant results are hidden, use -v to show them:
etree_parse.
---

Output when I ran fastunpickle alone:
---
### fastunpickle ###
Min: 0.510994 -> 0.522419: 1.02x slower
Avg: 0.524633 -> 0.536876: 1.02x slower
Significant (t=-2.69)
Stddev: 0.02983 -> 0.03436: 1.1518x larger
---

2% slower on a single test doesn't look to be significant. Raising a SystemError is more important than the minor slowdown. What do you think?

[vstinner]

I ran pybench, even if I know that it's not really revelant: overall 0.3% slowdown. But pybench doesn't look reliable: some tests are faster, which looks like noise in the benchmark.

-------------------------------------------------------------------------------
PYBENCH 2.1
-------------------------------------------------------------------------------

• using CPython 3.5.0a1+ (default:648b35f22b91+, Mar 6 2015, 13:18:57) [GCC 4.9.2 20141101 (Red Hat 4.9.2-1)]
• disabled garbage collection
• system check interval set to maximum: 2147483647
• using timer: time.perf_counter
• timer: resolution=1e-09, implementation=clock_gettime(CLOCK_MONOTONIC)

-------------------------------------------------------------------------------
Benchmark: orig.pybench
-------------------------------------------------------------------------------

Rounds: 10
Warp:   10
Timer:  time.perf_counter

Machine Details:
   Platform ID:    Linux-3.18.3-201.fc21.x86_64-x86_64-with-fedora-21-Twenty_One
   Processor:      x86_64

Python:
   Implementation: CPython
   Executable:     /home/haypo/prog/python/default/python.orig
   Version:        3.5.0a1+
   Compiler:       GCC 4.9.2 20141101 (Red Hat 4.9.2-1)
   Bits:           64bit
   Build:          Mar  6 2015 13:19:47 (#default:648b35f22b91)
   Unicode:        UCS4

-------------------------------------------------------------------------------
Comparing with: patch.pybench
-------------------------------------------------------------------------------

Rounds: 10
Warp:   10
Timer:  time.perf_counter

Machine Details:
   Platform ID:    Linux-3.18.3-201.fc21.x86_64-x86_64-with-fedora-21-Twenty_One
   Processor:      x86_64

Python:
   Implementation: CPython
   Executable:     /home/haypo/prog/python/default/python.patched
   Version:        3.5.0a1+
   Compiler:       GCC 4.9.2 20141101 (Red Hat 4.9.2-1)
   Bits:           64bit
   Build:          Mar  6 2015 13:18:57 (#default:648b35f22b91+)
   Unicode:        UCS4

Test minimum run-time average run-time
this other diff this other diff
-------------------------------------------------------------------------------
BuiltinFunctionCalls: 41ms 41ms -1.1% 41ms 41ms -1.5%
BuiltinMethodLookup: 24ms 24ms +0.3% 24ms 25ms -0.8%
CompareFloats: 26ms 26ms +2.2% 27ms 26ms +1.2%
CompareFloatsIntegers: 59ms 60ms -0.9% 60ms 61ms -2.5%
CompareIntegers: 38ms 36ms +4.6% 38ms 37ms +3.5%
CompareInternedStrings: 26ms 25ms +0.6% 26ms 26ms -0.5%
CompareLongs: 22ms 21ms +1.1% 22ms 22ms +0.0%
CompareStrings: 22ms 23ms -1.3% 23ms 23ms -1.3%
ComplexPythonFunctionCalls: 40ms 40ms -1.5% 40ms 41ms -2.5%
ConcatStrings: 28ms 28ms -0.3% 28ms 29ms -1.2%
CreateInstances: 41ms 42ms -1.7% 42ms 43ms -2.6%
CreateNewInstances: 31ms 32ms -2.2% 32ms 33ms -3.2%
CreateStringsWithConcat: 59ms 56ms +5.6% 60ms 58ms +2.7%
DictCreation: 43ms 48ms -10.3% 44ms 50ms -12.7%
DictWithFloatKeys: 36ms 37ms -4.8% 36ms 38ms -6.0%
DictWithIntegerKeys: 28ms 29ms -4.1% 29ms 31ms -6.4%
DictWithStringKeys: 24ms 26ms -8.9% 24ms 27ms -10.3%
ForLoops: 22ms 22ms -0.9% 22ms 23ms -2.2%
IfThenElse: 31ms 33ms -5.9% 31ms 34ms -7.9%
ListSlicing: 35ms 35ms -0.1% 36ms 36ms -2.0%
NestedForLoops: 34ms 35ms -2.3% 34ms 36ms -4.7%
NestedListComprehensions: 37ms 37ms -1.2% 37ms 40ms -7.6%
NormalClassAttribute: 77ms 71ms +8.1% 77ms 74ms +3.9%
NormalInstanceAttribute: 37ms 36ms +2.1% 38ms 38ms -0.6%
PythonFunctionCalls: 35ms 34ms +1.7% 35ms 35ms -0.5%
PythonMethodCalls: 45ms 44ms +3.3% 46ms 45ms +0.7%
Recursion: 60ms 58ms +3.7% 60ms 59ms +1.0%
SecondImport: 33ms 33ms -0.0% 33ms 33ms -1.9%
SecondPackageImport: 34ms 34ms +0.1% 34ms 34ms -2.0%
SecondSubmoduleImport: 83ms 86ms -3.5% 85ms 87ms -2.4%
SimpleComplexArithmetic: 23ms 23ms +0.7% 23ms 24ms -2.2%
SimpleDictManipulation: 51ms 53ms -3.8% 52ms 55ms -5.7%
SimpleFloatArithmetic: 24ms 24ms +0.3% 24ms 25ms -2.0%
SimpleIntFloatArithmetic: 31ms 30ms +1.1% 31ms 31ms -0.1%
SimpleIntegerArithmetic: 30ms 30ms +0.0% 30ms 31ms -2.0%
SimpleListComprehensions: 30ms 32ms -4.6% 31ms 33ms -5.7%
SimpleListManipulation: 27ms 26ms +2.4% 27ms 27ms +0.4%
SimpleLongArithmetic: 21ms 21ms +2.1% 22ms 22ms +0.2%
SmallLists: 36ms 36ms -1.3% 36ms 37ms -3.0%
SmallTuples: 43ms 43ms -0.5% 44ms 45ms -2.5%
SpecialClassAttribute: 75ms 72ms +4.3% 75ms 73ms +2.5%
SpecialInstanceAttribute: 38ms 41ms -7.7% 38ms 42ms -9.0%
StringMappings: 77ms 76ms +1.4% 77ms 78ms -0.3%
StringPredicates: 43ms 44ms -2.6% 44ms 45ms -3.9%
StringSlicing: 39ms 38ms +3.2% 39ms 39ms +1.0%
TryExcept: 21ms 21ms -2.8% 21ms 22ms -4.8%
TryFinally: 29ms 29ms +0.2% 29ms 29ms -1.2%
TryRaiseExcept: 11ms 11ms -0.2% 11ms 11ms -1.1%
TupleSlicing: 42ms 43ms -0.8% 45ms 45ms +0.5%
WithFinally: 45ms 45ms -0.8% 45ms 45ms -1.2%
WithRaiseExcept: 36ms 36ms -0.9% 36ms 36ms -1.5%
-------------------------------------------------------------------------------
Totals: 1920ms 1927ms -0.3% 1941ms 1980ms -2.0%

(this=orig.pybench, other=patch.pybench)

[vstinner]

Even worse benchmark: timeit :-)

python -m timeit -s 'def f(): pass' 'f()'

• original: 67.5 nanoseconds
• patched: 64.1 nanoseconds (faster!?)

[vstinner]

New patch which removes some assertions and move other assertions on result/PyErr_Occurred.

I removed useless duplicated assertions and move the final assertion at the end of PyEval_EvalFrameEx() to guarantee that the function behave correctly.

[vstinner]

New patch to take in account Serhiy's comments. I also fixed call_function(), I forgot to call _Py_CheckFunctionResult() in two cases.

[serhiy-storchaka]

I'm not sure that all removed assertions are not needed. There are many ways to get raised exception, not only by calling a function.

[vstinner]

Updated patch to take in account more Serhiy's comments.

[serhiy-storchaka]

Could you please run microbenchmarks from bpo-23507?

[vstinner]

Oh, test_io and test_sqlite are failing in release mode with check_result-4.patch. io and sqlite modules only call PyErr_Clear() in debug mode. It's my fault, I added them, but I made them conditionnal to not impact performances. It's now fixed in the new patch.

Patch 5:

• "./python -m test -j0 -rW" succeeded in debug mode
• "./python -m test -j0 -rW" succeeded in release mode
• pybench: overall 0.6% slower with the patch (on the minimum runtime column)

Micro-benchmarks of issue bpo-23507:

$ ./python -m timeit -s "f = lambda x: x" -s "s = list(range(1000))" -- "list(filter(f, s))"
Unpatched: 10000 loops, best of 3: 96.3 usec per loop
Patched:   10000 loops, best of 3: 99.2 usec per loop (+3.0%)

$ ./python -m timeit -s "f = lambda x: x" -s "s = list(range(1000))" -- "list(map(f, s))"
Unpatched: 10000 loops, best of 3: 89 usec per loop
Patched:   10000 loops, best of 3: 92.3 usec per loop (+3.7%)

$ ./python -m timeit -s "f = lambda x: x" -s "s = list(range(1000))" -- "sorted(s, key=f)"
Unpatched: 10000 loops, best of 3: 104 usec per loop
Patched:   10000 loops, best of 3: 106 usec per loop (+1.9%)

[serhiy-storchaka]

We can ignore such little slow down (if this is not just compiler artifact).

The patch LGTM. I afraid there will be issues with third-party code that will need an adaptation to 3.5, but on other hand, this patch could help with catching some hidden bugs.

[vstinner]

2015-03-06 16:55 GMT+01:00 Serhiy Storchaka <report@bugs.python.org>:

We can ignore such little slow down (if this is not just compiler artifact).

Maybe it's time to play with micro-optimizations like __builtin_expect
:-) But I would prefer to do that in a separated issue. The patch is
already long and complex.

The patch LGTM. I afraid there will be issues with third-party code that will need an adaptation to 3.5, but on other hand, this patch could help with catching some hidden bugs.

My patch doesn't make more strict. Without my patch, if a function
returns a result and raise an exception, the exception will still be
noticed, but probably later. The purpose of my change is to notify
immediatly that the function raised an exception.

Without my patch, if a function returns a result and raise an
exception, it's possible that the exception is cleared and so never
seen.

In older Python versions, Python/ceval.c displayed the message "XXX
undetected error" when an exception was raised but also a result was
returned.

Anyway, you must fix your code :-)