bpo-37388: Check codec error handler in development mode #14341

vstinner · 2019-06-24T14:33:50Z

https://bugs.python.org/issue37388

vstinner · 2019-06-24T14:34:39Z

Proof-of-concept PR to always check the error handle in development mode. If the idea is approved, I will extend it to every decoder and encoder.

serhiy-storchaka

This will add an overhead for every decoding operation, even for decoding an empty or single-byte bytes object.

Objects/unicodeobject.c

vstinner · 2019-06-24T14:45:55Z

Microbenchmark:

./env/bin/python -m pyperf timeit 'b"hello".decode("utf8")' -v --inherit=PYTHONDEVMODE -o FILE.json

ref.json (master branch): 89.0 ns +- 1.9 ns
check.json (with this PR): 90.9 ns +- 0.9 ns
dev_mode.json (with this PR and PYTHONDEVMODE=1): 127 ns +- 3 ns

Cost of this PR, the overhead seems small (2 ns) and may be lower with a PGO build:

Mean +- std dev: [ref] 89.0 ns +- 1.9 ns -> [check] 90.9 ns +- 0.9 ns: 1.02x slower (+2%)

Cost of checking the error handler, it's quite significant, 38 ns on 89 ns:

Mean +- std dev: [ref] 89.0 ns +- 1.9 ns -> [devmode] 127 ns +- 3 ns: 1.43x slower (+43%)

I would appreciate if someone could check with CPU isolation and a build using PGO.

serhiy-storchaka · 2019-06-24T14:50:28Z

Could you please test with an empty and 1-byte data? Use --duplicate to reduce the looping overhead.

vstinner · 2019-06-24T15:11:26Z

Could you please test with an empty and 1-byte data? Use --duplicate to reduce the looping overhead.

I wrote bench.py that I attached to https://bugs.python.org/issue37388:

vstinner@apu$ python3 -m pyperf compare_to ref.json patch.json  --table
+-----------+---------+-----------------------------+
| Benchmark | ref     | patch                       |
+===========+=========+=============================+
| 0B        | 47.4 ns | 50.4 ns: 1.06x slower (+6%) |
+-----------+---------+-----------------------------+
| 1B        | 59.7 ns | 63.0 ns: 1.06x slower (+6%) |
+-----------+---------+-----------------------------+
| 5B        | 85.3 ns | 89.0 ns: 1.04x slower (+4%) |
+-----------+---------+-----------------------------+
| 25B       | 91.0 ns | 85.9 ns: 1.06x faster (-6%) |
+-----------+---------+-----------------------------+
| 1000B     | 248 ns  | 230 ns: 1.08x faster (-7%)  |
+-----------+---------+-----------------------------+

Not significant (2): 10B; 100B

My laptop is not really stable without CPU isolation:

vstinner@apu$ python3 -m pyperf check ref.json patch.json  
ref
===

1000B
-----

WARNING: the benchmark result may be unstable
* the standard deviation (27.1 ns) is 11% of the mean (248 ns)
* the maximum (393 ns) is 59% greater than the mean (248 ns)

Try to rerun the benchmark with more runs, values and/or loops.
Run 'python3 -m pyperf system tune' command to reduce the system jitter.
Use pyperf stats, pyperf dump and pyperf hist to analyze results.
Use --quiet option to hide these warnings.

patch
=====

0B
--

WARNING: the benchmark result may be unstable
* the standard deviation (5.72 ns) is 11% of the mean (50.4 ns)

Try to rerun the benchmark with more runs, values and/or loops.
Run 'python3 -m pyperf system tune' command to reduce the system jitter.
Use pyperf stats, pyperf dump and pyperf hist to analyze results.
Use --quiet option to hide these warnings.

1B
--

WARNING: the benchmark result may be unstable
* the standard deviation (6.45 ns) is 10% of the mean (63.0 ns)

Try to rerun the benchmark with more runs, values and/or loops.
Run 'python3 -m pyperf system tune' command to reduce the system jitter.
Use pyperf stats, pyperf dump and pyperf hist to analyze results.
Use --quiet option to hide these warnings.

vstinner · 2019-06-24T15:12:37Z

Sorry, I forgot my menton that the new benchmark results were measured at commit e77c1e548e0dff8dffc3f68b655014a503eb138f which uses __builtin_expect(x, 0).

vstinner · 2019-06-24T15:17:06Z

Sorry, I forgot my menton that the new benchmark results were measured at commit e77c1e5 which uses __builtin_expect(x, 0).

As expected, I misused this dangerous macro :-) I used UNLIKELY instead of LIKELY causing branch misprediction which adds a 5 ns overhead... Fixed benchmark using LIKELY, commit 09de3a9eefd8934405a1a6c4b9ee065475dbe0a1:

+-----------+---------+-----------------------------+
| Benchmark | ref     | patch                       |
+===========+=========+=============================+
| 1B        | 59.7 ns | 63.4 ns: 1.06x slower (+6%) |
+-----------+---------+-----------------------------+
| 25B       | 91.0 ns | 87.2 ns: 1.04x faster (-4%) |
+-----------+---------+-----------------------------+
| 1000B     | 248 ns  | 231 ns: 1.07x faster (-7%)  |
+-----------+---------+-----------------------------+

Not significant (4): 0B; 5B; 10B; 100B

With std dev:

$ python3 -m pyperf compare_to ref.json patch.json -b 1B
Mean +- std dev: [ref] 59.7 ns +- 4.3 ns -> [patch] 63.4 ns +- 2.5 ns: 1.06x slower (+6%)

$ python3 -m pyperf compare_to ref.json patch.json -b 25B
Mean +- std dev: [ref] 91.0 ns +- 8.3 ns -> [patch] 87.2 ns +- 3.8 ns: 1.04x faster (-4%)

$ python3 -m pyperf compare_to ref.json patch.json -b 1000B
Mean +- std dev: [ref] 248 ns +- 27 ns -> [patch] 231 ns +- 8 ns: 1.07x faster (-7%)

vstinner · 2019-06-24T15:53:06Z

New benchmark run with CPU isolation, the std dev is way better (way smaller). The overhead on b"".decode() is 1.8 ns. It's around 1% in general:

$ python3 -m pyperf compare_to ref.json patch.json
0B: Mean +- std dev: [ref] 53.6 ns +- 0.8 ns -> [patch] 55.4 ns +- 0.7 ns: 1.03x slower (+3%)
1B: Mean +- std dev: [ref] 69.7 ns +- 0.6 ns -> [patch] 73.3 ns +- 1.0 ns: 1.05x slower (+5%)
5B: Mean +- std dev: [ref] 98.4 ns +- 0.7 ns -> [patch] 100 ns +- 0 ns: 1.02x slower (+2%)
10B: Mean +- std dev: [ref] 97.9 ns +- 0.7 ns -> [patch] 99.7 ns +- 0.5 ns: 1.02x slower (+2%)
25B: Mean +- std dev: [ref] 101 ns +- 1 ns -> [patch] 102 ns +- 1 ns: 1.02x slower (+2%)
100B: Mean +- std dev: [ref] 111 ns +- 1 ns -> [patch] 113 ns +- 1 ns: 1.02x slower (+2%)
1000B: Mean +- std dev: [ref] 272 ns +- 2 ns -> [patch] 273 ns +- 3 ns: 1.01x slower (+1%)

$ python3 -m pyperf compare_to ref.json patch.json  --table
+-----------+---------+-----------------------------+
| Benchmark | ref     | patch                       |
+===========+=========+=============================+
| 0B        | 53.6 ns | 55.4 ns: 1.03x slower (+3%) |
+-----------+---------+-----------------------------+
| 1B        | 69.7 ns | 73.3 ns: 1.05x slower (+5%) |
+-----------+---------+-----------------------------+
| 5B        | 98.4 ns | 100 ns: 1.02x slower (+2%)  |
+-----------+---------+-----------------------------+
| 10B       | 97.9 ns | 99.7 ns: 1.02x slower (+2%) |
+-----------+---------+-----------------------------+
| 25B       | 101 ns  | 102 ns: 1.02x slower (+2%)  |
+-----------+---------+-----------------------------+
| 100B      | 111 ns  | 113 ns: 1.02x slower (+2%)  |
+-----------+---------+-----------------------------+
| 1000B     | 272 ns  | 273 ns: 1.01x slower (+1%)  |
+-----------+---------+-----------------------------+

vstinner · 2019-06-24T20:26:16Z

Since @serhiy-storchaka likes the idea, I completed my PR to handle more cases and I wrote documentation.

@serhiy-storchaka: Would you mind to review the update PR?

@methane: Would yiu mind to review it as well? I know that you care about performance of codecs :-)

I removed the LIKELY/UNLIKELY macros. A PGO compilation builds the branch prediction statistics for us automatically. I'm trying to avoid starting to use these macros, since they can have the opposite effect of the expected effect, when misused. See my first attempt: I used UNLIKELY rather than LIKELY, and so added an overhead rather than an optimization...

serhiy-storchaka

What about open()? Should not we add early checks for encoding and errors?

Doc/library/stdtypes.rst

Doc/using/cmdline.rst

Objects/unicodeobject.c

vstinner · 2019-06-25T15:58:38Z

I modified open() to check errors early. open() already checks encoding indirectly. I modified TextIOWrapper in practice.

I added tests.

Oh, in some cases, the encoding is not tested neither:

>>> b''.decode(encoding='xxx')
''
>>> ''.encode(encoding='xxx')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
LookupError: unknown encoding: xxx

I will complete my PR to also test the encoding.

In development mode and in debug build, encoding and errors arguments are now checked on string encoding and decoding operations. Examples: open(), str.encode() and bytes.decode(). By default, for best performances, the errors argument is only checked at the first encoding/decoding error, and the encoding argument is sometimes ignored for empty strings.

vstinner · 2019-06-25T22:25:58Z

I rebased my PR, squashed commits. I add many unit tests on open, byte, bytearray, io.open, _pyio.open. I also added checks on the encoding argument, not only on the errors argument. encoding is ignored sometimes with empty strings.

hroncok · 2019-06-25T22:55:23Z

Thank You!

vstinner · 2019-06-25T23:24:41Z

Thank You!

Thank you, your "Boom, Shaka Laka, Boom!" example is now part of the Python code base!

In development mode and in debug build, encoding and errors arguments are now checked on string encoding and decoding operations. Examples: open(), str.encode() and bytes.decode(). By default, for best performances, the errors argument is only checked at the first encoding/decoding error, and the encoding argument is sometimes ignored for empty strings.

the-knights-who-say-ni added the CLA signed label Jun 24, 2019

bedevere-bot added the awaiting core review label Jun 24, 2019

vstinner requested review from methane and serhiy-storchaka June 24, 2019 14:33

serhiy-storchaka reviewed Jun 24, 2019

View reviewed changes

Objects/unicodeobject.c Outdated Show resolved Hide resolved

serhiy-storchaka approved these changes Jun 24, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jun 24, 2019

vstinner requested a review from serhiy-storchaka June 24, 2019 20:26

serhiy-storchaka reviewed Jun 25, 2019

View reviewed changes

Doc/library/stdtypes.rst Outdated Show resolved Hide resolved

Doc/library/stdtypes.rst Outdated Show resolved Hide resolved

Doc/using/cmdline.rst Outdated Show resolved Hide resolved

methane reviewed Jun 25, 2019

View reviewed changes

Objects/unicodeobject.c Outdated Show resolved Hide resolved

methane approved these changes Jun 25, 2019

View reviewed changes

vstinner requested a review from a team as a code owner June 25, 2019 15:57

vstinner merged commit 22eb689 into python:master Jun 25, 2019

bedevere-bot removed the awaiting merge label Jun 25, 2019

vstinner deleted the unicode_check_errors branch June 25, 2019 22:51

Uh oh!

bpo-37388: Check codec error handler in development mode #14341

bpo-37388: Check codec error handler in development mode #14341

Uh oh!

Conversation

vstinner commented Jun 24, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jun 24, 2019

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Jun 24, 2019

Uh oh!

serhiy-storchaka commented Jun 24, 2019

Uh oh!

vstinner commented Jun 24, 2019

Uh oh!

vstinner commented Jun 24, 2019

Uh oh!

vstinner commented Jun 24, 2019

Uh oh!

vstinner commented Jun 24, 2019

Uh oh!

vstinner commented Jun 24, 2019

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jun 25, 2019

Uh oh!

vstinner commented Jun 25, 2019

Uh oh!

hroncok commented Jun 25, 2019

Uh oh!

vstinner commented Jun 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vstinner commented Jun 24, 2019 •

edited by bedevere-bot

Loading